[Lens] Move dataViews cache into main Lens state by dej611 · Pull Request #137309 · elastic/kibana

dej611 · 2022-07-27T16:19:36Z

Summary

This PR is a required step for #129299 and more future features in Lens.
The dataViews state was historically stored into the specific indexPatterns datasource, which was one of many potential datasources of the editor, but since Visualization starts to know more about it, it had to be lifted up to the editor main state to be reachable.

A new dataViews lens inner group has been created in the main Lens state, which holds refs, dataViews cache, fields existence and potential errors states.
Also the loading phase has been refactored to inspect directly the SO references to load only required dataViews (this sounds like a possible assumption as there is no hidden dataViews concept in Lens yet).

All the dataViews management method, which used to be stored into datasource are now stored within the indexPatternService, which is a tiny API around few functions that can be shared across the Lens app hierarchy.
The current state has not been stored into the indexPatternService as this is used also outside of the frame and it was easier to manage two variables rather than duplicate all the code for the TopNav + EditorFrame + Embeddable .

Checklist

Delete any items that are not applicable to this PR.

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

flash1293 · 2022-08-08T10:00:43Z

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

-                    indexPatternId: dataView.id,
-                    setDatasourceState,
-                  });
+                  dispatchChangeIndexPattern(dataView.id);


When a new data view is created from within Lens, the data panel crashes (seems like the new data view isn't added to the cache correctly?)

reportUnhandledError.js:10 Uncaught TypeError: Cannot read properties of undefined (reading 'fields') at InnerIndexPatternDataPanel (datapanel.tsx:211:1) at nh (react-dom.profiling.min.js:153:1) at ii (react-dom.profiling.min.js:175:1) at hi (react-dom.profiling.min.js:175:1) at fi (react-dom.profiling.min.js:174:1) at ek (react-dom.profiling.min.js:272:1) at dk (react-dom.profiling.min.js:248:1) at Xj (react-dom.profiling.min.js:247:1) at Ij (react-dom.profiling.min.js:240:1) at react-dom.profiling.min.js:122:1 InnerIndexPatternDataPanel @ datapanel

flash1293 · 2022-08-08T10:09:54Z

x-pack/plugins/lens/public/state_management/lens_slice.ts

+      }
+    ) => {
+      const { visualizationIds, datasourceIds, layerId, indexPatternId, dataViews } = payload;
+      const newState: Partial<LensAppState> = { dataViews: { ...state.dataViews, ...dataViews } };


I think I realized this problem - on changing the index pattern, we don't update the list of index pattern refs, but if it has been created on the spot this is necessary. At this point we could make sure that the new index pattern is listed in the refs and add it if that's not the case.

Right. I've added a loading check in the top nav change index pattern function to reload all refs in case the target id is missing in the refs cache.

x-pack/plugins/lens/public/state_management/lens_slice.ts

mbondyra

there's a small bug when switching data view for a layer, doesn't happen for all dimensions.

Aug-11-2022.13-26-53.mp4

x-pack/plugins/lens/public/visualizations/xy/types.ts

dej611 · 2022-08-11T14:30:38Z

@mbondyra the layer dataViews switch bug should have been addressed in the latest version

elasticmachine · 2022-08-11T14:30:47Z

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

flash1293 · 2022-08-11T17:07:33Z

x-pack/plugins/lens/public/visualizations/xy/visualization.tsx

          seriesType: firstUsedSeriesType || state.preferredSeriesType,
          layerId,
          layerType,
+          indexPatternId: indexPatternId ?? core.uiSettings.get('defaultIndex'),


Seems like these bits are related to query based annotations. Does it make sense to keep them here? We definitely need to extract/inject these references and as it's not happening on this PRs it's probably better to split it out of this PR.

flash1293

In terms of the refactoring, this looks great, however it seems like there is a little leftover from the query based annotations that should probably be removed

flash1293

LGTM, tested and didn't find any problems.

flash1293 · 2022-08-12T09:58:27Z

Conflicts are due to the improved error handling, shouldn't be too difficult to resolve

dej611 · 2022-08-12T11:02:32Z

Conflicts should be solved now.

kibana-ci · 2022-08-12T11:51:58Z

💚 Build Succeeded

Buildkite Build
Commit: 958ae77

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	903	906	+3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`lens`	530	535	+5

Async chunks

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

id	before	after	diff
`lens`	1.2MB	1.2MB	+7.4KB

Page load bundle

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

id	before	after	diff
`lens`	34.6KB	34.6KB	+60.0B

Unknown metric groups

API count

id	before	after	diff
`lens`	614	621	+7

History

💚 Build #64729 succeeded 1b64dda
💚 Build #64556 succeeded d317513
💚 Build #64528 succeeded 07e3136
💔 Build #64479 failed 5a5d3b9
💔 Build #64455 failed d32f9dd
💔 Build #64264 failed d594e09

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

mbondyra · 2022-08-12T12:00:12Z

Retested and couldn't find anything not working! Amazing work!

* Move lens dataViews state into main state * 🔥 Remove some old cruft from the code * 🐛 Fix dataViews layer change * 🐛 Fix datasourceLayers refs * 🔥 Remove more old cruft * 🐛 Fix bug when loading SO * 🐛 Fix initial existence flag * 🏷️ Fix type issues * 🏷️ Fix types and tests * 🏷️ Fix types issues * ✅ Fix more tests * ✅ Fix with new dataViews structure * ✅ Fix more test mocks * ✅ More tests fixed * 🔥 Removed unused prop * ✅ Down to single broken test suite * 🏷️ Fix type issue * 👌 Integrate selector feedback * ✅ Fix remaining unit tests * 🏷️ fix type issues * 🐛 Fix bug when creating dataview in place * 🐛 Fix edit + remove field flow * Update x-pack/plugins/lens/public/visualizations/xy/types.ts * 📸 Fix snapshot * 🐛 Fix the dataViews switch bug * 🔥 remove old cruft Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>

dej611 added 2 commits July 27, 2022 17:34

Move lens dataViews state into main state

57d814d

🔥 Remove some old cruft from the code

751c2a2

dej611 added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:Lens labels Jul 27, 2022

dej611 added 9 commits July 27, 2022 18:57

🐛 Fix dataViews layer change

c7a9ce8

🐛 Fix datasourceLayers refs

79566af

🔥 Remove more old cruft

4822b36

🐛 Fix bug when loading SO

220aa39

🐛 Fix initial existence flag

759f19a

🏷️ Fix type issues

7f590da

🏷️ Fix types and tests

12b91fc

🏷️ Fix types issues

2ee6f6d

✅ Fix more tests

ce7bddd

flash1293 mentioned this pull request Aug 2, 2022

[Lens] Ad-hoc data views support #137809

Closed

dej611 and others added 8 commits August 3, 2022 11:59

✅ Fix with new dataViews structure

80647be

✅ Fix more test mocks

63bf020

✅ More tests fixed

3a7f9fb

🔥 Removed unused prop

2fc1f0f

✅ Down to single broken test suite

e9abe5a

Merge remote-tracking branch 'upstream/main' into feature/dataview-state

ca51c4f

🏷️ Fix type issue

13a669e

Merge branch 'main' into feature/dataview-state

120e52a

flash1293 reviewed Aug 8, 2022

View reviewed changes

drewdaemon reviewed Aug 10, 2022

View reviewed changes

x-pack/plugins/lens/public/state_management/lens_slice.ts Outdated Show resolved Hide resolved

👌 Integrate selector feedback

58b873a

stratoula mentioned this pull request Aug 10, 2022

[Lens] Fixes columns type #138385

Closed

dej611 added 3 commits August 10, 2022 16:14

✅ Fix remaining unit tests

70cab64

Merge remote-tracking branch 'upstream/main' into feature/dataview-state

2301d19

🏷️ fix type issues

e68a013

dej611 added 3 commits August 10, 2022 17:01

🐛 Fix bug when creating dataview in place

d594e09

Merge remote-tracking branch 'upstream/main' into feature/dataview-state

d32f9dd

🐛 Fix edit + remove field flow

5a5d3b9

mbondyra reviewed Aug 11, 2022

View reviewed changes

x-pack/plugins/lens/public/visualizations/xy/types.ts Outdated Show resolved Hide resolved

dej611 and others added 3 commits August 11, 2022 14:47

Update x-pack/plugins/lens/public/visualizations/xy/types.ts

0ed3fdf

📸 Fix snapshot

07e3136

🐛 Fix the dataViews switch bug

d317513

dej611 marked this pull request as ready for review August 11, 2022 14:30

dej611 requested a review from a team as a code owner August 11, 2022 14:30

dej611 added release_note:skip Skip the PR/issue when compiling release notes v8.5.0 backport:skip This PR does not require backporting labels Aug 11, 2022

flash1293 reviewed Aug 11, 2022

View reviewed changes

dej611 added 2 commits August 12, 2022 10:09

Merge remote-tracking branch 'upstream/main' into feature/dataview-state

8510724

🔥 remove old cruft

1b64dda

flash1293 approved these changes Aug 12, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feature/dataview-state

958ae77

mbondyra approved these changes Aug 12, 2022

View reviewed changes

stratoula merged commit 137994f into elastic:main Aug 12, 2022

flash1293 mentioned this pull request Aug 18, 2022

[Lens] On missing data view all data views are loaded and initialized #139082

Closed

Conversation

dej611 commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Risk Matrix

For maintainers

Uh oh!

flash1293 Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 Aug 8, 2022

Choose a reason for hiding this comment

Uh oh!

dej611 Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mbondyra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dej611 commented Aug 11, 2022

Uh oh!

elasticmachine commented Aug 11, 2022

Uh oh!

flash1293 Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Aug 12, 2022

Uh oh!

dej611 commented Aug 12, 2022

Uh oh!

kibana-ci commented Aug 12, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

mbondyra commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dej611 commented Jul 27, 2022 •

edited

Loading

flash1293 Aug 8, 2022 •

edited

Loading