Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ES|QL] Canceling the async query from the Lens inline editing flyout #176277

Merged
merged 7 commits into from Feb 8, 2024
Merged

[ES|QL] Canceling the async query from the Lens inline editing flyout #176277

merged 7 commits into from Feb 8, 2024

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Feb 6, 2024 •
edited

Summary

adds support for canceling queries in lens side editor

Checklist

Delete any items that are not applicable to this PR.

@ppisljar ppisljar force-pushed the lenssideeditor/cancel branch 2 times, most recently from 3618ca9 to a3c8f47 Compare February 6, 2024 13:42
@ppisljar ppisljar added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 6, 2024
@ppisljar ppisljar marked this pull request as ready for review February 6, 2024 13:45
@ppisljar ppisljar requested a review from a team as a code owner February 6, 2024 13:45
@elasticmachine
Copy link
Contributor

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

stratoula
stratoula reviewed Feb 7, 2024
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great Peter! Some comments:

  • Can we improve the error message here? Expression is a very kibana oriented term. Something like The request was cancelled. Also I am not sure if this should be an error. Maybe a warning instead? The ES|QL editor also supports warnings.
image

  • Can we change a little bit the styling of the Cancel button? Let's remove the CMD+Enter icons and add a stop icon instead (or X)

  • This cancel logic should not be triggered when the editor consumers don't give the onAbortControllers. For everyone else at this point should work as before. For example check the maps. Here as long as they don't yet support the abortcontroller the text should not change in Cancel.

meow

cc @nreese for this new functionality.

@nreese nreese mentioned this pull request Feb 7, 2024
Open
@nreese
Copy link
Contributor

nreese commented Feb 7, 2024

cc @nreese for this new functionality.

created #176409 to track issue for Maps

@ppisljar ppisljar requested a review from a team as a code owner February 8, 2024 07:37
stratoula
stratoula approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, this works great! I tested it in dashboard, the obs ai assistant and maps and everything works as expected

@ppisljar ppisljar requested a review from a team as a code owner February 8, 2024 08:51
@stratoula
Copy link
Contributor

/ci

@stratoula stratoula changed the title lens esql editor query cancellation [ES|QL] Cancel of the async query from the Lens inline editing flyout Feb 8, 2024
@stratoula stratoula added release_note:enhancement Feature:ES|QL v8.13.0 and removed release_note:skip Skip the PR/issue when compiling release notes labels Feb 8, 2024
@elastic elastic deleted a comment from ppisljar Feb 8, 2024
@elastic elastic deleted a comment from ppisljar Feb 8, 2024
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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 11 13 +2
textBasedLanguages 9 10 +1
total +3

Async chunks

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

id before after diff
dataVisualizer 646.1KB 646.1KB +6.0B
lens 1.4MB 1.4MB +76.0B
securitySolution 11.4MB 11.4MB +60.0B
stackAlerts 82.2KB 82.2KB +77.0B
textBasedLanguages 154.2KB 155.0KB +828.0B
unifiedSearch 217.2KB 217.2KB +5.0B
total +1.0KB
Unknown metric groups

API count

id before after diff
@kbn/text-based-editor 27 30 +3
textBasedLanguages 24 26 +2
total +5

History

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

@stratoula stratoula changed the title [ES|QL] Cancel of the async query from the Lens inline editing flyout [ES|QL] Cancellin the async query from the Lens inline editing flyout Feb 8, 2024
@stratoula stratoula changed the title [ES|QL] Cancellin the async query from the Lens inline editing flyout [ES|QL] Canceling the async query from the Lens inline editing flyout Feb 8, 2024
doakalexi
doakalexi approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@doakalexi doakalexi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseOps changes LGTM

@qn895
Copy link
Member

qn895 commented Feb 8, 2024

ML changes LGTM 🎉

@stratoula stratoula merged commit dac49bd into elastic:main Feb 8, 2024
23 checks passed
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@ppisljar @stratoula @CoenWarmer
[ES|QL] Canceling the async query from the Lens inline editing flyout (…
b526ffd 
…elastic#176277)

## Summary

adds support for canceling queries in lens side editor

### 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)
- [ ] [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [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))

---------

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@ppisljar @stratoula @CoenWarmer
[ES|QL] Canceling the async query from the Lens inline editing flyout (…
9648a5e 
…elastic#176277)

## Summary

adds support for canceling queries in lens side editor

### 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)
- [ ] [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [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))

---------

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
@ppisljar @stratoula @fkanout
[ES|QL] Canceling the async query from the Lens inline editing flyout (…
0f2595b 
…elastic#176277)

## Summary

adds support for canceling queries in lens side editor

### 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)
- [ ] [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [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))

---------

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@qn895 qn895 qn895 approved these changes

@doakalexi doakalexi doakalexi approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:ES|QL release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ppisljar @elasticmachine @nreese @stratoula @kibana-ci @qn895 @doakalexi