[esaggs][inspector]: Refactor to prep for esaggs move to server. #83199

lukeelmers · 2020-11-11T18:35:23Z

Summary

This PR restructures the esaggs expression function in preparation for moving it to the server. It also improves some of the Adapters typings in the inspector plugin so that they provide added type safety in the esaggs request handler.

Following this PR, we'll be able to make some of the needed changes to improve the semantics of esaggs args, and ultimately move the function to common so it can be registered on both client & server.

Changes

Inspector:

Adds typings for TabularData which has been a longstanding TODO item
Adds typings for data and requests adapters to the Adapters interface
- There is no perfect way to do this, as anybody can add Adapters at any point, and the current typings for Adapters are basically [key: string]: any.
- Since data and requests are both baked into the inspector plugin, it felt like we should at least provide type safety for these. It also prevents any potential naming collisions if someone else wants to add an adapter with the same names.
Updates downstream plugins which rely on Adapters to handle the new typings.

Esaggs:

Splits courierRequestHandler out of esaggs and renames requestHandler
Relocates buildTabularInspectorData to esaggs since it's the only place it is used
Refactoring in esaggs, requestHandler, buildTabularInspectorData to extract dependencies on external services
- This will make it easier to plug in each of the service dependencies on both the client & the server, without needing to change the esaggs implementation.

Reviewers

If you're tagged for a codeowner review, it's due to the updated inspector typings, as all other changes are internal to data
It is probably easiest to review this PR commit-by-commit, with "hide whitespace changes" enabled. Otherwise the git diff doesn't make it clear what changes I made to the contents of esaggs & courier request handler

Testing

This PR should introduce no functional changes as it is simply a refactor. It touches the search infrastructure used by Visualizations, including Lens. It also touches the code that logs requests/responses to the inspector in these apps.

lukeelmers · 2020-11-12T21:49:11Z

src/plugins/data/public/autocomplete/providers/value_suggestion_provider.ts

@@ -40,8 +40,10 @@ interface ValueSuggestionsGetFnArgs {
  signal?: AbortSignal;
 }

-const getAutocompleteTimefilter = (indexPattern: IIndexPattern) => {
-  const { timefilter } = getQueryService().timefilter;


When I removed the getQueryService service getter, I realized it was still being used here. Rather than revert the change, I simply updated the autocomplete service since it was a straightforward fix.

lukeelmers · 2020-11-12T21:49:47Z

src/plugins/data/public/plugin.ts

+        getStartDependencies: async () => {
+          const [, , self] = await core.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,
+          };
+        },


All these service dependencies are now wired up in the top-level plugin.ts, which should make it easy to update when we register the function on the server.

Some of these dependencies may also be adjusted when we clean up the esaggs index patterns & aggs args in a follow up PR.

lukeelmers · 2020-11-12T21:52:29Z

src/plugins/discover/public/application/embeddable/search_embeddable.ts

@@ -131,7 +131,7 @@ export class SearchEmbeddable
    this.savedSearch = savedSearch;
    this.$rootScope = $rootScope;
    this.$compile = $compile;
-    this.inspectorAdaptors = {
+    this.inspectorAdapters = {


Changed this to fix a typo :) Adaptors -> Adapters

lukeelmers · 2020-11-12T21:54:18Z

src/plugins/discover/public/application/embeddable/search_embeddable.ts

    const title = i18n.translate('discover.embeddable.inspectorRequestDataTitle', {
      defaultMessage: 'Data',
    });
    const description = i18n.translate('discover.embeddable.inspectorRequestDescription', {
      defaultMessage: 'This request queries Elasticsearch to fetch the data for the search.',
    });

-    const inspectorRequest = this.inspectorAdaptors.requests.start(title, {
+    const inspectorRequest = this.inspectorAdapters.requests!.start(title, {


The only reason this worked without ! or a check for inspectorAdapters.requests was because the Adapters interface was [key: string]: any

Now that requests and data are added as optional properties, TS makes you check for their presence or assert that you know they are there.

lukeelmers · 2020-11-12T21:57:17Z

src/plugins/inspector/common/adapters/types.ts

+  data?: DataAdapter;
+  requests?: RequestAdapter;


Rationale for this change:

data and requests are the most commonly used adapters

they ship in the inspector plugin so it stands to reason that we would provide typings for them

they are optional because technically they aren't guaranteed to be there as they need to be added by apps

putting them here improves type safety, and also helps prevent name collisions

in a perfect world, we could consider a refactor to make this generic so that you can provide type params for other Adapters to it. In the meantime, downstream apps can use casting.

lukeelmers · 2020-11-12T21:58:21Z

src/plugins/inspector/public/views/data/components/data_view.tsx

@@ -44,7 +44,7 @@ interface DataViewComponentState {
  tabularData: TabularData | null;
  tabularOptions: TabularLoaderOptions;
  adapters: Adapters;
-  tabularPromise: TabularCallback | null;
+  tabularPromise: Promise<TabularHolder> | null;


The existing typings were actually incorrect, which was hidden by the fact that TabularData was any

elasticmachine · 2020-11-12T23:18:07Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

ppisljar · 2020-11-16T14:49:41Z

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

 * This function builds tabular data from the response and attaches it to the
 * inspector. It will only be called when the data view in the inspector is opened.
+ *


I think we can get rid of this function (maybe in a separate PR). We had this to support inspector in expressions in the early days but i think right now its no longer needed. Rather, we should:

log the Datatable in every chart function (as esaggs is not the right place, futrther modifications to the datatable could happen further in expression)

update inspector view to work with new datatable format

ppisljar · 2020-11-16T14:52:27Z

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

+  // response data incorrectly in the inspector.
+  let response = (searchSource as any).rawResponse;
+  for (const agg of aggs.aggs) {
+    if (typeof agg.type.postFlightRequest === 'function') {


not related to this PR, but so bad :| terms other bucket only works inside expressions, not with searchsource directly

ppisljar · 2020-11-16T14:53:40Z

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

+  let request;
+  if (inspectorAdapters.requests) {
+    inspectorAdapters.requests.reset();
+    request = inspectorAdapters.requests.start(


hmm .... feels this should rather exist on the searchsource?

kindsun

Maps changes lgtm!

code review

lukeelmers · 2020-11-16T20:42:21Z

Here's the plan @ppisljar and I discussed offline. First, we wrap up this PR and merge it as-is so we can move toward getting esaggs on the server (which is the most time-sensitive priority).

Then we take some follow-up steps:

Generalize buildTabularInspectorData to work with the new Datatable format and export it from the data runtime contract so that other apps can use it. We will be able to extract the filter data from the table itself rather than needing the FilterManager.
Have updates to inspector & the postFlightRequest for aggs "other" bucket happen in SearchSource. It doesn't make sense that you'd get this functionality when using esaggs but not when using SearchSource.
In the end, esaggs should simply be a small function that calls SearchSource with the provided aggs, and runs the result through tabify

kibanamachine · 2020-11-17T20:53:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a35018f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	597	599	+2
`inspector`	60	61	+1
total			+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`data`	245.4KB	245.5KB	+2.0B
`inspector`	39.3KB	39.5KB	+242.0B
`maps`	2.8MB	2.8MB	+138.0B
total			+382.0B

Page load bundle

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

id	before	after	diff
`data`	965.9KB	968.9KB	+3.0KB
`inspector`	49.9KB	52.1KB	+2.2KB
total			+5.2KB

History

💚 Build #87786 succeeded 1004b0b
💚 Build #87756 succeeded 90aa172
💔 Build #87516 failed e7121ca
💔 Build #87418 failed 17715c7

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

kertal

KibanaApp owned code LGTM 👍 , didn't test, just a typo & TypeScript fix, thx for you explanations!

ppisljar

code LGTM

lukeelmers · 2020-11-18T16:10:56Z

@ppisljar I created follow-up issues for the items we discussed here:

[data.search.esaggs] Move 'other' bucket & inspector integrations to SearchSouce. #83652
[data][inspector] Update buildTabularInspectorData to use new Datatable format, & implement in chart fns #83658

) (#83716)

…o-node-details * 'master' of github.com:elastic/kibana: Derive the port from the protocol in cases where it's not explicitly stated (elastic#83583) [CI] Build docker image during packer_cache (elastic#82145) [esaggs][inspector]: Refactor to prep for esaggs move to server. (elastic#83199)

lukeelmers added Feature:Search Querying infrastructure in Kibana WIP Work in progress v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 11, 2020

lukeelmers self-assigned this Nov 11, 2020

lukeelmers force-pushed the feat/esaggs-server-1 branch 2 times, most recently from e7121ca to 90aa172 Compare November 12, 2020 21:47

lukeelmers commented Nov 12, 2020

View reviewed changes

lukeelmers removed the WIP Work in progress label Nov 12, 2020

lukeelmers force-pushed the feat/esaggs-server-1 branch from 90aa172 to 1004b0b Compare November 12, 2020 23:17

lukeelmers requested a review from ppisljar November 12, 2020 23:17

lukeelmers added the Team:AppArch label Nov 12, 2020

lukeelmers marked this pull request as ready for review November 12, 2020 23:18

lukeelmers requested a review from a team November 12, 2020 23:18

lukeelmers requested review from a team as code owners November 12, 2020 23:18

ppisljar reviewed Nov 16, 2020

View reviewed changes

botelastic bot added the Feature:Embedding Embedding content via iFrame label Nov 16, 2020

ppisljar reviewed Nov 16, 2020

View reviewed changes

kindsun approved these changes Nov 16, 2020

View reviewed changes

lukeelmers added 6 commits November 17, 2020 12:04

Split esaggs and request handler.

294a960

Add inspector TabularData typings.

2d22686

Refactor esaggs & courier request handler.

6b3dc60

Improve inspector adapter typings.

b9cedf9

Extract esaggs start dependencies.

c9bfc2f

Remove service getters which are no longer used.

6e7ff36

Update generated docs.

a35018f

lukeelmers force-pushed the feat/esaggs-server-1 branch from 1004b0b to a35018f Compare November 17, 2020 19:13

kertal approved these changes Nov 17, 2020

View reviewed changes

ppisljar approved these changes Nov 18, 2020

View reviewed changes

This was referenced Nov 18, 2020

[data.search.esaggs] Move 'other' bucket & inspector integrations to SearchSouce. #83652

Closed

[data][inspector] Update buildTabularInspectorData to use new Datatable format, & implement in chart fns #83658

Closed

lukeelmers merged commit 62e06ae into elastic:master Nov 18, 2020

lukeelmers deleted the feat/esaggs-server-1 branch November 18, 2020 16:11

lukeelmers mentioned this pull request Nov 18, 2020

[7.x] [esaggs][inspector]: Refactor to prep for esaggs move to server. (#83199) #83716

Merged

lukeelmers added a commit that referenced this pull request Nov 18, 2020

[esaggs][inspector]: Refactor to prep for esaggs move to server. (#83199

5257f0d

) (#83716)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[esaggs][inspector]: Refactor to prep for esaggs move to server. #83199

[esaggs][inspector]: Refactor to prep for esaggs move to server. #83199

lukeelmers commented Nov 11, 2020 •

edited

Loading

lukeelmers Nov 12, 2020

lukeelmers Nov 12, 2020

lukeelmers Nov 12, 2020

lukeelmers Nov 12, 2020

lukeelmers Nov 12, 2020

lukeelmers Nov 12, 2020

elasticmachine commented Nov 12, 2020

ppisljar Nov 16, 2020 •

edited

Loading

ppisljar Nov 16, 2020

ppisljar Nov 16, 2020

kindsun left a comment

lukeelmers commented Nov 16, 2020

kibanamachine commented Nov 17, 2020

kertal left a comment

ppisljar left a comment

lukeelmers commented Nov 18, 2020

[esaggs][inspector]: Refactor to prep for esaggs move to server. #83199

[esaggs][inspector]: Refactor to prep for esaggs move to server. #83199

Conversation

lukeelmers commented Nov 11, 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

elasticmachine commented Nov 12, 2020

ppisljar Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kindsun left a comment

Choose a reason for hiding this comment

lukeelmers commented Nov 16, 2020

kibanamachine commented Nov 17, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

kertal left a comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

lukeelmers commented Nov 18, 2020

lukeelmers commented Nov 11, 2020 •

edited

Loading

ppisljar Nov 16, 2020 •

edited

Loading