[Discover] [ES|QL] Prevent redundant requests when loading Discover sessions and toggling chart visibility #206699

davismcphee · 2025-01-15T05:18:22Z

Summary

This PR prevents redundant Discover requests in ES|QL mode for the following scenarios:

Creating a new Discover session.
Saving the current Discover session.
Loading a saved Discover session.
Toggling the Unified Histogram chart visibility.

It does so by addressing several underlying state related issues that were triggering the redundant requests:

Skipping the initial emission of currentSuggestionContext on Unified Histogram mount, which immediately triggered a second fetch.
Treating the Unified Histogram table prop the same as other props which affect Lens suggestions (data view, query, columns), and deferring updates to it until result fetching completes to avoid unnecessary suggestion updates.
Removing all auto-fetching behaviour from Unified Histogram and instead relying solely on the consumer to control when fetching should occur (including the initial fetch).

Part of #165192.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests 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
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 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

… mount

…rentSuggestionContext updates and refetches

…unt, reducing redundant requests

kertal

Niiiiiiiice ❤️ !

kertal · 2025-01-15T07:33:44Z

.../plugins/shared/discover/public/application/main/components/layout/use_discover_histogram.ts

@@ -267,18 +268,6 @@ export const useDiscoverHistogram = ({
   * Data fetching
   */

-  const skipRefetch = useRef<boolean>();


it's great that we can remove this logic here! 🥳

I agree! I've always disliked this workaround and have been wanting to remove it for a while, and now I finally had a good reason 😁

kertal · 2025-01-15T07:38:20Z

test/functional/apps/discover/group3/_request_counts.ts

@@ -289,7 +291,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
        await expectSearches(type, 1, async () => {
          await discover.toggleChartVisibility();
        });
-        await expectSearches(type, 3, async () => {
+        await expectSearches(type, 2, async () => {


ha! 🎼 another requests bites the dust !
and in case this turns green: #206389
3 more requests will bite the dust

Yes, I wasn't even aiming to address this one originally but it was a pleasant bonus for sure! I don't think it fully resolves #206389 yet since we should still skip the documents fetch on toggle, but it's definitely related and gets us closer to that goal 👍

I just realized #206389 is a PR and not an issue 🤦 In that case, awesome! These enhancements combined should resolve all chart visibility related fetching issues 🤘

kibanamachine · 2025-01-15T08:53:33Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7723

[✅] test/functional/apps/discover/group3/config.ts: 25/25 tests passed.

see run history

davismcphee

@kertal Thanks for looking at it! It seems like both CI and the flaky test runner are happy with these changes, so I'm going to publish this for review now 🙂

davismcphee · 2025-01-15T15:17:30Z

.../plugins/shared/discover/public/application/main/components/layout/use_discover_histogram.ts

@@ -267,18 +268,6 @@ export const useDiscoverHistogram = ({
   * Data fetching
   */

-  const skipRefetch = useRef<boolean>();


I agree! I've always disliked this workaround and have been wanting to remove it for a while, and now I finally had a good reason 😁

davismcphee · 2025-01-15T15:20:13Z

test/functional/apps/discover/group3/_request_counts.ts

@@ -289,7 +291,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
        await expectSearches(type, 1, async () => {
          await discover.toggleChartVisibility();
        });
-        await expectSearches(type, 3, async () => {
+        await expectSearches(type, 2, async () => {


Yes, I wasn't even aiming to address this one originally but it was a pleasant bonus for sure! I don't think it fully resolves #206389 yet since we should still skip the documents fetch on toggle, but it's definitely related and gets us closer to that goal 👍

elasticmachine · 2025-01-15T15:21:28Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

elasticmachine · 2025-01-17T05:50:12Z

💚 Build Succeeded

Buildkite Build
Commit: a6a6c63

Metrics [docs]

Async chunks

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

id	before	after	diff
`discover`	844.1KB	843.9KB	-159.0B
`unifiedHistogram`	72.1KB	71.1KB	-1008.0B
total			-1.1KB

History

💚 Build #267221 succeeded a0d9b5b

cc @davismcphee

kertal

Code LGTM, great simplification of the code leading to omitting all redundant requests that were mentioned in #206699

So this PR is a release_note:fix 🥳

I've tested the reported extra fetches with #206699 using
https://kertal-pr-206699-fix-esql-fetches.kbndev.co/app/r/s/WIE7x

we now have a new request for fields triggered by the ES|QL editor, so the numbers changed

✅ When loading a ES|QL Discover session: 3 fetches
✅ Hiding the chart: 1 fetch, will get to 0 with [Discover] Remove redundant data fetching when hiding/showing the histogram/chart #206389
✅ Reload with chart: 2 fetches, expected
✅ Changing from Discover session with hidden chart, to one with Chart, 3 fetches, expected
✅ Navigating back from Dashboard, 3 fetches, expected
✅ Saving / opening a Discover session, 3 fetches, expected

davismcphee added 3 commits January 15, 2025 00:22

Skip initial currentSuggestionContext emission to avoid refetching on…

eab1c4c

… mount

Move table to getUnifiedHistogramPropsForEsql to avoid additional cur…

b01b79c

…rentSuggestionContext updates and refetches

Remove Unified Histogram auto fetching, and avoid auto fetching on mo…

a0d9b5b

…unt, reducing redundant requests

davismcphee self-assigned this Jan 15, 2025

davismcphee mentioned this pull request Jan 15, 2025

[Discover][ES|QL] Omit redundant requests when loading Discover sessions #204418

Draft

9 tasks

kertal reviewed Jan 15, 2025

View reviewed changes

davismcphee commented Jan 15, 2025

View reviewed changes

davismcphee marked this pull request as ready for review January 15, 2025 15:21

davismcphee requested a review from a team as a code owner January 15, 2025 15:21

Merge branch 'main' into fix-esql-fetches

a6a6c63

kertal mentioned this pull request Jan 17, 2025

[Unified histogram][ES|QL] Extra fetches in specific scenarios #165192

Open

kertal approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] [ES|QL] Prevent redundant requests when loading Discover sessions and toggling chart visibility #206699

[Discover] [ES|QL] Prevent redundant requests when loading Discover sessions and toggling chart visibility #206699

davismcphee commented Jan 15, 2025 •

edited

Loading

kertal left a comment

kertal Jan 15, 2025

davismcphee Jan 15, 2025

kertal Jan 15, 2025

davismcphee Jan 15, 2025

davismcphee Jan 15, 2025

kibanamachine commented Jan 15, 2025

davismcphee left a comment

davismcphee Jan 15, 2025

davismcphee Jan 15, 2025

elasticmachine commented Jan 15, 2025

elasticmachine commented Jan 17, 2025

kertal left a comment

[Discover] [ES|QL] Prevent redundant requests when loading Discover sessions and toggling chart visibility #206699

Are you sure you want to change the base?

[Discover] [ES|QL] Prevent redundant requests when loading Discover sessions and toggling chart visibility #206699

Conversation

davismcphee commented Jan 15, 2025 • edited Loading

Summary

Checklist

kertal left a comment

Choose a reason for hiding this comment

kertal Jan 15, 2025

Choose a reason for hiding this comment

davismcphee Jan 15, 2025

Choose a reason for hiding this comment

kertal Jan 15, 2025

Choose a reason for hiding this comment

davismcphee Jan 15, 2025

Choose a reason for hiding this comment

davismcphee Jan 15, 2025

Choose a reason for hiding this comment

kibanamachine commented Jan 15, 2025

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7723

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Jan 15, 2025

Choose a reason for hiding this comment

davismcphee Jan 15, 2025

Choose a reason for hiding this comment

elasticmachine commented Jan 15, 2025

elasticmachine commented Jan 17, 2025

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kertal left a comment

Choose a reason for hiding this comment

davismcphee commented Jan 15, 2025 •

edited

Loading