[Lens] value based ES|QL data source #171081

ppisljar · 2023-11-13T12:02:10Z

Summary

Enhances the ES|QL lens datasource to receive a DataTable as an input (so no datafetching happens in lens)

…atasource

ppisljar · 2023-11-15T07:07:38Z

x-pack/plugins/lens/public/datasources/text_based/types.ts

@@ -21,8 +21,9 @@ export interface TextBasedField {
 }

 export interface TextBasedLayer {
-  index: string;
-  query: AggregateQuery | undefined;
+  index?: string;


when table is provided index and query are optional, which introduces some additional checks in the code for this.

ppisljar · 2023-11-15T07:08:17Z

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

@@ -1088,6 +1107,7 @@ export class Embeddable
                embeddableTitle: this.getTitle(),
                ...(input.palette ? { theme: { palette: input.palette } } : {}),
                ...('overrides' in input ? { overrides: input.overrides } : {}),
+                ...getInternalTables(this.savedVis.state.datasourceStates),


we extract all tables from layers and pass them as variables to expressions, so expressions can later access them using var function

x-pack/plugins/lens/public/datasources/text_based/to_expression.ts

elasticmachine · 2024-01-03T14:55:53Z

Pinging @elastic/kibana-visualizations (Team:Visualizations)

stratoula · 2024-01-03T14:57:28Z

/ci

stratoula · 2024-01-03T15:40:58Z

It seems that there is a circular dependency Peter :/

stratoula

@ppisljar this looks great! We are definitely in the right track! Found some bugs:

The histogram doesnt work in ES|QL mode. We want the tables approach only when the histogramMode is false here https://github.com/elastic/kibana/blob/main/src/plugins/unified_histogram/public/layout/hooks/use_lens_suggestions.ts#L146

It seems as we don't observer the datatable in every refresh. I hit the refresh button multiple times and although the tables values are changing the chart seems to be static.

src/plugins/unified_histogram/public/container/container.tsx

…fix'

stratoula · 2024-01-25T07:31:57Z

ppisljar#12

This PR:

isolates the histogram with the stats mode logic
removes the veil from this embeddable (is not needed and is the reason why sometimes you see this white veil instead of the chart
tries to return the FT on its previous state but I am not sure if I rolled back everything successfully

Fixes based on review

stratoula · 2024-01-25T08:12:23Z

/ci

stratoula · 2024-01-25T12:54:03Z

/ci

ppisljar · 2024-01-25T14:33:13Z

/ci

kibana-ci · 2024-01-25T15:44:08Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 2400cbd

Failed CI Steps

FTR Configs #74

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`unifiedHistogram`	110	111	+1

Async chunks

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

id	before	after	diff
`discover`	564.8KB	565.0KB	+219.0B
`lens`	1.4MB	1.4MB	+539.0B
`unifiedHistogram`	69.6KB	70.4KB	+762.0B
total			+1.5KB

History

💛 Build #189291 was flaky 3a48475
💔 Build #189263 failed c6c0835
💔 Build #189223 failed df3f27b
💔 Build #189204 failed 53a4461
💔 Build #189007 failed 5287cb9
💚 Build #188970 succeeded 6cff500

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

cc @ppisljar

stratoula · 2024-01-26T06:21:16Z

@jughosta can you take a look again? 🙏

jughosta

LGTM 👍

jughosta · 2024-01-26T13:34:48Z

src/plugins/discover/public/application/main/components/layout/discover_histogram_layout.tsx

+      datatable &&
+      datatable &&
+      ['partial', 'complete'].includes(datatable.fetchStatus)


datatable && [FetchStatus.PARTIAL, FetchStatus.COMPLETE].includes(datatable.fetchStatus)

stratoula · 2024-01-26T15:10:42Z

I am merging and address this comment in a follow up PR right after, synced with Julia offline

## Summary Follow up of #171081 - Address Julia's last comment on the PR - Fix the veil bug correctly as Drew proposed here #175550 instead of adding a variable in the embeddable level as I did in the aforementioned PR. We dont want the veil in the embeddable level --------- Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

## Summary Closes elastic#167631 Enhances the ES|QL lens datasource to receive a DataTable as an input (so no datafetching happens in lens) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Stratoula Kalafateli <stratoula1@gmail.com> Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

## Summary Follow up of elastic#171081 - Address Julia's last comment on the PR - Fix the veil bug correctly as Drew proposed here elastic#175550 instead of adding a variable in the embeddable level as I did in the aforementioned PR. We dont want the veil in the embeddable level --------- Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

## Summary Closes elastic#167631 Enhances the ES|QL lens datasource to receive a DataTable as an input (so no datafetching happens in lens) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Stratoula Kalafateli <stratoula1@gmail.com> Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

## Summary Follow up of elastic#171081 - Address Julia's last comment on the PR - Fix the veil bug correctly as Drew proposed here elastic#175550 instead of adding a variable in the embeddable level as I did in the aforementioned PR. We dont want the veil in the embeddable level --------- Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

## Summary Closes elastic#167631 Enhances the ES|QL lens datasource to receive a DataTable as an input (so no datafetching happens in lens) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Stratoula Kalafateli <stratoula1@gmail.com> Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

## Summary Follow up of elastic#171081 - Address Julia's last comment on the PR - Fix the veil bug correctly as Drew proposed here elastic#175550 instead of adding a variable in the embeddable level as I did in the aforementioned PR. We dont want the veil in the embeddable level --------- Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

@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>

ppisljar added Team:Visualizations Visualization editors, elastic-charts and infrastructure 8.13 candidate labels Nov 13, 2023

ppisljar force-pushed the lens/valueBased branch from 91e6263 to 3c333bd Compare November 14, 2023 07:58

elastic deleted a comment from kibana-ci Nov 14, 2023

lens value based source

c873473

ppisljar force-pushed the lens/valueBased branch from 3c333bd to c873473 Compare November 14, 2023 09:26

elastic deleted a comment from kibana-ci Nov 14, 2023

ppisljar force-pushed the lens/valueBased branch from b1072a0 to b9d4e37 Compare November 15, 2023 06:57

extend textBasedDatasource rather than introducing a new valueBased d…

fe255c1

…atasource

ppisljar force-pushed the lens/valueBased branch from b9d4e37 to fe255c1 Compare November 15, 2023 07:05

ppisljar commented Nov 15, 2023

View reviewed changes

x-pack/plugins/lens/public/datasources/text_based/to_expression.ts Show resolved Hide resolved

ppisljar added 2 commits January 3, 2024 06:20

Merge branch 'main' into lens/valueBased

d6caf5d

add support in discover

029de8d

ppisljar marked this pull request as ready for review January 3, 2024 14:55

ppisljar requested review from a team as code owners January 3, 2024 14:55

ppisljar added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Jan 3, 2024

stratoula reviewed Jan 4, 2024

View reviewed changes

src/plugins/unified_histogram/public/container/container.tsx Outdated Show resolved Hide resolved

fixing circular dependency

25a48aa

ppisljar force-pushed the lens/valueBased branch from e27868c to 25a48aa Compare January 4, 2024 09:22

fixing histogram mode

75e1f13

ppisljar force-pushed the lens/valueBased branch from 85f7751 to 75e1f13 Compare January 4, 2024 10:22

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

845ded1

…fix'

mbondyra changed the title ~~lens value based data source~~ [Lens] value based data source Jan 4, 2024

ppisljar and others added 4 commits January 24, 2024 15:46

fixing state issues

c61b3df

fixing state issues

5287cb9

Fixes based on review

5aaf6ab

Clenaup

2351e02

Merge pull request #12 from stratoula/woof-meow-3

53a4461

Fixes based on review

ppisljar added 3 commits January 25, 2024 10:25

fixing test

df3f27b

fixing test

c6c0835

Merge branch 'main' into lens/valueBased

3a48475

Merge branch 'main' into lens/valueBased

2400cbd

jughosta mentioned this pull request Jan 26, 2024

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

Merged

11 tasks

jughosta approved these changes Jan 26, 2024

View reviewed changes

stratoula merged commit 0e3513a into elastic:main Jan 26, 2024
19 checks passed

kibanamachine added the v8.13.0 label Jan 26, 2024

stratoula mentioned this pull request Jan 26, 2024

[Lens] Value based ESQL followup #175691

Merged

stratoula mentioned this pull request Jan 30, 2024

[Lens] Use Lens embeddable only for data fetching #152567

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] value based ES|QL data source #171081

[Lens] value based ES|QL data source #171081

ppisljar commented Nov 13, 2023 •

edited by stratoula

Loading

ppisljar Nov 15, 2023

ppisljar Nov 15, 2023

elasticmachine commented Jan 3, 2024

stratoula commented Jan 3, 2024

stratoula commented Jan 3, 2024

stratoula left a comment

stratoula commented Jan 25, 2024 •

edited

Loading

stratoula commented Jan 25, 2024

stratoula commented Jan 25, 2024

ppisljar commented Jan 25, 2024

kibana-ci commented Jan 25, 2024

stratoula commented Jan 26, 2024

jughosta left a comment

jughosta Jan 26, 2024

stratoula commented Jan 26, 2024

[Lens] value based ES|QL data source #171081

[Lens] value based ES|QL data source #171081

Conversation

ppisljar commented Nov 13, 2023 • edited by stratoula Loading

Summary

ppisljar Nov 15, 2023

Choose a reason for hiding this comment

ppisljar Nov 15, 2023

Choose a reason for hiding this comment

elasticmachine commented Jan 3, 2024

stratoula commented Jan 3, 2024

stratoula commented Jan 3, 2024

stratoula left a comment

Choose a reason for hiding this comment

stratoula commented Jan 25, 2024 • edited Loading

stratoula commented Jan 25, 2024

stratoula commented Jan 25, 2024

ppisljar commented Jan 25, 2024

kibana-ci commented Jan 25, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Async chunks

History

stratoula commented Jan 26, 2024

jughosta left a comment

Choose a reason for hiding this comment

jughosta Jan 26, 2024

Choose a reason for hiding this comment

stratoula commented Jan 26, 2024

ppisljar commented Nov 13, 2023 •

edited by stratoula

Loading

stratoula commented Jan 25, 2024 •

edited

Loading