-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Obs ai assistant][ESQL] Visualizes a query #174677
[Obs ai assistant][ESQL] Visualizes a query #174677
Conversation
import type { AggregateQuery, Query, Filter } from '@kbn/es-query'; | ||
import type { Suggestion } from './types'; | ||
|
||
export const getLensAttributesFromSuggestion = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ I can reuse it in Lens but I will do it on a follow up PR. I moved it on the package because othwerise it creates circular dependencies.
| 'reorder' | ||
| 'layers'; | ||
|
||
export interface Suggestion<T = unknown, V = unknown> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Same as above. This can be reused in Lens and unified-histogram but I will do it on a follow up PR
@@ -49,3 +48,18 @@ export const castEsToKbnFieldTypeName = (esType: ES_FIELD_TYPES | string): KBN_F | |||
*/ | |||
export const getFilterableKbnTypeNames = (): string[] => | |||
registeredKbnTypes.filter((type) => type.filterable).map((type) => type.name); | |||
|
|||
export function normalizeType(type: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ I just moved this helper here as we are duplicating this code in many places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this function takes an Elasticsearch field type and translates it to a Kibana field type? If so, perhaps we could call this function something like
esFieldTypeToKibanaFieldType
normalizeType
could mean practically anything. (I know you didn't name this function BTW—still thought the comment was worth making 🤷♂️ )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is fair, I just moved it and didnt think to rename. I addressed it here e843fec
…ry/index.ts Co-authored-by: Milton Hultgren <[email protected]>
…stratoula/kibana into obs-ai-assistant-visualize-esql
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
</EuiFlyout> | ||
<MultiPaneFlyout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miltonhultgren Your PR should take precedence over this implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miltonhultgren I've integrated your work in this PR. Thank you, it helped a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no, thank you! 🙌🏼
@@ -104,6 +106,7 @@ export function ChatBody({ | |||
knowledgeBase: UseKnowledgeBaseResult; | |||
currentUser?: Pick<AuthenticatedUser, 'full_name' | 'username'>; | |||
startedFrom?: StartedFrom; | |||
chatFlyoutSecondSlotHandler?: ChatFlyoutSecondSlotHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's good enough for now. Lets merge it and see if we can improve it later. An Eui solution would be preferable.
…stratoula/kibana into obs-ai-assistant-visualize-esql
This is how it looked before 4547567
|
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
## Summary This PR 1. Adds a new CTA (Visualize query) on the generated ES|QL queries <img width="965" alt="image" src="https://github.com/elastic/kibana/assets/17003240/3ec3176a-23e1-4329-9d27-a01c6ff8aa92"> 2. Clicking the CTA, requests from Lens to suggest a chart based on the given query <img width="955" alt="image" src="https://github.com/elastic/kibana/assets/17003240/466da7d8-f6c4-4c46-9b51-a7fad5e31e55"> 3. The embeddable has 2 actions: - Edit the embeddable - Save the embeddable on a dashboard 4. Editing the embeddable opens a push flyout where the user can - Change the query - Change the chart configuration (colors, dimensions etc) - Click one of the chart suggestions ![ai_assistant](https://github.com/elastic/kibana/assets/17003240/11fb6a55-60a6-491c-9540-060bebdfaa4a) 5. With clicking the apply button, the new chart configuration is saved to the conversation 6. User can save the ES|QL chart on a dashboard. From there they can continue editing the chart (we also display the same inline editing flyout giving a seamless experience) ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [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] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]> Co-authored-by: Milton Hultgren <[email protected]> Co-authored-by: Coen Warmer <[email protected]>
## Summary This PR 1. Adds a new CTA (Visualize query) on the generated ES|QL queries <img width="965" alt="image" src="https://github.com/elastic/kibana/assets/17003240/3ec3176a-23e1-4329-9d27-a01c6ff8aa92"> 2. Clicking the CTA, requests from Lens to suggest a chart based on the given query <img width="955" alt="image" src="https://github.com/elastic/kibana/assets/17003240/466da7d8-f6c4-4c46-9b51-a7fad5e31e55"> 3. The embeddable has 2 actions: - Edit the embeddable - Save the embeddable on a dashboard 4. Editing the embeddable opens a push flyout where the user can - Change the query - Change the chart configuration (colors, dimensions etc) - Click one of the chart suggestions ![ai_assistant](https://github.com/elastic/kibana/assets/17003240/11fb6a55-60a6-491c-9540-060bebdfaa4a) 5. With clicking the apply button, the new chart configuration is saved to the conversation 6. User can save the ES|QL chart on a dashboard. From there they can continue editing the chart (we also display the same inline editing flyout giving a seamless experience) ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [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] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]> Co-authored-by: Milton Hultgren <[email protected]> Co-authored-by: Coen Warmer <[email protected]>
## Summary This PR 1. Adds a new CTA (Visualize query) on the generated ES|QL queries <img width="965" alt="image" src="https://github.com/elastic/kibana/assets/17003240/3ec3176a-23e1-4329-9d27-a01c6ff8aa92"> 2. Clicking the CTA, requests from Lens to suggest a chart based on the given query <img width="955" alt="image" src="https://github.com/elastic/kibana/assets/17003240/466da7d8-f6c4-4c46-9b51-a7fad5e31e55"> 3. The embeddable has 2 actions: - Edit the embeddable - Save the embeddable on a dashboard 4. Editing the embeddable opens a push flyout where the user can - Change the query - Change the chart configuration (colors, dimensions etc) - Click one of the chart suggestions ![ai_assistant](https://github.com/elastic/kibana/assets/17003240/11fb6a55-60a6-491c-9540-060bebdfaa4a) 5. With clicking the apply button, the new chart configuration is saved to the conversation 6. User can save the ES|QL chart on a dashboard. From there they can continue editing the chart (we also display the same inline editing flyout giving a seamless experience) ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [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] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]> Co-authored-by: Milton Hultgren <[email protected]> Co-authored-by: Coen Warmer <[email protected]>
) - 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 <[email protected]> Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Stratoula Kalafateli <[email protected]>
Summary
This PR
With clicking the apply button, the new chart configuration is saved to the conversation
User can save the ES|QL chart on a dashboard. From there they can continue editing the chart (we also display the same inline editing flyout giving a seamless experience)
Checklist
Delete any items that are not applicable to this PR.