Remove data <--> expressions circular dependencies. #82685

lukeelmers · 2020-11-05T05:39:53Z

As part of the work to migrate to TS project references in preparation for the new build toolchain, we needed to remove some circular dependencies that exist between the data and expressions plugins.

As explained in the original issue, there were a few areas that were causing problems:

data definitely needs the runtime dependency on expressions, so the question is what items from data is expressions using? Here's the list of what I could find:

toPromise -- helper for AbortHandler

Used in the executor... I can't even find where it is used in data

uniqFilters -- helper for deduping filters

Used in kibana_context

Query Filter and TimeRange interfaces

Used in the ExpressionLoader's ExecutionContextSearch

Used in kibana_context

This PR addresses these issues by doing the following:

Moves AbortSignal helpers toPromise, AbortError, and getCombinedSignal from data to kibana_utils, as these were used in multiple plugins.
- Renames to abortSignalToPromise and getCombinedAbortSignal for clarity
Moves kibana and kibana_context expression functions to the data plugin, and registers them from there instead.
Also moves the kibana_context expression type to data.
Updates the expression plugin's ExecutionContext to take an additional generic type parameter for ExecutionContextSearch, and defaults to SerializableState. Also updates a few places where this was used to pass in the correct param.
Updates a few other places in expressions that used ExecutionContextSearch and replaced with SerializableState, as the search typings should really be determined by data anyway.
Removes data from the expressions plugin's requiredBundles.

lukeelmers · 2020-11-05T05:43:11Z

src/plugins/data/common/search/expressions/utils/function_wrapper.ts

+ * Takes a function spec and passes in default args,
+ * overriding with any provided args.
+ */
+export const functionWrapper = (spec: AnyExpressionFunctionDefinition) => {


This was just copied from expressions. It's simple enough that doesn't feel worth exporting from there, especially as it is only a testing utility.

lukeelmers · 2020-11-05T05:44:22Z

src/plugins/data/server/search/search_service.ts

@@ -77,7 +81,7 @@ type StrategyMap = Record<string, ISearchStrategy<any, any>>;

 /** @internal */
 export interface SearchServiceSetupDependencies {
-  registerFunction: AggsSetupDependencies['registerFunction'];
+  expressions: ExpressionsServerSetup;


Updated setup dependencies for consistency with the public search service, and also as we need registerType from the expressions contract now.

lukeelmers · 2020-11-05T05:45:50Z

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

-export interface ExecutionContext<InspectorAdapters extends Adapters = Adapters> {
+export interface ExecutionContext<
+  InspectorAdapters extends Adapters = Adapters,
+  ExecutionContextSearch extends SerializableState = SerializableState


I still called this ExecutionContextSearch, but at this point expressions only knows that it's something serializable. This means less type safety out of the box (unless you remember to pass the generic type param), however it helps us get rid of a circular type dependency and makes for clearer separation of concerns between the plugins.

lukeelmers · 2020-11-05T05:47:23Z

src/plugins/vis_type_timelion/public/helpers/timelion_request_handler.ts

@@ -59,7 +58,7 @@ export interface TimelionSuccessResponse {
  sheet: Sheet[];
  stats: Stats;
  visType: string;
-  type: KIBANA_CONTEXT_NAME;


This was the only place in Kibana this type was used, and you can derive it from KibanaContext already -- so rather than keeping it in the data plugin contract, I removed it and updated accordingly.

lukeelmers · 2020-11-05T05:49:22Z

src/plugins/vis_type_vega/public/vega_fn.ts

@@ -51,7 +47,7 @@ export type VegaExpressionFunctionDefinition = ExpressionFunctionDefinition<
  Input,
  Arguments,
  Output,
-  ExecutionContext<VegaInspectorAdapters>
+  ExecutionContext<VegaInspectorAdapters, ExecutionContextSearch>


For plugins registering expression functions which rely on the search execution context and want to maintain the same level of type safety as we had previously, this is how they can go about achieving it.

For this PR I did not comb through every registered expression function to make this change; rather, I only looked for places where a specific ExecutionContext was already being included.

lukeelmers · 2020-11-05T21:55:49Z

@elasticmachine merge upstream

elasticmachine · 2020-11-05T23:44:13Z

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

patrykkopycinski

SIEM/Endpoint LGTM

crob611

Presentation changes are good 👍

flash1293

Kibana app changes LGTM, code review only.

lizozom

LGTM in terms of moving things like AbortError, 'toPromiseand such. However I would appreciate someone more involved inexpressionsconfirming that it makes sense to move thekibanaandkibana_contextfunctions into thedata` plugin.

lukeelmers · 2020-11-06T17:18:34Z

I would appreciate someone more involved in expressions confirming that it makes sense to move the kibana and kibana_context functions into the data plugin.

@ppisljar and I discussed kibana_context based on the ideas I outlined in the original issue & agreed it makes sense for it to live in data, because it's loading saved searches, and also parsing query, filter, and timerange. We did not discuss kibana specifically, however it is virtually the same as kibana_context in that it's providing the query/filter/timerange as inputs to each expression. (It wouldn't really make sense to move one without the other)

lukeelmers · 2020-11-09T17:21:54Z

@elasticmachine merge upstream

kibanamachine · 2020-11-09T19:16:12Z

💚 Build Succeeded

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

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	584	587	+3
`expressions`	106	103	-3
`kibanaUtils`	190	191	+1
total			+1

Async chunks

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

id	before	after	diff
`kibanaUtils`	123.2KB	123.2KB	+1.0B
`lens`	1.0MB	1.0MB	+32.0B
`securitySolution`	7.8MB	7.8MB	+731.0B
total			+764.0B

Distributable file count

id	before	after	diff
`default`	42767	42768	+1
`oss`	22452	22453	+1

Page load bundle

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

id	before	after	diff
`data`	955.3KB	962.6KB	+7.3KB
`dataEnhanced`	28.0KB	28.3KB	+309.0B
`expressions`	188.8KB	181.4KB	-7.4KB
`kibanaUtils`	148.0KB	150.2KB	+2.3KB
`securitySolution`	243.3KB	243.6KB	+238.0B
total			+2.7KB

History

💚 Build #86478 succeeded 429f498
💔 Build #86468 failed 2a0d358
💚 Build #86270 succeeded e53c369
💔 Build #86147 failed aa011ac
💔 Build #86041 failed 5dc0bf1

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

* master: (39 commits) Fix ilm navigation (elastic#81664) [Lens] Distinct icons for XY and pie chart value labels toolbar (elastic#82927) [data.search.aggs] Throw an error when trying to create an agg type that doesn't exist. (elastic#81509) Index patterns api - load field list on server (elastic#82629) New events resolver (elastic#82170) [App Search] Misc naming tech debt (elastic#82770) load empty_kibana in test to have clean starting point (elastic#82772) Remove data <--> expressions circular dependencies. (elastic#82685) Update 8.0 breaking change template to gather information on how to programmatically detect it. (elastic#82905) Add alerting as codeowners to related documentation folder (elastic#82777) Add captions to user and space grid pages (elastic#82713) add alternate path for x-pack/Cloud test for Lens (elastic#82634) Uses asCurrentUser in getClusterUuid (elastic#82908) [Alerting][Connectors] Add new executor subaction to get 3rd party case fields (elastic#82519) Fix test import objects (elastic#82767) [ML] Add option for anomaly charts for metric detector should plot min, mean or max as appropriate (elastic#81662) Update alert type selection layout to rows instead of grid (elastic#73665) Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. (elastic#82817) Update grunt and related packages (elastic#79327) Allow the repository to search across all namespaces (elastic#82863) ...

lukeelmers added Feature:Search Querying infrastructure in Kibana Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch NeededFor:Core labels Nov 5, 2020

lukeelmers self-assigned this Nov 5, 2020

lukeelmers commented Nov 5, 2020

View reviewed changes

lukeelmers added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Nov 5, 2020

lukeelmers force-pushed the fix/data-circular-deps branch 3 times, most recently from 5dc0bf1 to aa011ac Compare November 5, 2020 15:45

lukeelmers marked this pull request as ready for review November 5, 2020 23:44

lukeelmers requested a review from a team as a code owner November 5, 2020 23:44

lukeelmers requested a review from a team November 5, 2020 23:44

lukeelmers requested review from a team as code owners November 5, 2020 23:44

patrykkopycinski approved these changes Nov 6, 2020

View reviewed changes

crob611 approved these changes Nov 6, 2020

View reviewed changes

flash1293 approved these changes Nov 6, 2020

View reviewed changes

lukeelmers mentioned this pull request Nov 6, 2020

[Meta] Migrate Kibana plugins to TS project references #80508

Closed

lizozom approved these changes Nov 6, 2020

View reviewed changes

lukeelmers added 4 commits November 6, 2020 10:12

Move abort utils to kibana_utils.

94e8038

Move kibana and kibana_context to the data plugin.

cb0a094

Update ExecutionContext types & remove data from requiredBundles.

af491b0

Update generated docs.

2a0d358

lukeelmers force-pushed the fix/data-circular-deps branch from e53c369 to 2a0d358 Compare November 6, 2020 17:14

Fix type error in lens.

429f498

Merge branch 'master' into fix/data-circular-deps

a165aee

lukeelmers merged commit c6afc47 into elastic:master Nov 9, 2020

lukeelmers deleted the fix/data-circular-deps branch November 9, 2020 20:01

lukeelmers mentioned this pull request Nov 9, 2020

[7.x] Remove data <--> expressions circular dependencies. (#82685) #83010

Merged

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Nov 9, 2020

Remove data <--> expressions circular dependencies. (elastic#82685)

688bfb8

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

Remove data <--> expressions circular dependencies. (#82685) (#83010)

7b0ce96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove data <--> expressions circular dependencies. #82685

Remove data <--> expressions circular dependencies. #82685

lukeelmers commented Nov 5, 2020 •

edited

lukeelmers Nov 5, 2020

lukeelmers Nov 5, 2020

lukeelmers Nov 5, 2020

lukeelmers Nov 5, 2020

lukeelmers Nov 5, 2020

lukeelmers commented Nov 5, 2020

elasticmachine commented Nov 5, 2020

patrykkopycinski left a comment

crob611 left a comment

flash1293 left a comment

lizozom left a comment

lukeelmers commented Nov 6, 2020 •

edited

lukeelmers commented Nov 9, 2020

kibanamachine commented Nov 9, 2020

Remove data <--> expressions circular dependencies. #82685

Remove data <--> expressions circular dependencies. #82685

Conversation

lukeelmers commented Nov 5, 2020 • edited

lukeelmers Nov 5, 2020

Choose a reason for hiding this comment

lukeelmers Nov 5, 2020

Choose a reason for hiding this comment

lukeelmers Nov 5, 2020

Choose a reason for hiding this comment

lukeelmers Nov 5, 2020

Choose a reason for hiding this comment

lukeelmers Nov 5, 2020

Choose a reason for hiding this comment

lukeelmers commented Nov 5, 2020

elasticmachine commented Nov 5, 2020

patrykkopycinski left a comment

Choose a reason for hiding this comment

crob611 left a comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

lizozom left a comment

Choose a reason for hiding this comment

lukeelmers commented Nov 6, 2020 • edited

lukeelmers commented Nov 9, 2020

kibanamachine commented Nov 9, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

Page load bundle

History

lukeelmers commented Nov 5, 2020 •

edited

lukeelmers commented Nov 6, 2020 •

edited