[Discover] Allow to store the configured ES|QL visualization v3 #175227
Conversation
… src/core/server/integration_tests/ci_checks'
…earch # Conflicts: # src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts
…o 167887-esql-saved-search-v2
Note that I wasn't suggesting to use the mixed charts icon, I was suggesting to use the icon of whatever chart the user picked in the flyout.
My 2 cents is that for users it might not be immediately clear that the dropdown above the chart is |
This will be tricky technically. This is why I suggested to remove the suggestion dropdown from there and move it to the flyout which has all the logic. It will also be aligned with the dashboards. For now I think is ok to go with the custom + mixed icon though. |
Thanks! Found what was causing it and pushed a fix.
I don't think we can import lens icons directly in Discover. What I tried instead is to reuse a 
|
|
Even better :) Thanx Julia will test again! |
|
@andreadelrio, @stratoula, @jughosta: Tossing my two cents in here. In the short-term, I feel we should either:
I believe doing so would alleviate us from having to show any kind of "customized" indicator, which is something I tend to prefer. My reason for this is because the entire visualization (and all of the user's customizations) can be undone with a simple edit to their ES|QL query. Indicating that it was "customized" might give users the false impression that it is in a kind of frozen state, when in fact it isn't. In the long-term, this is just one of many reasons why I'm not a big fan of hijacking the document count histogram and replacing it with our "suggested" visualizations before the user has made any indications that they wish to create a visualization. I would love for us to explore alternative posibilities that allow us to only reveal such visualization suggestions when the user has expressed interest in creating one. Hopefully something we can explore iteratively as we proceed. |
|
@MichaelMarcialis thanx
I also suggested that and it will simplify the unified histogram code + there will be an alignment with the dashboard inline editing. The second suggestion is much tricker, I would like to avoid bringing it to Discover. |
|
++ @MichaelMarcialis & @stratoula on simplifying. I am also not a fan of the "customized" label and in general confusing having two chart types selectors. We have a challenge with the discoverability of the "edit" icon, but perhaps we can test that as a separate issue. |
|
Cool we are going to proceed with this change, @jughosta I can help next week to bring it to the finish line |
|
Moving back to "draft" as the removal of the dropdown requires another refactoring. |
| break; | ||
| case UnifiedHistogramExternalVisContextStatus.unknown: | ||
| // using `{}` to overwrite the value inside the saved search SO during saving | ||
| stateContainer.internalState.transitions.setOverriddenVisContextAfterInvalidation({}); |
There was a problem hiding this comment.
Oh that's surprising, I was under the impression it was only undefined that gets skipped since I used null for clearing values recently in saved query SOs. In any case it's a minor concern and doesn't impact the functionality, so no big deal.
| const lensEditFlyoutCancelButton = document.getElementById('lnsCancelEditOnFlyFlyout'); | ||
| if (lensEditFlyoutCancelButton) { | ||
| lensEditFlyoutCancelButton.click?.(); | ||
| } |
There was a problem hiding this comment.
Neither do I 😅 But I think it's worth tracking as a cleanup item since I'm sure we can find a nicer way to approach it later, but it's good for now.
| id: `${dataView.timeFieldName} every ${interval}`, | ||
| name: `${dataView.timeFieldName} every ${interval}`, |
There was a problem hiding this comment.
@stratoula can you help me understand why localizing won't work? It's not a big deal for this PR since it was already this way before, but I'd like to understand why.
There was a problem hiding this comment.
Yeah that makes sense. Not something to address for now anyway, mainly just thinking out loud.
| if (!table) { | ||
| delete newLayer.table; | ||
| } |
There was a problem hiding this comment.
Oh that's what I was missing, makes sense 👍
|
@jughosta, @davismcphee pinged me and told me that removing the suggestions logic is a big change and might not make it for 8.14. If this is the case, I would like to proceed with this PR as it is (with the mixed icon for the customized) but then priorite the removal in a follow up PR. So if it makes it, it would be awesome, if not, it will be merged early in the 8.15. The reason I want this merged for 8.14 is because we are going for GA. I want the Discover SOs to save the vis state from this minor as I am expecting that the customers / users are going to start creating more SOs now that the ES|QL is going to be GA. Adding this feature in 8.15, means that many SOs are going to be created in 8.14 without the viz state so the customers need to update them in 8.15. |
|
I agree with @stratoula with the importance of this and with the split of the PRs. Would be optimal if it can make it with the removal of the suggestion, but the risk mitigation is the right path to increase chances of customers saving the state with the SOs when GA in 8.14. 👍 |
|
/ci |
There was a problem hiding this comment.
- Vis Changes LGTM!
- Discover SO behavior LGTM!
- Suggestions / chart configuration changes LGTM!
I tested it thoroughly and it works great. Let's prioritize afterwards the removal of the suggestions dropdown
Great job @jughosta, I know that it was a tricky one 👏
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @jughosta |
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:
type: UnifiedHistogramSuggestionType.histogramForDataViewin this PR)type: UnifiedHistogramSuggestionType.lensSuggestionin this PR) Example query:from kibana_sample_data_logs | stats avg(bytes) by message.keywordtype: UnifiedHistogramSuggestionType.histogramForESQLin this PR). Example query:from kibana_sample_data_logs | limit 10The main flow is that Unified Histogram first picks a suggestion (one of
UnifiedHistogramSuggestionTypetype), then calculates lens attributes which are necessary to build Lens embeddable. With a saved search we are saving those calculated lens attributes undersavedSearch.visContext. For handling this logic, I refactoreduseLensSuggestion,getLensAttributesintoLensVisService.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:
Flow of creating a new search:
Flow of editing a saved search:
Previous details
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 10as query, customize color of a lens histogram, and save it with a saved search. Next I check 2 cases: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 workHere are some nuances with the state management I am seeing which together do not allow to successfully revert unsaved changes:
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.queryprop update is delayed forUnifiedHistogramContainercomponent until Discover finishes the documents fetchkibana/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts
Line 346 in fc2ec95
visContextchanges so we should find another way. With (1) there is another problem that DiscovervisContextstate 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. I tried delayingexternalVisContextprop update too (to keep in sync withqueryupdate) but it does not helpkibana/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts
Line 437 in fc2ec95
kibana/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts
Line 289 in fc2ec95
Other challenges
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 [SavedSearch][Discover] Make sure that lens vis state is persisted after running client side migrations #179151
visContext(large lens attributes object) ends up in the URL too. We should excludevisContextfrom URL sync as it can make the URL too long.Updates here: we are not using appState for this any more
LensVisService.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 rangeonApplyCbinstead of lens attributes? <= the shape does not seem to be different and it works as it is so I'm keeping lens attributesgetLensAttributesfrom [Obs ai assistant][ESQL] Visualizes a query #17467710x flaky test https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5578
Checklist