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.

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

[Discover] Remove redundant data fetching when hiding/showing the histogram/chart #206389

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

kertal
Copy link
Member

@kertal kertal commented Jan 13, 2025

Summary

With #204694 being merged, fixing the issue that the requests for results of the data table and the chart can have different time ranges, we now no longer need to fetch all data data when the chart visibility is toggled in Discover. We can now fetch it when needed, the Lens embeddable automatically takes care of it in this case. This simplifies the logic and removes and resolves another issue of additional redundant data fetching when using ES|QL in Discover.

This is how it's supposed to work. When showing a histogram that's hidden, the timerange of the recent request should be used to get the data, also when hiding the histogram, no data should be requested. With the change in #204694 this is possible and it's implemented this way now

Checklist

@kertal kertal self-assigned this Jan 13, 2025
@kertal
Copy link
Member Author

kertal commented Jan 13, 2025

/ci

@kertal
Copy link
Member Author

kertal commented Jan 13, 2025

/ci

1 similar comment
@kertal
Copy link
Member Author

kertal commented Jan 13, 2025

/ci

@kertal kertal changed the title [Discover] don't fetch data when hiding/showing chart if not needed [Discover] Remove redundant data fetching when hiding/showing the histogram/chart Jan 14, 2025
@kertal kertal added Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. release_note:fix backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Jan 14, 2025
@kertal
Copy link
Member Author

kertal commented Jan 14, 2025

/ci

@kertal
Copy link
Member Author

kertal commented Jan 14, 2025

/ci

1 similar comment
@kertal
Copy link
Member Author

kertal commented Jan 15, 2025

/ci

@kertal
Copy link
Member Author

kertal commented Jan 15, 2025

/ci

@elasticmachine
Copy link
Contributor

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

@kertal
Copy link
Member Author

kertal commented Jan 15, 2025

/ci

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

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

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

see run history

@kertal
Copy link
Member Author

kertal commented Jan 16, 2025

/ci

Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kertal kertal enabled auto-merge (squash) January 17, 2025 09:22
@kertal kertal merged commit a042747 into elastic:main Jan 17, 2025
14 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12827602801

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #38 / console app console app tabs navigation tabs should be navigable through URL
  • [job] [logs] FTR Configs #16 / Search solution tests Search index details page Solution Nav - Search search index details page API key details should show api key

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 844.0KB -90.0B

History

cc @kertal

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 17, 2025
…togram/chart (elastic#206389)

Since the timerange in Discover of the main request is stable we don't need to trigger a main fetch for all data when the histogram/chart is being hidden/displayed, unless it's necessary to get the data (e.g. when the histogram/chart was hiden when a discover session was being loaded)

(cherry picked from commit a042747)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jan 17, 2025
…he histogram/chart (#206389) (#207051)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Discover] Remove redundant data fetching when hiding/showing the
histogram/chart
(#206389)](#206389)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Matthias
Wilhelm","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-17T10:49:37Z","message":"[Discover]
Remove redundant data fetching when hiding/showing the histogram/chart
(#206389)\n\nSince the timerange in Discover of the main request is
stable we don't need to trigger a main fetch for all data when the
histogram/chart is being hidden/displayed, unless it's necessary to get
the data (e.g. when the histogram/chart was hiden when a discover
session was being
loaded)","sha":"a04274723ef64182df96d37d134cc645e20571ee","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","Feature:Discover","performance","v9.0.0","Team:DataDiscovery","backport:prev-minor"],"title":"[Discover]
Remove redundant data fetching when hiding/showing the
histogram/chart","number":206389,"url":"https://github.com/elastic/kibana/pull/206389","mergeCommit":{"message":"[Discover]
Remove redundant data fetching when hiding/showing the histogram/chart
(#206389)\n\nSince the timerange in Discover of the main request is
stable we don't need to trigger a main fetch for all data when the
histogram/chart is being hidden/displayed, unless it's necessary to get
the data (e.g. when the histogram/chart was hiden when a discover
session was being
loaded)","sha":"a04274723ef64182df96d37d134cc645e20571ee"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/206389","number":206389,"mergeCommit":{"message":"[Discover]
Remove redundant data fetching when hiding/showing the histogram/chart
(#206389)\n\nSince the timerange in Discover of the main request is
stable we don't need to trigger a main fetch for all data when the
histogram/chart is being hidden/displayed, unless it's necessary to get
the data (e.g. when the histogram/chart was hiden when a discover
session was being
loaded)","sha":"a04274723ef64182df96d37d134cc645e20571ee"}}]}]
BACKPORT-->

Co-authored-by: Matthias Wilhelm <[email protected]>
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Jan 17, 2025
…togram/chart (elastic#206389)

Since the timerange in Discover of the main request is stable we don't need to trigger a main fetch for all data when the histogram/chart is being hidden/displayed, unless it's necessary to get the data (e.g. when the histogram/chart was hiden when a discover session was being loaded)
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Jan 21, 2025
…togram/chart (elastic#206389)

Since the timerange in Discover of the main request is stable we don't need to trigger a main fetch for all data when the histogram/chart is being hidden/displayed, unless it's necessary to get the data (e.g. when the histogram/chart was hiden when a discover session was being loaded)
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
…togram/chart (elastic#206389)

Since the timerange in Discover of the main request is stable we don't need to trigger a main fetch for all data when the histogram/chart is being hidden/displayed, unless it's necessary to get the data (e.g. when the histogram/chart was hiden when a discover session was being loaded)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Discover Discover Application performance release_note:enhancement Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants