[Discover] Split saved search plugin v2 #174939

jughosta · 2024-01-16T16:21:06Z

Required for [Discover] Allow to store the configured ES|QL visualization v2 #174783

Summary

This PRs separates "search" SO registration and its migrations into a separate plugin. This package can depend on Lens plugins to call Lens migrations.

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
Flaky Test Runner was used on any tests changed
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

jughosta · 2024-01-16T16:22:18Z

src/plugins/saved_search_so/kibana.jsonc

+    "server": true,
+    "browser": false,
+    "requiredPlugins": ["data"],
+    "optionalPlugins": ["lens"],


Kibana starts without errors. Seems to be this circular dependency is resolved.

…fix'

…ocs'

…fix'

jughosta · 2024-02-08T13:31:19Z

/ci

kibana-ci · 2024-02-08T15:26:20Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: f7ec154

Failed CI Steps

Jest Tests #1

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`savedSearch`	47	49	+2

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
`savedSearchSo`	-	3	+3

Page load bundle

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

id	before	after	diff
`savedSearch`	11.5KB	11.4KB	-66.0B

Unknown metric groups

API count

id	before	after	diff
`savedSearchSo`	-	3	+3

ESLint disabled in files

id	before	after	diff
`savedSearch`	1	0	-1

References to deprecated APIs

id	before	after	diff
`savedSearch`	9	4	-5
`savedSearchSo`	-	5	+5
total			-0

Total ESLint disabled count

id	before	after	diff
`savedSearch`	3	2	-1

History

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

cc @jughosta

@timestamp

) - Resolves #167887 ## Summary On Discover page user can see a visualization for data view and ES|QL modes. For ES|QL mode it's also allowed to customize the visualization. This PR allows to save such customization together with a saved search. In more details, various types of Lens visualization can be shown on Discover page: - If in the default (data view) mode, Unified Histogram shows a "formBased" histogram (`type: UnifiedHistogramSuggestionType.histogramForDataView` in this PR) - If in the ES|QL mode, 2 scenarios are possible (so far only these are customizable): - If Lens has suggestions for the current query, Unified Histogram shows one of them (`type: UnifiedHistogramSuggestionType.lensSuggestion` in this PR) Example query: `from kibana_sample_data_logs | stats avg(bytes) by message.keyword` - If Lens suggestion list is empty, Unified Histogram shows a "textBased" histogram (`type: UnifiedHistogramSuggestionType.histogramForESQL` in this PR). Example query: `from kibana_sample_data_logs | limit 10` The main flow is that Unified Histogram first picks a suggestion (one of `UnifiedHistogramSuggestionType` type), then calculates lens attributes which are necessary to build Lens embeddable. With a saved search we are saving those calculated lens attributes under `savedSearch.visContext`. For handling this logic, I refactored `useLensSuggestion`, `getLensAttributes` into `LensVisService`. Restoring a saved customization adds complexity to the flow as it should pick now not just any available suggestion but the suggestion which matches to the previously saved lens attributes. Changes to the current query, time range, time field etc can make the current vis context incompatible and we have to drop the vis customization. This PR already includes this logic of invalidating the stored lens attributes if they are not compatible any more. New vis context will override the previous one when user presses Save for the current search. Until then, we try to restore the customization from the previously saved vis context (for example when the query changes back to the compatible one). What can invalidate the saved vis context and drop the user's customization: - data view id - data view time field name - query/filters - time range if it has a different time interval - text based columns affect what lens suggestions are available Flow of creating a new search: ![1](https://github.com/elastic/kibana/assets/1415710/9274d895-cedb-454a-9a9d-3b0cf600d801) Flow of editing a saved search: ![2](https://github.com/elastic/kibana/assets/1415710/086ce4a0-f679-4d96-892b-631bcfee7ee3) <details> <summary>Previous details</summary> - Previous approach #174373 (saving current suggestion instead of lens attributes) - Previous approach #174783 (saving lens attributes but it's based on existing hooks) But I was stuck with how to make "Unsaved changes" badge work well when user tries to revert changes. For testing in ES|QL mode I use `from kibana_sample_data_logs | limit 10` as query, customize color of a lens histogram, and save it with a saved search. Next I check 2 cases: 1. edit query limit `from kibana_sample_data_logs | limit 100`, see that vis customization gets reset which is expected, press "Revert changes" in the "Unsaved changes" badge => notice that reset did not work 2. edit only histogram color, press "Revert changes" in the "Unsaved changes" badge => notice that reset did not work Here are some nuances with the state management I am seeing which together do not allow to successfully revert unsaved changes: - For ES|QL histogram lens attributes include a modified query `from kibana_sample_data_logs | limit 10 | EVAL timestamp=DATE_TRUNC(30 second, @timestamp) | stats results = count(*) by timestamp | rename timestamp as "@timestamp every 30 second"` which means that not only changes to the original query but also a different time interval invalidates the saved lens attributes. - In ES|QL mode, `query` prop update is delayed for `UnifiedHistogramContainer` component until Discover finishes the documents fetch https://github.com/elastic/kibana/blob/fc2ec957fe78900967da26c80817aea8a0bd2c65/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts#L346 which means that Discover should make a request on revert changes. And It's not happening for (2) as it does not make sense for Discover to trigger refetch if only `visContext` changes so we should find another way. With (1) there is another problem that Discover `visContext` state gets hijacked by lens attributes invalidation logic (as query is not sync yet to UnifiedHistogram) before fetch is completed or get [a chance to be fired](https://github.com/elastic/kibana/blob/6038f92b1fcaeedf635a0eab68fd9cdadd1103d3/src/plugins/discover/public/application/main/hooks/utils/build_state_subscribe.ts#L51-L54). I tried delaying `externalVisContext` prop update too (to keep in sync with `query` update) but it does not help https://github.com/elastic/kibana/blob/fc2ec957fe78900967da26c80817aea8a0bd2c65/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts#L437 - Unified Histogram should signal to Discover to start a refetch when current suggestion changes https://github.com/elastic/kibana/blob/fc2ec957fe78900967da26c80817aea8a0bd2c65/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts#L289 - for some reason this logic is required for "Revert changes" to work as it triggers the refetch. I would expect Discover on its own to notice the change in query and refetch data but it does not seem to be the case. </details> <details> <summary>Other challenges</summary> - [ ] Since we are starting to save lens attributes inside a saved search object (similar to how Dashboard saves lens vis by value), we should integrate Lens migrations into saved search migrations logic. I made a quick PoC how it could look like here jughosta@4529711 This showed that adding Lens plugin as a dependency to saved search plugin causes lots of circular deps in Kibana. To resolve that I am suggesting to spit saved search plugin into 2 plugins #174939 - not the best solution but it seems impossible to split lens plugins instead. Updates here: - [x] revert the code regarding migrations and saved search plugin split - [x] create a github issue to handle client side migrations once their API is available #179151 - [x] Discover syncs app state with URL which means that the new `visContext` (large lens attributes object) ends up in the URL too. We should exclude `visContext` from URL sync as it can make the URL too long. Updates here: we are not using appState for this any more - [x] Changes from #171081 would need to be refactored and integrated into the new `LensVisService`. - [x] Refactor after #177790 - [x] Handle a case when no chart is available for current ES|QL query - [ ] For ES|QL histogram lens attributes include a modified query `from kibana_sample_data_logs | limit 10 | EVAL timestamp=DATE_TRUNC(30 second, @timestamp) | stats results = count(*) by timestamp | rename timestamp as "@timestamp every 30 second"` which means that not only changes to the original query but also a different time range can reset the customization of lens vis as it gets a different time interval based on current time range - New update from Stratoula: - [ ] would it help to persist response of `onApplyCb` instead of lens attributes? <= the shape does not seem to be different and it works as it is so I'm keeping lens attributes - [x] use new `getLensAttributes` from #174677 </details> 10x flaky test https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5578 ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

jughosta added 2 commits January 16, 2024 16:32

[Discover] Extract saved search SO into a separate plugin

1e33a59

[Discover] Try to reference Lens

46b424b

jughosta added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) labels Jan 16, 2024

jughosta self-assigned this Jan 16, 2024

jughosta commented Jan 16, 2024

View reviewed changes

kibanamachine added 2 commits January 16, 2024 19:21

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

fbf6910

…fix'

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

6450c58

This was referenced Jan 26, 2024

[Discover] Allow to store the configured ES|QL visualization v3 #175227

Merged

[Discover] Split savedSearch plugin to prevent circular deps #174925

Closed

jughosta and others added 5 commits January 30, 2024 11:00

Merge branch 'main' into split-saved-search-plugin

003f7be

[CI] Auto-commit changed files from 'node scripts/build_plugin_list_d…

ef7a1ea

…ocs'

Merge branch 'main' into split-saved-search-plugin

caf273a

[Discover] Rearrange consts

4bd7a3d

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

f7ec154

…fix'

jughosta closed this Mar 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Split saved search plugin v2 #174939

[Discover] Split saved search plugin v2 #174939

jughosta commented Jan 16, 2024

jughosta Jan 16, 2024

jughosta commented Feb 8, 2024

kibana-ci commented Feb 8, 2024

API count

ESLint disabled in files

References to deprecated APIs

Total ESLint disabled count

[Discover] Split saved search plugin v2 #174939

[Discover] Split saved search plugin v2 #174939

Conversation

jughosta commented Jan 16, 2024

Summary

Checklist

Risk Matrix

For maintainers

jughosta Jan 16, 2024

Choose a reason for hiding this comment

jughosta commented Feb 8, 2024

kibana-ci commented Feb 8, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Public APIs missing comments

Page load bundle

API count

ESLint disabled in files

References to deprecated APIs

Total ESLint disabled count

History