[data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on the server. #84590

lukeelmers · 2020-11-30T22:41:06Z

Summary

This PR exposes the data plugin's esaggs and indexPatternLoad expression functions on the server as a part of the effort to make esaggs available on the server. (The indexPatternLoad function was moved here because it will be needed for the final step of improving the semantics of the esaggs function).

One complication that arose was that some of the services that esaggs relies on -- such as aggs, index patterns, and search source -- have slightly different contracts on the server in that they must be scoped to a specific user first. To address this, I've added an optional kibanaRequest handler to the expressions execution context. When executing an expression on the server, downstream consumers will need to provide this request object when making calls to run or execute so that the object is made available to the expression functions that need it. Assuming it is provided, the expression should return the same value that it would on the client.

Changes

Splits implementation of esaggs on both client & server
Moves some shared esaggs code to common
Splits implementation of indexPatternLoad on both client & server
- Once improved semantics for esaggs function #61768 is addressed, this will be needed in order for folks to use esaggs
Moves some shared indexPatternLoad code to common
Updates agg types to make expressionName required, and ensure each type has an assigned name
- Each of the agg type functions is already registered on the server, but we need expressionName so you can call aggConfig.toExpressionAst(), the output of which will be used when we refactor the esaggs arguments.

Reviewers

It's recommended to ignore whitespace changes and review this commit-by-commit, as github seems to have trouble displaying the diffs correctly. It looks like a ton of changes, but in reality a lot of them are from updating each of the agg types, which involved touching a bunch of files but was otherwise pretty repetitive.

Testing

This is a refactor that should introduce no functional changes. Areas touched include the querying infrastructure that's shared among all visualizations, including TSVB and lens.

lukeelmers · 2020-12-02T04:20:19Z

src/plugins/data/common/field_formats/field_formats_registry.ts

@@ -42,7 +41,7 @@ export class FieldFormatsRegistry {
  protected metaParamsOptions: Record<string, any> = {};
  protected getConfig?: FieldFormatsGetConfigFn;
  // overriden on the public contract
-  public deserialize: (mapping: SerializedFieldFormat) => IFieldFormat = () => {
+  public deserialize: FormatFactory = () => {


The public contract uses FormatFactory, so this inconsistency caused some type issues when moving esaggs to the server.

lukeelmers · 2020-12-02T04:21:34Z

src/plugins/data/common/search/aggs/buckets/date_range.test.ts

+        "type": "function",
+      }
+    `);
+  });


For the agg types that already had unit tests, I added one to verify the output of toExpressionAst

lukeelmers · 2020-12-02T04:23:26Z

src/plugins/data/common/search/expressions/esaggs/esaggs_fn.ts

+  };
+
+  return table;
+}


I've split out a few things here that we can share on the client & server:

The meta for the expression function definition

The types for the function's dependencies & definition

The request handler & logic that converts a response to a datatable

lukeelmers · 2020-12-02T04:28:08Z

src/plugins/data/public/search/expressions/esaggs.ts

+  return createEsaggs({
+    getStartDependencies: async () => {
+      const [, , self] = await getStartServices();
+      const { fieldFormats, indexPatterns, query, search } = self;
+      return {
+        addFilters: query.filterManager.addFilters.bind(query.filterManager),
+        aggs: search.aggs,
+        deserializeFieldFormat: fieldFormats.deserialize.bind(fieldFormats),
+        indexPatterns,
+        searchSource: search.searchSource,
+      };
+    },
+  });


This is the main difference between the public and server implementations -- basically it's a function that wires up all of the stateful dependencies on various start contracts.

The createEsaggs function above is much simpler: it's just responsible for retrieving the needed pieces of state from those services, and calling the helper that handles the entire request.

The goal here was to isolate differences between client & server code so that we could still share as much as possible from common, and I've used a similar approach with indexPatternLoad. Open to any feedback or other ideas on this.

Discussing this further in this thread.

lukeelmers · 2020-12-02T04:28:58Z

src/plugins/data/server/index_patterns/expressions/load_index_pattern.test.ts

+    expect(result.type).toEqual('index_pattern');
+    expect(result.value.title).toEqual('value');
+  });
+});


There were already tests for indexPatternLoad but I haven't yet added any for esaggs -- my plan was to wait until the next PR as I'll be refactoring the args at that time anyway

lukeelmers · 2020-12-02T04:30:10Z

src/plugins/data/server/search/expressions/esaggs.ts

+      if (!kibanaRequest) {
+        throw new Error(
+          i18n.translate('data.search.esaggs.error.kibanaRequest', {
+            defaultMessage:
+              'A KibanaRequest is required to execute this search on the server. ' +
+              'Please provide a request object to the expression execution params.',
+          })
+        );
+      }


On the server we must have a KibanaRequest or we cannot scope the dependencies of esaggs, so any function that relies on a scoped service will need to throw if this handler is not present (since it's optional).

lukeelmers · 2020-12-02T04:31:37Z

src/plugins/expressions/common/execution/types.ts

+// eslint-disable-next-line @kbn/eslint/no-restricted-paths
+import type { KibanaRequest } from 'src/core/server';


I've tested, and so far it seems that as long as we're only importing a type from server, it doesn't create issues in the bundle as the typescript is compiled away.

lukeelmers · 2020-12-02T04:33:10Z

src/plugins/data/public/plugin.ts

-          };
-        },
-      })
-    );


I moved esaggs function registration to the search service.

Did the same for indexPatternLoad in the index pattern service on the server, however kept it here on the client as there's no separate index pattern "service" that gets called in setup

elasticmachine · 2020-12-02T04:34:45Z

Pinging @elastic/kibana-app-services (Team:AppServices)

src/plugins/expressions/common/execution/types.ts

src/plugins/expressions/common/service/expressions_services.ts

src/plugins/data/public/index_patterns/expressions/load_index_pattern.ts

lukeelmers · 2020-12-02T23:03:42Z

@elasticmachine merge upstream

kibanamachine · 2020-12-03T00:44:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 278cacb

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	602	605	+3

Distributable file count

id	before	after	diff
`default`	43460	43470	+10
`oss`	22744	22754	+10

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	983.3KB	987.2KB	+4.0KB
`expressions`	182.5KB	182.6KB	+93.0B
total			+4.0KB

History

💛 Build #91517 was flaky e2877dd
💚 Build #91511 succeeded d3196c45b786497f0017361bafad20c56dd7ca15
💚 Build #91261 succeeded 50f95784d36124ae814911c6a41bc89bd3853443
💔 Build #90902 failed 1eded798ed142b5dcca38b7816f1585d6bfc2bc6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

ppisljar

code LGTM

… the server. (elastic#84590)

… the server. (#84590) (#84916)

lukeelmers added Feature:Search Querying infrastructure in Kibana WIP Work in progress Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) v8.0.0 v7.11.0 labels Nov 30, 2020

lukeelmers requested a review from ppisljar November 30, 2020 22:41

lukeelmers self-assigned this Nov 30, 2020

lukeelmers force-pushed the feat/esaggs-server-2 branch from 1eded79 to 50f9578 Compare December 2, 2020 00:33

lukeelmers changed the title ~~[data.search] Relocate esaggs to common so it can be exposed on the server.~~ [data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on the server. Dec 2, 2020

lukeelmers added Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes and removed WIP Work in progress labels Dec 2, 2020

lukeelmers commented Dec 2, 2020

View reviewed changes

lukeelmers marked this pull request as ready for review December 2, 2020 04:34

lukeelmers requested a review from a team as a code owner December 2, 2020 04:34

lukeelmers added the Team:AppServices label Dec 2, 2020

lukeelmers added the review label Dec 2, 2020

ppisljar reviewed Dec 2, 2020

View reviewed changes

src/plugins/expressions/common/execution/types.ts Outdated Show resolved Hide resolved

botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Dec 2, 2020

ppisljar reviewed Dec 2, 2020

View reviewed changes

src/plugins/expressions/common/service/expressions_services.ts Show resolved Hide resolved

ppisljar reviewed Dec 2, 2020

View reviewed changes

src/plugins/data/public/index_patterns/expressions/load_index_pattern.ts Show resolved Hide resolved

lukeelmers added 7 commits December 2, 2020 08:16

Move esaggs-related code to common.

4f820fa

Refactor indexPatternLoad and move to common.

54a5f04

Require all agg types to have an expression fn name.

3cd80ed

Add KibanaRequest handler to expression execution context.

19f6203

Refactor how indexPatternLoad & esaggs are structured.

7be4829

Register indexPatternLoad & esaggs on the server.

c0939ce

Add comments to document how dependency injection works.

8b95a6a

lukeelmers force-pushed the feat/esaggs-server-2 branch from 50f9578 to 7b09cbe Compare December 2, 2020 20:02

lukeelmers added 2 commits December 2, 2020 13:32

Wrap KibanaRequest execution context in a getter.

0ad30dd

Update generated docs.

e2877dd

lukeelmers force-pushed the feat/esaggs-server-2 branch from d3196c4 to e2877dd Compare December 2, 2020 20:38

Merge branch 'master' into feat/esaggs-server-2

278cacb

ppisljar approved these changes Dec 3, 2020

View reviewed changes

lukeelmers removed the review label Dec 3, 2020

lukeelmers merged commit d2fc976 into elastic:master Dec 3, 2020

lukeelmers deleted the feat/esaggs-server-2 branch December 3, 2020 15:28

lukeelmers mentioned this pull request Dec 3, 2020

[7.x] [data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on the server. (#84590) #84916

Merged

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Dec 3, 2020

[data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on…

766e692

… the server. (elastic#84590)

lukeelmers added a commit that referenced this pull request Dec 3, 2020

[data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on…

4b65a4d

… the server. (#84590) (#84916)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on the server. #84590

[data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on the server. #84590

lukeelmers commented Nov 30, 2020 •

edited

Loading

lukeelmers Dec 2, 2020

lukeelmers Dec 2, 2020

lukeelmers Dec 2, 2020

lukeelmers Dec 2, 2020

lukeelmers Dec 2, 2020

lukeelmers Dec 2, 2020

lukeelmers Dec 2, 2020

lukeelmers Dec 2, 2020

lukeelmers Dec 2, 2020

elasticmachine commented Dec 2, 2020

lukeelmers commented Dec 2, 2020

kibanamachine commented Dec 3, 2020

ppisljar left a comment

		// eslint-disable-next-line @kbn/eslint/no-restricted-paths
		import type { KibanaRequest } from 'src/core/server';

[data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on the server. #84590

[data.search][data.indexPatterns] Expose esaggs + indexPatternLoad on the server. #84590

Conversation

lukeelmers commented Nov 30, 2020 • edited Loading

Summary

Changes

Reviewers

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 2, 2020

lukeelmers commented Dec 2, 2020

kibanamachine commented Dec 3, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Distributable file count

Page load bundle

History

ppisljar left a comment

Choose a reason for hiding this comment

lukeelmers commented Nov 30, 2020 •

edited

Loading