-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Main] Split single query into 2 queries for faster results #104818
[Discover][Main] Split single query into 2 queries for faster results #104818
Conversation
…com:kertal/kibana into kertal-pr-2021-06-14-discover-improve-state
…-16-discover-split-queries
…-16-discover-split-queries
…-16-discover-split-queries
…-16-discover-split-queries
…b.com:kertal/kibana into kertal-pr-2020-06-08-discover-split-queries-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sass changes LGTM.
@elasticmachine merge upstream |
const [from, to] = x; | ||
this.props.timefilterUpdateHandler({ from, to }); | ||
const formatXValue = (val: string) => { | ||
const xAxisFormat = chartData!.xAxisFormat.params!.pattern; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const xAxisFormat = chartData!.xAxisFormat.params!.pattern; | |
const xAxisFormat = chartData.xAxisFormat.params!.pattern; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the pre version change, all the queries seem to work as expected. Reviewed code, LGTM
@elasticmachine merge upstream (github failure on doc build) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code review src/plugins/data/common/search/search_source/mocks.ts
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @kertal |
…elastic#104818) Co-authored-by: Tim Roes <tim.roes@elastic.co>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…elastic#104818) Co-authored-by: Tim Roes <tim.roes@elastic.co>
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
Summary
This PR splits the single query for documents in Discover main view into 2 queries sent to ES.
The purpose of this PR is to present results to the user as quick as possible, if e.g. the query for the chart takes a bit longer the user should already be able to view the documents and vice versa.
There are now 4 instead of 1 BehaviorSubjects the UI subscribes to get messages about the state of data fetching
Changes in UI
When data fetching is started and no data was fetched previously, there are no changes, it looks like this:
When the first of the 2 requests is returned, and it contains data, the spinners of the remaining request are displayed (in the screen all requests are still loading, but it's just mocked):
When the first of the 2 request is returned, and it contains no data/documents, the following screen is displayed initially, without waiting for the other requests to be finished
Requests sent to ES
Documents
Total hits
Total Hits and Chart
More changes in this PR:
Out of scope of this PR:
When displaying the cart was toggled to a state when the chart is displayed, we currently request all the data, we could now switch to just request the chart data, which would be a nice follow up PR. (Same when changing the chart interval in UI)
Resolves #69134
Part of #101041
Also fixes parts of #69049
Fixes #37519 (since we have an aria-live on the result count now)
Checklist