Split data and expression plugins by lizozom · Pull Request #45125 · elastic/kibana

lizozom · 2019-09-09T12:51:50Z

Summary

Split data and expression plugins to avoid circular dependencies.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-09-09T12:57:13Z

Pinging @elastic/kibana-app-arch

elasticmachine · 2019-09-09T14:02:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers · 2019-09-09T14:22:39Z

Sorry, I know I've asked this before, but just to clarify: exactly where was the circular dependency again?

Asking because I still think it makes much more sense to keep expressions as a service in data if there is anything we can do to help it. Of course if there is no other choice and it is unavoidable, we've gotta do what we've gotta do.

elasticmachine · 2019-09-09T15:26:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

replace __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED with __LEGACY

lizozom · 2019-09-09T15:59:56Z

@ppisljar actually mentioned this last week on our team sync I think.
He reminded me of this today.

We have a circular dependency between visualizations and data plugins, and we wanted to split those two anyway.

…plugin/split-me

elasticmachine · 2019-09-09T17:17:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers · 2019-09-09T17:21:08Z

We have a circular dependency between visualizations and data plugins, and we wanted to split those two anyway.

Yeah I knew it was a vis/data thing, I was just looking for what the specific dependency was. Something to do with renderers presumably? It would be nice to write that down so we have a documented reason for this decision for future reference.

The other issues we discussed offline (specifically around difficulties with circular deps in testing) can be mitigated with updated mocks and anyway won't be an issue by the time we get to New Platform -- they will just pose challenges during the migration process.

I'm on board with splitting out expressions if it is our only option... just making confirming that this is, in fact, the case 😉

If there's one piece of the data plugin that would make sense to split out, it would probably be expressions anyway. Though I'd still suggest we put any custom expression functions related to data querying (e.g. esaggs) inside of data and register them to the expressions service from there.

…plugin/split-me

elasticmachine · 2019-09-10T10:31:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lizozom · 2019-09-10T10:42:39Z

@lukeelmers I guess @ppisljar might be able to shed some light on what's exactly creating this dependency.

src/plugins/data/common/index.ts

src/plugins/data/common/types.ts

elasticmachine · 2019-09-10T11:46:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-10T13:54:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

src/plugins/expressions/server/index.ts

src/plugins/data/public/plugin.ts

ppisljar · 2019-09-11T07:38:56Z

didn't go thru the code yet, but just to answer the question about circular dependency:

currently expressions service includes renderers registry and RenderHandler.
renderer handler depends on visualization registry
visualizations plugin depends on the data plugin (circular dependency)

there are two approaches we might take to resolve that:

move renderers registry and renderer handler to visulizations plugin. They are not coupled to expressions in any way.
split expressions into its own plugin that has dependency on visalizations and data

there is this other thing Liza is mentioning, when you want to query ES using our low level search service you don't really need anything, but you might want to still use indexPatterns.
when you want to query ES using our high level search API you will need using our search service, you will most likely need: Query, Filters, TimeFilter, AggConfigs and IndexPattern .... but you still won't need the expressions. Expressions come on top of all of this, and when you use them you might or might not use anything else from data plugin. From this perspective it makes sense to me for them to be a separate plugin. But it doesn't say it needs to be.

…plugin/split-me

elasticmachine · 2019-09-11T09:28:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

crob611

Canvas changes look good 👍

ppisljar

code LGTM, one minor comment

ppisljar · 2019-09-12T10:51:39Z

src/legacy/core_plugins/expressions/index.ts

+export default function DataExpressionsPlugin(kibana: any) {
+  const config: Legacy.PluginSpecOptions = {
+    id: 'expressions',
+    require: ['elasticsearch'],


requires data as well

actually, ignore this comment. it shouldn't require data, all the functions that do require data should be registered from data plugin (or another plugin listing data as dependency)

Feedback addressed

* split data plugin from expression plugin * added expressions as dependency to visualizations * ui/agg import * expressions start contract replace __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED with __LEGACY * Rename contract * vis deps * Fix build pipeline mocks * Added expressions plugin to karma mock * update imports * export types from common * Restore types.ts * Remove unused expressions plugin server code

* Split data and expression plugins (#45125) * split data plugin from expression plugin * added expressions as dependency to visualizations * ui/agg import * expressions start contract replace __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED with __LEGACY * Rename contract * vis deps * Fix build pipeline mocks * Added expressions plugin to karma mock * update imports * export types from common * Restore types.ts * Remove unused expressions plugin server code * fixed failing test

rayafratkina · 2019-11-13T21:49:38Z

@lizozom can you please add a DevDocs section?

split data plugin from expression plugin

e73d2ea

lizozom self-assigned this Sep 9, 2019

lizozom added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Feature:NP Migration Team:AppArch v7.5.0 v8.0.0 labels Sep 9, 2019

lizozom added the review label Sep 9, 2019

lizozom requested a review from ppisljar September 9, 2019 12:58

added expressions as dependency to visualizations

c271f38

lizozom marked this pull request as ready for review September 9, 2019 14:13

lizozom requested review from a team as code owners September 9, 2019 14:13

ui/agg import

1cf82a1

Liza K added 3 commits September 9, 2019 18:47

Merge

b6c6c72

expressions start contract

28fc379

replace __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED with __LEGACY

Rename contract

d1a00c3

elastic deleted a comment from elasticmachine Sep 9, 2019

Liza K added 2 commits September 9, 2019 19:35

vis deps

f36943d

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

de42080

…plugin/split-me

lukeelmers mentioned this pull request Sep 9, 2019

[NP] Timelion #44039

Merged

7 tasks

Liza K added 2 commits September 10, 2019 11:27

Fix build pipeline mocks

6f5656f

Added expressions plugin to karma mock

bf38fb6

Liza K added 2 commits September 10, 2019 12:14

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

3e0ad07

…plugin/split-me

update imports

6c771e9

lizozom requested a review from mattkime September 10, 2019 11:04

gospodarsky reviewed Sep 10, 2019

View reviewed changes

src/plugins/data/common/index.ts Show resolved Hide resolved

src/plugins/data/common/types.ts Outdated Show resolved Hide resolved

Liza K added 2 commits September 10, 2019 14:59

export types from common

f806abb

Restore types.ts

88cc327

elastic deleted a comment from elasticmachine Sep 10, 2019

mattkime previously requested changes Sep 10, 2019

View reviewed changes

src/plugins/expressions/server/index.ts Outdated Show resolved Hide resolved

src/plugins/data/public/plugin.ts Show resolved Hide resolved

Liza K added 2 commits September 11, 2019 10:50

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

c57da43

…plugin/split-me

Remove unused expressions plugin server code

add11ef

crob611 approved these changes Sep 11, 2019

View reviewed changes

lizozom requested a review from mattkime September 12, 2019 08:07

ppisljar approved these changes Sep 12, 2019

View reviewed changes

lizozom merged commit a45b0a7 into elastic:master Sep 12, 2019

lizozom mentioned this pull request Sep 12, 2019

[7.x] Split data and expression plugins (#45125) #45516

Merged

wylieconlon mentioned this pull request Sep 12, 2019

[lens] Fix usage of expressions plugin #45544

Merged

lizozom deleted the newplatform/data-plugin/split-me branch November 14, 2019 13:06

streamich removed the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Nov 14, 2019

lizozom added the release_note:skip Skip the PR/issue when compiling release notes label Nov 14, 2019

Conversation

lizozom commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Sep 9, 2019

Uh oh!

elasticmachine commented Sep 9, 2019

💔 Build Failed

Uh oh!

lukeelmers commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 9, 2019

💔 Build Failed

Uh oh!

lizozom commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 9, 2019

💔 Build Failed

Uh oh!

lukeelmers commented Sep 9, 2019

Uh oh!

elasticmachine commented Sep 10, 2019

💔 Build Failed

Uh oh!

lizozom commented Sep 10, 2019

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Sep 10, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Sep 10, 2019

💚 Build Succeeded

Uh oh!

Uh oh!

Uh oh!

ppisljar commented Sep 11, 2019

Uh oh!

elasticmachine commented Sep 11, 2019

💚 Build Succeeded

Uh oh!

crob611 left a comment

Choose a reason for hiding this comment

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

ppisljar Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

ppisljar Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

rayafratkina commented Nov 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lizozom commented Sep 9, 2019 •

edited

Loading

lukeelmers commented Sep 9, 2019 •

edited

Loading

lizozom commented Sep 9, 2019 •

edited

Loading