[Lens][Embeddable] Gracefully handle abort fetch requests#246588
[Lens][Embeddable] Gracefully handle abort fetch requests#246588dej611 wants to merge 331 commits into
Conversation
markov00
left a comment
There was a problem hiding this comment.
I've played and I haven't noticed anything in particular, but I believe is also due to the missing handling of abort signal sent and handled from Discover yet.
I was playing with ESQL DELAY function but it doesn't seems to abort yet the query.
So only partially testing this.
| sourceId: ReloadReason | ||
| ) { | ||
| addLog(`Embeddable reload reason: ${sourceId}`); | ||
| loadingReasons.push(sourceId); |
There was a problem hiding this comment.
is it ok to have duplicated reasons?
There was a problem hiding this comment.
You mean something like this?
["searchContext", "searchContext"]
In my mind the array should never stretch more than length 2 in order to recall the previous in flight request.
| import React from 'react'; | ||
| import type { UserMessage } from '@kbn/lens-common'; | ||
|
|
||
| export const ignoredBlockingMessagesByID = new Set<string>(['The expression was aborted.']); |
There was a problem hiding this comment.
Can we make this a bit less fragile and add a constant referring to the expression abort message?
We don't need to export this ignoredBlockingMessagesByID, I believe we are not even going to add more things there
There was a problem hiding this comment.
Ok for the constant.
I believe we are not even going to add more things there
Last famous words :D
|
|
||
| export const ignoredBlockingMessagesByID = new Set<string>(['The expression was aborted.']); | ||
|
|
||
| function extractUniqueId(message: string | { short: string; long: React.ReactNode }): string { |
There was a problem hiding this comment.
| function extractUniqueId(message: string | { short: string; long: React.ReactNode }): string { | |
| function extractErrorMessageText(message: string | { short: string; long: React.ReactNode }): string { |
The error message can 't be actually be considered unique... is a bit strange but ok for now
| export function isIgnorableBlockingMessage(error: ExpressionRenderError | null): boolean { | ||
| const message = error?.original?.message ?? error?.message; | ||
| if (!message) { | ||
| return false; | ||
| } | ||
| const uniqueId = extractUniqueId(message); | ||
| return ignoredBlockingMessagesByID.has(uniqueId); | ||
| } |
There was a problem hiding this comment.
Instead of relying on the message can't we revisit the ExpressionRenderError error adding a property that describe if this can be ignored or not? or giving a Tag or an ID that doesn't rely on the message itself?
We can also consider changing this from a general name AbortError to `ExpressionAbortError' to make it clear and less error prone
There was a problem hiding this comment.
That would limit the support of this skippable logic to only ExpressionRenderError, while I was thinking to have a more wide support by id.
There was a problem hiding this comment.
Instead of relying on the message can't we revisit the ExpressionRenderError error adding a property that describe if this can be ignored or not?
This would impose a Lens logic to the entire Kibana expression system: the fact that Lens wants to skip this specific error should not influence other tools, unless we all agree in Kibana to ignore this specific error type IMHO.
I've tried to use it, but apparently the ES|QL flow has a different handle that his PR doesn't cover yet. |
|
ES|QL cancellation now works differently after #242346. Regarding the chart, yes overall it should get the same I checked the classic mode and it looks great! My suggestion for testing is instead of changing the breakdown field - change the time range now. It's because we changed the logic around the breakdown selection in #245523 and it does not trigger the main request now => hence no "abort" would be called in-between. Changing the time range would still call both refetches after aborting the previous abortController. |
Thanks for the tip, I've updated the testing steps with that now. |
|
Pinging @elastic/kibana-visualizations (Team:Visualizations) |
|
Investigated the failing FTR and apparently there's a regression on the loading bar UI with this PR which makes it fail the test, even tho the numbers are ok. |
…tic#247282) ## Summary ```javascript FROM kibana_sample_data_logs | SORT @timestamp | EVAL now = NOW() | EVAL `ss` = CASE(@timestamp < now - 1 hour AND @timestamp > now - 2 hour, "Last hour", "Other") | STATS count = COUNT(*) BY `ss` | EVAL count_last_hour = CASE(`ss` == "Last hour", count), count_rest = CASE(`ss` == "Other", count) | EVAL total_visits = TO_DOUBLE( COALESCE(count_last_hour, 0::LONG) + COALESCE(count_rest, 0::LONG)) | STATS count_last_hour = SUM(count_last_hour), total_visits = SUM(total_visits) ``` this query generate a validation error : unknow column in: ```javascript count_last_hour = CASE(`ss ```` In STATS ` columns_after.ts,` -> the last fallback block in `BY `failed to strip backticks because it relied on raw query text rather than a normalized identifier.
…ts (elastic#247266) ## Summary This PR fixes an issue with the suggestions widget of the esql editor not being visible inside flyouts. Follow-up to elastic#247187 Initially there was an issue created by recent flyout changes, affecting the position of the widget. But elastic#246170 resolved that unintentionally by rendering the suggestions in its own stacking context (independent from the flyout). ### Screenshots #### Before <img width="560" height="419" alt="Screenshot 2025-12-22 at 11 51 51" src="https://github.com/user-attachments/assets/c59b2726-723f-4854-a65c-55f13ed64f9c" /> #### After <img width="595" height="330" alt="Screenshot 2025-12-22 at 15 20 00" src="https://github.com/user-attachments/assets/7ba9b556-5874-4847-a24c-4b897978709b" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] 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/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Stratou <efstratia.kalafateli@elastic.co>
### Summary Hey folks! We've been doing a review of the recently implemented navigation functioning, and [according to the guidelines](https://docs.google.com/document/d/1WdkXW1SfLpbEyE1tDUzsi9LrZHN8iFrzpH2q6WgZGhQ/edit?tab=t.pn8c8k66xzoq#heading=h.kumkz4ekl6hr) we recommend to avoid the differences between secondary navigation items names and page headers. Currently, it is the case for Logstash pipelines: <img width="2656" height="1018" alt="CleanShot 2025-12-22 at 09 09 41@2x" src="https://github.com/user-attachments/assets/90043eb9-1822-40ab-821f-1960e049359f" /> ### What has been done in this PR. - Changed Pipelines to Logstash pipelines - Changed Ingest Pipelines to Ingest pipelines for consistency, and also because we have a [writing guidelines](https://eui.elastic.co/docs/content/language/#case-and-capitalization) that suggest avoiding using capitalization (for the navigation it will be fixed in a separate PR). Please let me know if some changes are needed, or tag someone who needs to review this as well 🙏🏻 <img width="3456" height="938" alt="CleanShot 2025-12-22 at 09 43 12@2x" src="https://github.com/user-attachments/assets/5b1e1717-e685-40f2-9b6e-09ff963d8654" /> <img width="3456" height="848" alt="CleanShot 2025-12-22 at 09 43 05@2x" src="https://github.com/user-attachments/assets/8455a3fc-cb47-4604-8176-7a63135e30f0" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [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/src/platform/packages/shared/kbn-i18n/README.md) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary This will help making test failures cleaner (see elastic#188963 (comment)).
…lastic#246700) Resolves elastic#227865 The title comparison logic has been updated by removing the lowercasing step before comparing the two titles. As a result, the error no longer appears when only the title casing is changed. The tests have also been updated accordingly. Below is a video demonstrating the correct behavior after the changes. https://github.com/user-attachments/assets/8fa5fb11-14da-4b38-be8a-e3768005c85a --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary
Queries with complex terms inside quotes or triple quotes where breaking
the autocomplete, as our `correctQuerySyntax` function was not working
correctly.
For instance:
`FROM ebt-kibana-browser | WHERE KQL("""/*log.level*/:"something"""") `
was transformed into `FROM ebt-kibana-browser | WHERE
KQL("""/*log.level*/:"something"""") """`
I think we should not try to correct brackets on content that's within
quotes or triple quotes, there is not correct way to deduce if the user
wanted to write something using our special bracket tokens or not, so
this PR disables the correction of query for the content found within
`"` or `"""`.
<img width="1644" height="612" alt="image"
src="https://github.com/user-attachments/assets/e77391d7-a371-43d8-b9e8-12e104d6655f"
/>
---------
Co-authored-by: Stratou <efstratia.kalafateli@elastic.co>
Co-authored-by: Stratou <stratoula1@gmail.com>
## Summary Prevents folks from having to propagate the context from the request handler all the way down to where they need it. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary <img width="2552" height="1232" alt="Screenshot 2025-12-22 at 10 24 25" src="https://github.com/user-attachments/assets/e7a3cf7d-a203-458e-b31f-ab61b2995877" /> <img width="2559" height="1225" alt="Screenshot 2025-12-22 at 10 23 44" src="https://github.com/user-attachments/assets/52d9176e-b835-48db-a71d-4eb95511b8fa" />
Fixes elastic#246916 ## Summary This PR fixes the json type settings field, which was incorrectly saving an empty string (invalid json) if the user inputs an empty value. Instead, it should save a corresponding valid json (`{}` or `[]` depending on whether the default value is an array). **How to test:** 1. Go to Advanced settings -> Stack management 2. Set any of the json fields (for example, `dateFormat:scaled`) to an empty string and save. 3. Click on "Reset to default" and try to save. It should be saved successfully.
…6952) ## Summary This PR adds the **Critical Path visualization** to the unified trace waterfall in Discover, following the same behavior and UX patterns used in the existing APM trace waterfall. A new `CriticalPathToggle` component was created, along with visual overlays that highlight the critical path segments within the trace timeline. The implementation reuses the core algorithm from APM (`getCriticalPath`: adapted to support generics types) while adapting the filtering logic to work with Discover's flat map structure (`traceWaterfallMap`). ## How to test 1. Navigate to Discover and select a trace document 2. Open the trace waterfall view 3. Click the **"Show critical path"** toggle in the waterfall header 4. Verify that: - Only items in the critical path remain visible - Critical path segments are highlighted with colored overlays on the timeline bars - The waterfall maintains proper parent-child relationships 5. Toggle off to return to the full trace view Also, you can compare with APM behavior ## Demo https://github.com/user-attachments/assets/a7fdc7f9-6ec1-4f24-acdb-1191d3acf790 ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] 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/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: Cauê Marcondes <55978943+cauemarcondes@users.noreply.github.com>
…lastic#247084) Closes elastic#241429 ## Summary Since the Flamegraph component requires WebGL to render correctly, we should display the relevant callout when the library is disabled (for instance GPU acceleration is disabled by default on Chrome browser in Debian 12 linux distribution). Before (no console errors): <img width="1723" height="1080" alt="image" src="https://github.com/user-attachments/assets/fdc60a21-130c-49c3-9d47-e2f21d31dca2" /> After: <img width="1726" height="1082" alt="image" src="https://github.com/user-attachments/assets/d7407ba8-3916-4ab4-9c1a-32f8cc16f1a4" />
…ted is greater than 0 (elastic#246716) ## Summary Fix flaky test: increase the rule execution schedule from 1s to 5s to avoid having duplicate alerts which make the test fail.
…nsistent 24-hour time window (elastic#247222) It closes elastic/streams-program#668. ### Summary This PR updates all AI generation features (stream description and significant events) to consistently use the last 24 hours of data, matching the existing behavior of feature identification.
Closes elastic#221397 ## Summary This test was flaky. Added `await testSubjects.waitForDeleted('consoleSkipTourButton') `after skipping the tour to wait for the popover to fully close before asserting all steps are hidden. Flaky test runner: ~https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/10211~ This was cancelled and failed for other test. New run: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/10217
…JS template literals as policy variables (elastic#247284) ## Summary Related to elastic#228696 Use Unicode escaping to prevent the Agent from interpreting JS template literals as policy variables <img width="1724" height="539" alt="Image" src="https://github.com/user-attachments/assets/e5601d2c-90f1-4206-a3dc-1c4c42f0d0e4" />
|
uff, sorry for the ping folks. Bad rebase. |
🔍 Preview links for changed docs |
⏳ Build in-progress, with failures
Failed CI StepsHistory
|
Summary
Fixes #226313, built on top of #241184
It partially reintroduce some logic from #237857 , but adds some additional logic from the Lens side:
ReactExpressionRenderer(lower level than the Lens Embeddable) is notified when an error should be ignored (via the onRenderError callback) and avoids to show a blank panelReactExpressionRenderercomponent due to unstable Lens parameters, so I've stabilised them.How to test
The changes affects aborted and regular queries, so it's best to check both fast and slow queries.
To test this feature open Discover and create a classic histogram then
breakdownoptions quickly. (fast queries check){ "error_query": { "indices": [ { "error_type": "warning", "message": "'Watch out!'", "name": "*", "stall_time_seconds": 5 } ] } }