[ES|QL] Edits query in the dashboard

[stratoula]

Summary

Part of #165928
Closes #144498

Allows the user to edit the ES|QL query from the dashboard. Also allows the user to select one of the suggestions.

Testing

Navigate to Discover ES|QL mode and save a Lens chart to a dashboard. Click the edit Visualization.

Important notes

• We can very easily enable suggestions for the dataview panels but I am going to do it on a follow up PR to keep this PR clean
• Creation is going to be added on a follow up PR
• Warnings are not rendered in the editor because I am using the limit 0 for performance reasons. We need to find another way to depict them via the embeddable or store. It will be on a follow up PR.
• Errors are being displayed though. The user is not allowed to apply the changes when an error occurs.
• Creating ES|QL charts from dashboard will happen to a follow up PR

Running queries which don't return numeric fields

In these cases (i.e. from logstash-* | keep clientip we are returning a table. I had to change the datatable logic for text based datasource to not depend to isBucketed flag. This is something we had foreseen from the beginning of text based languages

Running queries which return a lot of fields

For queries with many fields Lens is going to suggest a huge table trying to add the fields to the different dimensions. This is not something we want:

• not performant
• user possibly will start removing fields from the dimensions
• this table is unreadable

For this reason we decided to select the first 5 fields and then the user can easily adjust the dimensions with the fields they want.

Checklist

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• 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)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

[davismcphee]

Quick initial code review of Data Discovery changes.

[davismcphee]

Thanks for the note 👍 Did it not work correctly before or do we just need to do this now because of the changes in this PR?

[stratoula]

@MichaelMarcialis thanx a lot for your thorough review as always! 🙇

Check my comments:

It looks like the word wrapping option has been changed in the ES|QL editor from what was expected. The original design intent was that this would be a simple word wrap on/off setting. Instead, we're now offering a sort of automated formatting based on pipe usage (with word wrap being always on). Is this concept of a pipe wrapping a common one? I don't believe I've ever seen such a pattern in the wild, but I'll admit I'm probably not the typical audience for such a feature. If this is not a common pattern, I'd suggest we revert to the original design intent and change this option back to a pure word wrap on/off setting, with it turned on by default. Thoughts?

This was a feature request. Many customers asked to make it work like that which makes sense to me. It is very useful to wrap and unwrap by pipes so I would like to keep it as it is

The original design intent for the "Run query" button was to keep it always enabled and let users rerun queries as often as they like. It looks like we're now disabling the run button here if no query changes are detected. Should we be disabling the "Run query" button when no query changes have been made? Is there a case for wanting to re-run a query when no changes have been made to the query? For example, what if the data set changes behind the scenes?

I think at this case they will run the Refresh button which is what they already do for the rest of the panels. It is more performant like that, the user wont click it without a reason.

I've never really been a fan of the default EUI resizable button styles in these sorts of full-bleed panels, as it has a tendency to look odd and out-of-place. Can we override the styles or create our own version of the component that basically hides the initial default styles (the two stacked horizontal lines) altogether, and instead only shows the hover and active styles? I realize that will reduce discoverability of the resizing feature, but I'd prefer users to happily stumble across the feature as they move their mouse in the flyout, rather than show an ever-present eyesore. For what it's worth, I think the same direction can be taken for the resize button in the expanded unified search interface in ES|QL mode.

I am fine with that but I prefer to address it in a separate PR. I created an ER here #171379.

Currently, the entire contents of the "Layer configuration" accordion is vertically scrollable. Can we change this to only allow the scrolling of layers panel(s), but not the top toolbar (so as to make the toolbar always visible and accessible)? Doing so would also better match the designs.

The best I can do is to change the toolbar to fixed position. The result will look like this:

If you are ok with it I can push the code. Otherwise I will need some help.

After manually resizing the editor and then open/closing the accordions that follow, it appears that the editor bugs out and moves up a few pixels, cutting off half of the editor header. Would it be possible to fix this bug?

I can't reproduce it. Can you give me more detailed steps?

Opening and closing the accordions shows a brief moment where they jump up and over the editor space. Is there any way we can correct this visual bug?

I can reproduce it but I don't know how to fix it. I will keep looking into it but maybe you have an idea? It is due to the CSS we added for the accordions scroll but I am not sure which specific property creates it.

[mbondyra]

Tested on Chrome, left some small comments but code LGTM 👌🏼

[MichaelMarcialis]

Thanks so much for those changes and comments, @stratoula! I've added my responses below, as well as a few new comments for your re-review.

I'm going to be out on PTO this Thursday and Friday for Thanksgiving in the US and all of next week for the UX Summit. I know you're also out until this Wednesday for ElasticON. Given those circumstances, I'm going to go ahead and approve this now, under the assumption that you're able to address my comments (in the hopes that I don't hold you up for a week+). Thanks!

Response to previous comments

It looks like the word wrapping option has been changed in the ES|QL editor from what was expected. The original design intent was that this would be a simple word wrap on/off setting. Instead, we're now offering a sort of automated formatting based on pipe usage (with word wrap being always on). Is this concept of a pipe wrapping a common one? I don't believe I've ever seen such a pattern in the wild, but I'll admit I'm probably not the typical audience for such a feature. If this is not a common pattern, I'd suggest we revert to the original design intent and change this option back to a pure word wrap on/off setting, with it turned on by default. Thoughts?

This was a feature request. Many customers asked to make it work like that which makes sense to me. It is very useful to wrap and unwrap by pipes so I would like to keep it as it is

Thanks for that context. Given that customers have specifically requested such functionality, I'm not opposed to its inclusion as long as we can make a few small tweaks to accommodate for the differences between it and the previous word-wrapping:

• Rather than reusing the word wrap icons here, perhaps we should create new icons that better represent the addition or removal of line breaks on pipes. If you agree, I'd be happy to make some.

• The boolean-type button (on/off) feels strange in practice. While it made sense for word-wrap to be on a single toggling button, it gets a little muddy with the concept of formatting the query to add or remove line breaks on pipes. It's not a true boolean setting. For example, even if "Wrap with pipes" is disabled, I can still manually format all or part of my query to have line breaks before my pipes. The verbiage may also be contributing to this strangeness, as the action is technically more akin to adding/removing line breaks than "wrapping". To remedy this, would you be open to replacing the single boolean button with two ever-present buttons that allow the user to "Add line breaks on pipes" and "Remove line breaks on pipes"?

Thoughts? If you agree, let me know and I can work up some new icons when I get a moment and we can split this off as a separate issue.

The original design intent for the "Run query" button was to keep it always enabled and let users rerun queries as often as they like. It looks like we're now disabling the run button here if no query changes are detected. Should we be disabling the "Run query" button when no query changes have been made? Is there a case for wanting to re-run a query when no changes have been made to the query? For example, what if the data set changes behind the scenes?

I think at this case they will run the Refresh button which is what they already do for the rest of the panels. It is more performant like that, the user wont click it without a reason.

Given the better performance of the unified search refresh button that you mentioned, would it make sense to change the "Run query" button in the edit visualization flyout to a "Refresh" button, similar to how we do in unified search? That way there is a refresh button local to the section the user is working in (rather than disabling the "Run query" button and asking them to go to a different section on the page to refresh)?

I've never really been a fan of the default EUI resizable button styles in these sorts of full-bleed panels, as it has a tendency to look odd and out-of-place. Can we override the styles or create our own version of the component that basically hides the initial default styles (the two stacked horizontal lines) altogether, and instead only shows the hover and active styles? I realize that will reduce discoverability of the resizing feature, but I'd prefer users to happily stumble across the feature as they move their mouse in the flyout, rather than show an ever-present eyesore. For what it's worth, I think the same direction can be taken for the resize button in the expanded unified search interface in ES|QL mode.

I am fine with that but I prefer to address it in a separate PR. I created an ER here #171379.

Sounds good. Thanks for opening the issue!

Currently, the entire contents of the "Layer configuration" accordion is vertically scrollable. Can we change this to only allow the scrolling of layers panel(s), but not the top toolbar (so as to make the toolbar always visible and accessible)? Doing so would also better match the designs.

The best I can do is to change the toolbar to fixed position.

If you are ok with it I can push the code. Otherwise I will need some help.

Based on this GIF, I think that works. The only things to note are:

• It looks like the left edge shadow of the layer panel is overflowing its scrolling container when the layer panel moves up and out of sight, causing the shadow to then appear behind the toolbar buttons. Would this be possibly to remedy?

• It would be great if we could apply the CSS gradient fade effect (that EUI uses on some of its borderless scrolling containers) at the top of the scroll container.

After manually resizing the editor and then open/closing the accordions that follow, it appears that the editor bugs out and moves up a few pixels, cutting off half of the editor header. Would it be possible to fix this bug?

I can't reproduce it. Can you give me more detailed steps?

It looks like this has been resolved, as I can no longer reproduce it either. Thanks for looking into it!

Opening and closing the accordions shows a brief moment where they jump up and over the editor space. Is there any way we can correct this visual bug?

I can reproduce it but I don't know how to fix it. I will keep looking into it but maybe you have an idea? It is due to the CSS we added for the accordions scroll but I am not sure which specific property creates it.

Let's hold off on this one until some of the markup and styles refactoring that I mention below are implemented, as those changes will likely end up impacting how we decide to tackle this visual bug. We can probably split it off as a separate issue for the time being.

New comments

• If I edit my ES|QL query in the edit visualization flyout and click "Apply and close" before clicking "Run query", none of my query edits are run or saved. Instead, if it's detected that a query change has been made but not yet run, can we have it so that "Apply and close" also run those query changes?

• I noticed that there was a little gray dot appearing in the top-left of the editor chrome. It appears to be caused by one of the tooltip elements for the Monaco editor. Is it possible for us to hide or remove this?

  

• It appears that the height of the "Suggestions" accordion header is correct at 56px, but the height of the "Layer configuration" accordion header is 52px. Can we make both 56px for consistency?

• The padding to the left and right of the accordion header buttons doesn't trigger the accordion. Would it be possible to restructure the markup and styles so that the horizontal padding is part of the button that triggers the accordion (rather than on the container that houses that button)? That way users can click anywhere in those accordion header boxes to expand/contract the accordion contents. Happy to help on the HTML/CSS side if needed.

  

• Similar to the above, the way the markup and styles are currently configured, the "Layer configuration" accordion contents have the scrollbar being kicked in by the right padding. Can we change it so that the padding is applied directly to the element that renders the scrollbar, so as to make the padding appear between the content and scrollbar instead? I imagine the same would need to be done to the "Suggestion" accordion contents as well.

  

• Can we remove the extraneous EuiSpacer under the layer panel visualization type selector?

  

• The suggestion panel buttons are currently using a right margin of 16px to create the horizontal space between each suggestion. Unfortunately, this is also causing a space of 32px between the suggestions and the rightmost edge of their container. Can we remedy this? I imagine this can be achieved by either placing a -16px right margin on their container or using the gap property for flex spacing, instead of margins.

  

• I noticed that at smaller viewport sizes, the accordions weren't scaling down correctly (see screenshot below).

  

  To remedy this, I believe we'll need to apply a style of flex: 1; to the . euiAccordion__childWrapper of any accordion being opened.

  

[davismcphee]

Awesome work and such a cool feature! 🙌 Approving now because the Data Discovery/Unified Histogram changes LGTM, but I noticed a few things while testing that might be worth looking into or following up on later.

It looks like we add a visualization title by default in Discover when saving an ES|QL viz, but it disappears if I dismiss the modal and reopen it (this may have already existed before this PR, but figured it was worth noting):

missing_title.mp4

When I run the ES|QL query in the Dashboard flyout, it changes my selected viz type. Maybe this is expected, but it surprised me so I figured I'd mention it. In this case I modified the query, but it happens even if I don't change it and just run the query with ⌘⏎:

vis_change.mp4

Running the query resets the ES|QL panel height after resizing it:

resize.mp4

Clicking the refresh button in the suggestions panel after selecting a suggestion causes the section to disappear. Maybe this is just because of the rotating number example viz and not a real scenario in prod?

refresh.mp4

[davismcphee]

Makes sense to me, and thanks for the explanation!

[ninoslavmiskovic]

When I try to save the default histogram visualization and edit the query on a dashboard, I see the underlining ES|QL query that I guess is used to create the histogram in Discover.

This could confuse the user since the queries are not the same. - The query on the dashboard make sense, just might cause some confusion that the user does not see the same query.

Skaermoptagelse.2023-11-22.kl.12.37.50.mov

[stratoula]

@MichaelMarcialis

Thoughts? If you agree, let me know and I can work up some new icons when I get a moment and we can split this off as a separate issue.

Absolutely! If you provide me the icons I will address this to a separate PR. Also maybe we can improve the wording here (?)

Based on this GIF, I think that works. The only things to note are ...

I will create a new issue for this and try to improve this on a separate PR. Not sure if addressing your comments will be trivial and I want to merge this PR

would it make sense to change the "Run query" button in the edit visualization flyout to a "Refresh" button, similar to how we do in unified search?

Not sure about it. 🤔 Sometimes it can refresh and sometimes it can run a complete new query. Maybe we can detect if it refreshes or not and change the button text? Let's discuss when you are back and address this to a separate PR.

If I edit my ES|QL query in the edit visualization flyout and click "Apply and close" before clicking "Run query", none of my query edits are run or saved. Instead, if it's detected that a query change has been made but not yet run, can we have it so that "Apply and close" also run those query changes?

I see what you say but this is not as trivial as it seems. I want to think about it. For now I will only allow the Apply to run if changes have been made

The padding to the left and right of the accordion header buttons doesn't trigger the accordion. Would it be possible to restructure the markup and styles so that the horizontal padding is part of the button that triggers the accordion (rather than on the container that houses that button)? That way users can click anywhere in those accordion header boxes to expand/contract the accordion contents. Happy to help on the HTML/CSS side if needed.

I could not make it work. Moving the padding to the button css doesnt respect the arrow so yes I would like some help here!

Similar to the above, the way the markup and styles are currently configured, the "Layer configuration" accordion contents have the scrollbar being kicked in by the right padding. Can we change it so that the padding is applied directly to the element that renders the scrollbar, so as to make the padding appear between the content and scrollbar instead? I imagine the same would need to be done to the "Suggestion" accordion contents as well.

Happy to do that but I am not sure how to do that tbh. I will create an issue and we can look into it together when you are back.

[stratoula]

@davismcphee

It looks like we add a visualization title by default in Discover when saving an ES|QL viz, but it disappears if I dismiss the modal and reopen it (this may have already existed before this PR, but figured it was worth noting):

Correct, this is not related with the changes addressed in this PR. Please create an ER if you want this for Discover. I dont have a strong opinion

When I run the ES|QL query in the Dashboard flyout, it changes my selected viz type. Maybe this is expected, but it surprised me so I figured I'd mention it. In this case I modified the query, but it happens even if I don't change it and just run the query with ⌘⏎:

Expected for now, I guess we could store the selection on the url state but is not planned for now and is unerelated with this PR

Running the query resets the ES|QL panel height after resizing it:

Expected for now but we can def improve. I will create an ER

Clicking the refresh button in the suggestions panel after selecting a suggestion causes the section to disappear. Maybe this is just because of the rotating number example viz and not a real scenario in prod?

Correct, you wont see this in production!

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 6c8f8d7

Failed CI Steps

• x-pack/test_serverless/functional/test_suites/security/common_configs/config.group6.ts
• FTR Configs #12

Test Failures

• [job] [logs] x-pack/test_serverless/functional/test_suites/security/common_configs/config.group6.ts / context app context size "before all" hook for "should default to the context:defaultSize setting"
• [job] [logs] FTR Configs #12 / Entity Analytics - Risk Engine @ess @serverless Risk Scoring Task Execution with auditbeat data with some alerts containing hosts and others containing users @skipInQA calculates and persists risk scores for both types of entities

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
lens	1137	1139	+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
@kbn/text-based-editor	21	10	-11
lens	526	531	+5
textBasedLanguages	20	9	-11
total			-17

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.4MB	1.4MB	+8.4KB
textBasedLanguages	148.9KB	150.5KB	+1.6KB
unifiedHistogram	50.4KB	50.6KB	+177.0B
total			+10.2KB

Page load bundle

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

id	before	after	diff
lens	37.2KB	37.4KB	+148.0B

Unknown metric groups

API count

id	before	after	diff
@kbn/text-based-editor	22	25	+3
lens	625	630	+5
textBasedLanguages	20	23	+3
total			+11

History

• 💚 Build #178315 succeeded e229aa3
• 💚 Build #176518 succeeded 1525d7b
• 💛 Build #176450 was flaky 25e9fcf
• 💔 Build #176430 failed 7f15c78
• 💚 Build #176420 succeeded d94f1a4
• 💚 Build #176187 succeeded 466c911

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

[ninoslavmiskovic]

When I try to save the default histogram visualization and edit the query on a dashboard, I see the underlining ES|QL query that I guess is used to create the histogram in Discover.

This could confuse the user since the queries are not the same. - The query on the dashboard make sense, just might cause some confusion that the user does not see the same query.

Skaermoptagelse.2023-11-22.kl.12.37.50.mov

@stratoula did you have time to look at above 👆

[stratoula]

@ninoslavmiskovic the correct query for the histogram is the one you see in the dashboard. I think that an advanced user understands that the from index doesnt create time buckets and that we are using a different query :D They can also see it on the inspector :D.

I am not sure what we can do here. If the users find it confusing maybe we can discuss with the UX team but it is wrong to use the Discover unified search query for this chart if this is what you imply.

[ninoslavmiskovic]

@stratoula . We have had some users who do not quite understand the difference between the two table modes we have in Discover, and this challenge is kind of connected because it is only present if the user saves the histogram without doing STATS in Discover.

I am not applying to use the Discover query, because as you are stating it is wrong. I am just raising a potential UX challenge.

The challenge gets more visible if the user has evolved the query with e.g. a couple of "WHERE" and "EVAL"s

See example:

ES.QL_Query_discover_vs_dashboard.mov

The query is exactly the same if the user has aggregations and then saves the visualization, but not when they are not, then we are actually also adding a "STATS" the the query.

There is just some inconsistency in the flow. E.g. If you saved the query as a saved search, and you go back and forward you see the same query, but if you save the histogram, then you don't.

ES.QL_saved_search.mov

Showing the full query in Discover that is powering the visualization would not work.

I agree let's hear some UX folks on some input @MichaelMarcialis

[stratoula]

There is just some inconsistency in the flow. E.g. If you saved the query as a saved search, and you go back and forward you see the same query, but if you save the histogram, then you don't.

This is why the saved searches should be renamed differently because as I have said in the past you save the Discover view not only the query

We have had some users who do not quite understand the difference between the two table modes we have in Discover,

You mean on the dataview mode? Because both in dataview mode and ES|QL we have the same concept, different queries create the histogram.

Anyway I feel this discussion is irrelevant with this PR and should not happen here :D

[ninoslavmiskovic]

@stratoula Sure, we can take the discussion in a meeting or so. 👌🏻 For me, it was just important to note that users who go through this flow will not get 1:1 queries as they will when using aggregations starting in Discover.

[MichaelMarcialis]

Thanks for the responses, @stratoula. Please forgive my delayed reply, as I was traveling last week.

Thoughts? If you agree, let me know and I can work up some new icons when I get a moment and we can split this off as a separate issue.

Absolutely! If you provide me the icons I will address this to a separate PR. Also maybe we can improve the wording here (?)

Sounds good. I'll try to work up some icon and wording suggestions for you this week.

Based on this GIF, I think that works. The only things to note are ...

I will create a new issue for this and try to improve this on a separate PR. Not sure if addressing your comments will be trivial and I want to merge this PR

Thank you!

would it make sense to change the "Run query" button in the edit visualization flyout to a "Refresh" button, similar to how we do in unified search?

Not sure about it. 🤔 Sometimes it can refresh and sometimes it can run a complete new query. Maybe we can detect if it refreshes or not and change the button text? Let's discuss when you are back and address this to a separate PR.

Exactly. If we'd be able to enhance this button to detect and conditionally show as "Run query" or "Refresh", that would be great. Let me know if you want to discuss further or want me to write up an issue.

If I edit my ES|QL query in the edit visualization flyout and click "Apply and close" before clicking "Run query", none of my query edits are run or saved. Instead, if it's detected that a query change has been made but not yet run, can we have it so that "Apply and close" also run those query changes?

I see what you say but this is not as trivial as it seems. I want to think about it. For now I will only allow the Apply to run if changes have been made

When you say "I will only allow the Apply to run if changes have been made", I think that's what I'm asking for, as it didn't happen that way in my testing. When "Apply and close" is clicked and the current query has been changed but not run, then "Apply and close" will also save the query changes and run them. Or are you speaking about something different?

The padding to the left and right of the accordion header buttons doesn't trigger the accordion. Would it be possible to restructure the markup and styles so that the horizontal padding is part of the button that triggers the accordion (rather than on the container that houses that button)? That way users can click anywhere in those accordion header boxes to expand/contract the accordion contents. Happy to help on the HTML/CSS side if needed.

I could not make it work. Moving the padding to the button css doesnt respect the arrow so yes I would like some help here!

> Similar to the above, the way the markup and styles are currently configured, the "Layer configuration" accordion contents have the scrollbar being kicked in by the right padding. Can we change it so that the padding is applied directly to the element that renders the scrollbar, so as to make the padding appear between the content and scrollbar instead? I imagine the same would need to be done to the "Suggestion" accordion contents as well.

Happy to do that but I am not sure how to do that tbh. I will create an issue and we can look into it together when you are back.

Sounds good. I'll throw some time on the calendar for us to do some CSS pairing.

[stratoula]

@MichaelMarcialis about this:

When you say "I will only allow the Apply to run if changes have been made", I think that's what I'm asking for, as it didn't happen that way in my testing. When "Apply and close" is clicked and the current query has been changed but not run, then "Apply and close" will also save the query changes and run them. Or are you speaking about something different?

I don't think it is a good idea to run the query when the apply button is clicked. The query might be problematic or not produce a valid suggestion. The current experience makes more sense to me as a user. It tries to not produce problematic panels which I think is the correct experience here. But maybe you are thinking it differently. Let's discuss it synchronously. I have my reservations but happy to hear your thoughts.

[MichaelMarcialis]

I don't think it is a good idea to run the query when the apply button is clicked. The query might be problematic or not produce a valid suggestion. The current experience makes more sense to me as a user. It tries to not produce problematic panels which I think is the correct experience here. But maybe you are thinking it differently. Let's discuss it synchronously. I have my reservations but happy to hear your thoughts.

Yeah, let's plan to talk about it in our Friday meeting. I can appreciate your perspective that you feel running a changed query might be too heavy handed "Apply and close" is clicked. My concern is less about the query running and more of the query changes being saved/stored when "Apply and close" is clicked. To me, it feels strange to not have my query changes saved/stored when I use the apply action. Unfortunately in this case, saving/storing those changes also necessitates running the query, given that the inline editor is being dismissed in the process.