[Lens] Register saved object references #74523

flash1293 · 2020-08-06T13:33:16Z

This PR makes sure the Lens saved object does not include any direct references to other saved objects.

This changes a lot of things, I tried to list them out here:

Datasource API

We already have a separation of persistable state and runtime state - this can be used for extracting references. This PR extends the API by returning saved object references together with the serialized state. This means getPersistableState is responsible for pulling out all references from the state to turn it persistable. The initialize function is handling the other way around and is injecting references into the runtime state: https://github.com/elastic/kibana/pull/74523/files#diff-c9e959476594f76ae1ce129e8beb7e50R148

Visualization API

We didn't use the differentation of runtime and persistable state for visualizations - in fact, we passed in the wrong shape in some places. This PR just removes the persistable visualization state and the getPersistableState function. initialize is still needed to call side effects.

The toExpression function didn't use the whole frame api, but just the datasource meta information. To make it easier to construct the necessary parts when creating the expression in the embeddable, this PR removes the frame public api and just passses in the datasource layers https://github.com/elastic/kibana/pull/74523/files#diff-c9e959476594f76ae1ce129e8beb7e50R502

Datasource meta data

There is an API for the datasource to return a list of related index patterns. This API returned the id and the title of the index patterns. As we don't needs this API anymore, this PR removes it and simply uses the references created by the datasource instead.

Frame injection/extraction

Extraction of references happens in getSavedObjectFormat: https://github.com/elastic/kibana/pull/74523/files#diff-c4eb3c94dab15e7c5a0438f644a0146b

Injection of references happens in app.tsx for filters and, because it's passed to the filter bar from there: https://github.com/elastic/kibana/pull/74523/files#diff-e074336b6e49a778a50e4f9ba090155aR238

Saved object changes

The saved object has to change to not include hard coded index pattern ids, only references.
The following places currently store ids:

Current index pattern id in datasource (indexpattern-datasource-current-indexpattern)
Per layer index pattern id in datasource (indexpattern-datasource-layer-${layerid})
Per stored filter index pattern id (filter-index-pattern-${index})

In the saved object attributes, these properties can simply be stripped out, because they are always required.

For filters, a indexRefName property is added to the filter definition (this logic was copied from how the search source is handling this): https://github.com/elastic/kibana/pull/74523/files#diff-41572ea9f9059d9f9e4bb899b96c8f51R27

Removing expression

The expression is currently stored in the saved object and used directly from there in the embeddable. This PR removes the expression from the saved object and builds it on the fly when the embeddable is created using a similar logic in the embeddable factory. This requires changes in a few places:

buildExpression helper doesn't work with the whole frame public api, but with some optional properties: https://github.com/elastic/kibana/pull/74523/files#diff-61a6e70d17b513ae29040c0bff3aaa38R115
Do a subset of what the frame is doing in the embeddable factory to build the expression: https://github.com/elastic/kibana/pull/74523/files#diff-1b10d60418cfe2a1421410f7022ccc84R103
Expression is passed into the embeddable instance as input: https://github.com/elastic/kibana/pull/74523/files#diff-1be6dbe5313c6c97d8ec3d8145878ffaR78

Migrating

The existing saved objects need to get migrated. To do so, I copied over the necessary extraction logic into a migration script. It's not using the same code because it would require to move this code into the common folder which would mean quite some refactoring and it would make it easy to change migration code which should be avoided if possible.
https://github.com/elastic/kibana/pull/74523/files#diff-1dad129f516cb546effd934f4ce57a22

The migration also removes the expression from saved objects.

…nce-bug-72855

…d-object-references

…ed-object-references

flash1293 · 2020-08-14T10:23:24Z

Jenkins, test this.

…ed-object-references

elasticmachine · 2020-08-17T12:00:32Z

Pinging @elastic/kibana-app (Team:KibanaApp)

wylieconlon

I tested this out and it definitely was working, but I wasn't happy with the scope of the changes to the embeddable render. I prefer keeping the embeddable as close to output-only as we can. I've started a PR on top of yours which works in 7.10, but does not have migrations or unit tests- let's discuss if that's a better approach.

wylieconlon · 2020-08-18T17:16:35Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.tsx

@@ -107,7 +111,6 @@ export function EditorFrame(props: EditorFrameProps) {
        datasourceLayers[layer] = props.datasourceMap[id].getPublicAPI({
          state: datasourceState,
          layerId: layer,
-          dateRange: props.dateRange,


This change looks unrelated, but I'm fine with the cleanup of the unused dateRange here.

wylieconlon · 2020-08-18T17:17:24Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.tsx

+                  )
+                : props.doc.state.visualization,
+            },
+          },


👍 Good change to actually pass the saved document through the initialization

wylieconlon · 2020-08-18T17:21:30Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/expression_helpers.ts

+  framePublicAPI: {
+    datasourceLayers: Record<string, DatasourcePublicAPI>;
+    query: Query;
+    dateRange?: DateRange;
+    filters: Filter[];
+  };


This change doesn't seem to be required for the PR

Suggested change

framePublicAPI: {

datasourceLayers: Record<string, DatasourcePublicAPI>;

query: Query;

dateRange?: DateRange;

filters: Filter[];

};

framePublicAPI: FramePublicAPI;

I should have expanded on that in the description - this change is necessary because when building the expression in the embeddable factory, we don't have a meaningful date range available. This change (together with the very first one you commented on) is relaxing the requirement to provide a date range in those places.

wylieconlon · 2020-08-18T17:23:28Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/expression_helpers.ts

+  const visualizationExpression = visualization.toExpression(
+    visualizationState,
+    framePublicAPI.datasourceLayers
+  );


I was initially skeptical of this change, but I am now on board.

wylieconlon · 2020-08-18T17:49:02Z

x-pack/plugins/lens/public/app_plugin/app.tsx

+                onChange: ({ filterableIndexPatterns, doc, isSaveable }) => {
+                  if (isSaveable !== state.isSaveable) {
+                    setState((s) => ({ ...s, isSaveable }));
+                  }


Nitpick: This looks like it could trigger a double-render in some cases, if the change is something like clearing a layer. I don't think this is a problem, but wanted to mention it.

wylieconlon · 2020-08-18T17:50:07Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.tsx

-        });
-
-      const doc = getSavedObjectFormat({
+      const { filterableIndexPatterns, doc, isSaveable } = getSavedObjectFormat({


Suggested change

const { filterableIndexPatterns, doc, isSaveable } = getSavedObjectFormat({

props.onChange(getSavedObjectFormat({

wylieconlon · 2020-08-18T18:07:58Z

x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable_factory.ts

 import { getEditPath } from '../../../common';
 import { UiActionsStart } from '../../../../../../src/plugins/ui_actions/public';
+import { buildExpression } from '../editor_frame/expression_helpers';


I think this is the change that has surprised me the most, because it means that we are no longer using the same expressions in the editor and embeddable.

I've found a way to make the expression always the same in the editor and the embeddable by using variables. Please take a look at my PR on your fork!

Thanks for reviewing so thoroughly on both code and architectural level. I realize I never wrote down the reasons to remove the expression, so this is definitely on me.

There are two reasons why I think removing the expression completely for now is the right thing to do:

As the state is redundant with the expression, it's always a source of potential errors in migrations - we basically need to re-implement a small part of the expression building logic in respective migrations and hope it matches what the actual expression building does. In fact, it's what often makes migrations necessary in the first place because changes are limited to the expression. By not saving the expression, this source of bugs (and code/complexity in general) can be removed. We originally decided to save the expression to avoid initializing the datasources in the embeddable, but this is a purely theoretical problem, as we only need access to index patterns which will be cached already anyway.

Specifically, the app arch team plans to change the parameter structure of the esaggs function from a JSON string to nested sub expressions. When regenerating the expression, this change is trivial (just a single line in the toExpression function). With saved expressions, it becomes much more complex, because the logic for generating the nested sub expression ast is not available at migration time.

If your concern is the different code generating the expression in editor vs. embeddable, I could refactor the shared logic into a stateless helper that's used in both places in the same way. Do you think that would work?

This approach was also discussed with the app arch team.

flash1293 · 2020-08-19T08:10:16Z

@elasticmachine merge upstream

…ed-object-references

flash1293 · 2020-08-20T11:45:12Z

@wylieconlon Changed as discussed:

Move the expression building logic out of embeddable factory
Use same code to initialize datasources in embeddable factory and editor frame
Use expression execution search context instead of messing with expression in all places
Remove filterable index patterns and use the references instead

Let's postpone further cleanup work into separate PRs as this PR is already doing multiple things.

wylieconlon

Found a bug in the autorefresh behavior, but overall looking good. Tested the migration from 7.9 -> 7.10 and it worked. Please don't merge until the autorefresh is fixed.

x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_helpers.ts

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

wylieconlon · 2020-08-20T18:10:50Z

x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx

@@ -160,13 +161,36 @@ export class Embeddable extends AbstractEmbeddable<LensEmbeddableInput, LensEmbe
      <ExpressionWrapper
        ExpressionRenderer={this.expressionRenderer}
        expression={this.expression}
-        context={this.currentContext}
+        searchContext={this.getMergedSearchContext()}


I think you're forgetting to pass in the autorefresh parameter, so the dashboard autorefresh is not happening. You'll need to add something like context={{ lastReloadRequestTime: this.externalSearchContext.lastReloadRequestTime }}

wylieconlon · 2020-08-20T18:55:12Z

x-pack/plugins/lens/public/indexpattern_datasource/loader.ts

@@ -82,20 +88,67 @@ const setLastUsedIndexPatternId = (storage: IStorageWrapper, value: string) => {
  writeToStorage(storage, 'indexPatternId', value);
 };

+const CURRENT_PATTERN_REFERENCE_NAME = 'indexpattern-datasource-current-indexpattern';


I'm not sure I see the reasoning behind this- why do we add a reference to the selected index pattern, isn't this only part of the local state, not the persisted state?

No, we are also persisting the current index pattern: https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx#L136

I think we can discuss whether it makes sense, but I would like to do that separately.

x-pack/plugins/lens/public/editor_frame_service/service.tsx

x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx

…ed-object-references

flash1293 · 2020-08-21T10:13:53Z

@wylieconlon Addressed most of your comments, but I think we don't need to pass lastReloadRequestTime down as it's just used in the embeddable itself. Could you take another look?

kibanamachine · 2020-08-21T12:03:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6e545ce

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
lens	255	+2	253

async chunks size

id	value	diff	baseline
lens	25.6KB	+444.0B	25.2KB

page load bundle size

id	value	diff	baseline
lens	866.9KB	+13.2KB	853.7KB

History

💚 Build #69353 succeeded a343e44
💚 Build #68990 succeeded 63c8183
💚 Build #68357 succeeded 2e5d279
💔 Build #68341 failed 6386be5
💔 Build #68136 failed 660cfe8

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

mbondyra

Tested the editor, dashboard and migrations from 7.9. Works as expected.

* master: (71 commits) [Lens] Show 'No data for this field' for empty field in accordion (elastic#73772) Skip failing lens test Configure ScopedHistory consistenty regardless of URL used to mount app (elastic#75074) Fix returned payload by "search" usage collector (elastic#75340) [Security Solution] Fix missing key error (elastic#75576) Upgrade EUI to v27.4.1 (elastic#75240) Update datasets UI copy to data streams (elastic#75618) [Lens] Register saved object references (elastic#74523) [DOCS] Update links to Beats documentation (elastic#70380) [Enterprise Search] Convert our `public_url` route to `config_data` and collect initialAppData (elastic#75616) [Usage Collection Schemas] Remove Legacy entries (elastic#75652) [Dashboard First] Lens Originating App Breadcrumb (elastic#75470) Improve login UI error message. (elastic#75642) [Security Solution] modify circular deps checker to output images of circular deps graphs (elastic#75579) [Data Telemetry] Add index pattern to identify "meow" attacks (elastic#75163) Migrate CSP usage collector to `kibana_usage_collection` plugin (elastic#75536) [Console] Get ES Config from core (elastic#75406) [Uptime] Add delay in telemetry test (elastic#75162) [Lens] Use index pattern service instead saved object client (elastic#74654) Embeddable input (elastic#73033) ...

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…ic#75714) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

)

flash1293 added 14 commits August 6, 2020 10:19

clear out all attribute properties before updating

3ad79cd

fix tests

94293c2

Merge remote-tracking branch 'upstream/master' into lens/fix-persiste…

8e182aa

…nce-bug-72855

Merge branch 'lens/fix-persistence-bug-72855' into lens/register-save…

9f1bf89

…d-object-references

start implementing saved object references

ff69693

implement some more things

a16dad9

add reference stuff

358659b

make references work everywhere

781488e

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

0238da8

…ed-object-references

fix types and tests

2f1c7cd

fix tests

476b6d2

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

8f1b752

…ed-object-references

fix tests

e82c1fa

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

c5066cd

…ed-object-references

flash1293 added Feature:Lens v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 14, 2020

flash1293 added 5 commits August 14, 2020 14:26

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

0f54f77

…ed-object-references

do not show modal on global filters

660cfe8

fix on app leave condition

451fb87

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

6386be5

…ed-object-references

fix datatable test

2e5d279

flash1293 marked this pull request as ready for review August 17, 2020 12:00

flash1293 requested a review from a team August 17, 2020 12:00

flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Aug 17, 2020

wylieconlon suggested changes Aug 18, 2020

View reviewed changes

elasticmachine and others added 5 commits August 19, 2020 02:10

Merge branch 'master' into lens/register-saved-object-references

63c8183

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

02a1ca9

…ed-object-references

refactor

fdf6061

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

a898781

…ed-object-references

fix tests and types

a343e44

wylieconlon approved these changes Aug 20, 2020

View reviewed changes

flash1293 mentioned this pull request Aug 21, 2020

[Lens] App Leave Confirmation is Always Triggered #75607

Closed

flash1293 added 4 commits August 21, 2020 10:55

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

d6a2075

…ed-object-references

remarks

eacb355

Merge remote-tracking branch 'upstream/master' into lens/register-sav…

7017916

…ed-object-references

fix loader test

6e545ce

mbondyra approved these changes Aug 21, 2020

View reviewed changes

flash1293 merged commit 86f73cb into elastic:master Aug 21, 2020

flash1293 mentioned this pull request Aug 21, 2020

[7.x] [Lens] Register saved object references (#74523) #75668

Merged

flash1293 added a commit to flash1293/kibana that referenced this pull request Aug 21, 2020

[Lens] Register saved object references (elastic#74523)

c3b3e7f

flash1293 added a commit that referenced this pull request Aug 21, 2020

[Lens] Register saved object references (#74523) (#75668)

4dde848

brianseeders mentioned this pull request Aug 21, 2020

Failing test: X-Pack Jest Tests.x-pack/plugins/lens/public/app_plugin - Lens App sets originatingApp breadcrumb when the document title changes #75694

Closed

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 22, 2020

Updated and unskipped lens breadcrumb test after elastic#74523

1450a5f

ThomThomson mentioned this pull request Aug 22, 2020

Unskip Lens OriginatingApp Breadcrumb test #75714

Merged

1 task

ThomThomson added a commit that referenced this pull request Aug 24, 2020

Updated and unskipped lens breadcrumb test after #74523 (#75714)

f495b7d

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 24, 2020

Updated and unskipped lens breadcrumb test after elastic#74523 (elast…

5e12a5c

…ic#75714) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

ThomThomson mentioned this pull request Aug 24, 2020

[7.x] Updated and unskipped lens breadcrumb test after #74523 (#75714) #75792

Merged

ThomThomson added a commit that referenced this pull request Aug 24, 2020

Updated and unskipped lens breadcrumb test after #74523 (#75714) (#75792

f4b5c30

)

wylieconlon mentioned this pull request Nov 4, 2020

[Lens] Drilldowns to discover are not shown in dashboard #82657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Register saved object references #74523

[Lens] Register saved object references #74523

flash1293 commented Aug 6, 2020 •

edited

Loading

flash1293 commented Aug 14, 2020

elasticmachine commented Aug 17, 2020

wylieconlon left a comment

wylieconlon Aug 18, 2020

wylieconlon Aug 18, 2020

wylieconlon Aug 18, 2020

flash1293 Aug 19, 2020

wylieconlon Aug 18, 2020

wylieconlon Aug 18, 2020

wylieconlon Aug 18, 2020

wylieconlon Aug 18, 2020

flash1293 Aug 19, 2020 •

edited

Loading

flash1293 commented Aug 19, 2020

flash1293 commented Aug 20, 2020 •

edited

Loading

wylieconlon left a comment

wylieconlon Aug 20, 2020

wylieconlon Aug 20, 2020

flash1293 Aug 21, 2020

flash1293 commented Aug 21, 2020

kibanamachine commented Aug 21, 2020

mbondyra left a comment •

edited

Loading

	const { filterableIndexPatterns, doc, isSaveable } = getSavedObjectFormat({
	props.onChange(getSavedObjectFormat({

[Lens] Register saved object references #74523

[Lens] Register saved object references #74523

Conversation

flash1293 commented Aug 6, 2020 • edited Loading

Datasource API

Visualization API

Datasource meta data

Frame injection/extraction

Saved object changes

Removing expression

Migrating

flash1293 commented Aug 14, 2020

elasticmachine commented Aug 17, 2020

wylieconlon left a 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

Choose a reason for hiding this comment

flash1293 Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

flash1293 commented Aug 19, 2020

flash1293 commented Aug 20, 2020 • edited Loading

wylieconlon left a 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

flash1293 commented Aug 21, 2020

kibanamachine commented Aug 21, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

mbondyra left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 commented Aug 6, 2020 •

edited

Loading

flash1293 Aug 19, 2020 •

edited

Loading

flash1293 commented Aug 20, 2020 •

edited

Loading

mbondyra left a comment •

edited

Loading