-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][ES|QL] Performance troubles when there are lots of fields #199608
Comments
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
Pinging @elastic/kibana-esql (Team:ESQL) |
Pinging @elastic/kibana-visualizations (Team:Visualizations) |
Worth noting that it's not a usual histogram case. It's when Discover passes already fetched data to Lens via This is why it's slow only on Discover page (where Lens gets data from props) and fast on Dashboard page (where Lens fetches data directly). |
The problem is in the histogram implementation. The logic changed from my initial implementation. What is happening here is that you run the suggestions api twice:
In case of histogram the first should not run at all, is useless (is not performant to run an api when you are not going to use it). What is happening is:
so now it goes to this if https://github.com/elastic/kibana/blob/main/src/plugins/unified_histogram/public/services/lens_vis_service.ts#L316 and adds the bad suggestion (which is a DSL one) You should change the logic. The histogram mode should be calculated first. In case it returns nothing, then we dont show the histogram In case of stats mode then we ask for all suggestions. The suggestions api SHOULD not run for DSL mode |
@stratoula What do you mean by DSL mode? |
The dataview mode, in the dataview mode we should not request for suggestions. We have the state hardcoded and we pass it to Lens embeddable |
My suggestion is
We should not mix the transformational commands mode with the non-transformational one otherwise we are creating bugs like this one. I recently fixed another one related #195863 |
@stratoula Thanks for expanding on it!
The Lens suggestion API is not called for the data view mode What makes you think that it creates the reported issue?
Yes, in this case the current implementation would call the suggestions API twice (without histogram-related params and then with them). Should we now make only the call for suggestions with histogram-related params? What if it returns nothing? Sorry, I don't understand " If the api returns nothing there is a good reason for this, do not show nothing".
That should be already the case as |
Yes this was the initial's implementation logic. If there are transformational commands then all good. You run the suggestions api, I think this works ok. The problem is on the histogram mode. Histogram mode === Non transformational commands (no stats, no keep ...). In that case and if there is a date field it should check for https://github.com/elastic/kibana/blob/main/src/plugins/unified_histogram/public/services/lens_vis_service.ts#L485 I assume that one of this checks do not pass so it returns undefined. (is it timebased for example? It doesnt seem so) As it should, it will not return a histogram suggestion. No histogram suggestion means no chart. You should not return (nor run) the suggestions api and get other suggestions. This was never a requirement, I consider it a bug. I hope it is clear now.
I don't say it creates the reported issue. The reported issue is being created by the problem I explain above (run the suggestions for the histogram twice). I just want to highlight that in case of DSL we should not run the suggestions api. If this is the case already then all good. 👍 But still the suggestion that it suggests is bad as it behaves as it is on the transformational mode while it should not (in that case and only we send the table to the Lens embeddable). |
While I agree that we could have 1 lens suggestion API call for non-transformational ES|QL query and time-based data instead of current 2 calls, it's not the root issue for the reported case. In fact, this data set is not time-based, so UnifiedHistogram will call Lens suggestion API only once. The API call itself answers very fast with the following 3 suggestions. Then UnifiedHistogram picks the first suggestion and passes it to Lens for rendering. And the rendering of this suggestion (which was enriched with
|
@jughosta we dont want to suggest anything different in case is not time based for non transformational commands mode. The logic is can I render a histogram? No? Then I dont want to render a chart
This is the problem exactly, we don't want to suggest anything else here. This was always the requirement as I describe above. |
Ad to explain further. The table props was introduced by Peter for Discover transformational commands mode. It should not be passed for non transformational commands. We did this because on stats mode we dont want to run the api twice. It was never designed to work for histogram mode. (if anyone wants to pass the table in Lens should ensure that only the columns / rows that take part in the viz are passed, something not happening here) The lens api works exactly as designed. |
So I guess in this case, not rendering a chart, it should be caught higher up in the rendering tree, e.g. here we don't check for the case we are discussing in the function:
Modified it quickly in my exploration PR to take this into consideration export function checkChartAvailability({
chart,
dataView,
isPlainRecord,
query,
}: {
chart?: UnifiedHistogramChartContext;
dataView: DataView;
isPlainRecord?: boolean;
query: Query | AggregateQuery;
}): boolean {
if (query && isOfAggregateQueryType(query)) {
const isOnHistogramMode = shouldDisplayHistogram(query);
if (!isOnHistogramMode || !dataView.isTimeBased()) {
return false;
}
}
return Boolean(
chart &&
dataView.id &&
dataView.type !== DataViewType.ROLLUP &&
(isPlainRecord || (!isPlainRecord && dataView.isTimeBased()))
);
} Performance issue is gone, however the result is a bit ... white ... but I bet this is fixable |
@stratoula Got it, thanks! Going to work on a new implementation for the |
Thanx Jul! Let me know if I can help anywhere |
…and non-time-based ES|QL queries (elastic#200583) - Closes elastic#199608 ## Summary This PR changes the logic around when suggestions from lens API are used. Previously for non-transformational query and non-time-based data it would try to render one of lens suggestions supplying chart data via `table` prop. Now, it would not render any chart. Before: - Data view mode => Static histogram configuration - ES|QL mode and non-transformational query => _**Gets lens suggestions.**_ If histogram chart is not possible, **_takes the first lens suggestion for rendering the chart_** - ES|QL mode and transformational query => Gets lens suggestions. Takes the first lens suggestion for rendering the chart. After: - Data view mode => Static histogram configuration (same) - ES|QL mode and non-transformational query => ~~_**Gets lens suggestions.**_~~ If histogram chart is not possible, **_renders nothing_** (updated) - ES|QL mode and transformational query => Gets lens suggestions. Takes the first lens suggestion for rendering the chart. (same) ### Testing As per originally reported case: 1. `node scripts/es_archiver --kibana-url=http://elastic:changeme@localhost:5601 --es-url=http://elastic:changeme@localhost:9200 load test/functional/fixtures/es_archiver/many_fields` 2. Navigate to Discover, switch to ES|QL mode and enter `FROM indices-stats | LIMIT 10` 3. No chart is expected. Also there should be no regression for elastic#195863 ### Checklist - [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 --------- Co-authored-by: Davis McPhee <davis.mcphee@elastic.co> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> (cherry picked from commit b9439e6)
…and non-time-based ES|QL queries (elastic#200583) - Closes elastic#199608 ## Summary This PR changes the logic around when suggestions from lens API are used. Previously for non-transformational query and non-time-based data it would try to render one of lens suggestions supplying chart data via `table` prop. Now, it would not render any chart. Before: - Data view mode => Static histogram configuration - ES|QL mode and non-transformational query => _**Gets lens suggestions.**_ If histogram chart is not possible, **_takes the first lens suggestion for rendering the chart_** - ES|QL mode and transformational query => Gets lens suggestions. Takes the first lens suggestion for rendering the chart. After: - Data view mode => Static histogram configuration (same) - ES|QL mode and non-transformational query => ~~_**Gets lens suggestions.**_~~ If histogram chart is not possible, **_renders nothing_** (updated) - ES|QL mode and transformational query => Gets lens suggestions. Takes the first lens suggestion for rendering the chart. (same) ### Testing As per originally reported case: 1. `node scripts/es_archiver --kibana-url=http://elastic:changeme@localhost:5601 --es-url=http://elastic:changeme@localhost:9200 load test/functional/fixtures/es_archiver/many_fields` 2. Navigate to Discover, switch to ES|QL mode and enter `FROM indices-stats | LIMIT 10` 3. No chart is expected. Also there should be no regression for elastic#195863 ### Checklist - [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 --------- Co-authored-by: Davis McPhee <davis.mcphee@elastic.co> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> (cherry picked from commit b9439e6)
…and non-time-based ES|QL queries (elastic#200583) - Closes elastic#199608 ## Summary This PR changes the logic around when suggestions from lens API are used. Previously for non-transformational query and non-time-based data it would try to render one of lens suggestions supplying chart data via `table` prop. Now, it would not render any chart. Before: - Data view mode => Static histogram configuration - ES|QL mode and non-transformational query => _**Gets lens suggestions.**_ If histogram chart is not possible, **_takes the first lens suggestion for rendering the chart_** - ES|QL mode and transformational query => Gets lens suggestions. Takes the first lens suggestion for rendering the chart. After: - Data view mode => Static histogram configuration (same) - ES|QL mode and non-transformational query => ~~_**Gets lens suggestions.**_~~ If histogram chart is not possible, **_renders nothing_** (updated) - ES|QL mode and transformational query => Gets lens suggestions. Takes the first lens suggestion for rendering the chart. (same) ### Testing As per originally reported case: 1. `node scripts/es_archiver --kibana-url=http://elastic:changeme@localhost:5601 --es-url=http://elastic:changeme@localhost:9200 load test/functional/fixtures/es_archiver/many_fields` 2. Navigate to Discover, switch to ES|QL mode and enter `FROM indices-stats | LIMIT 10` 3. No chart is expected. Also there should be no regression for elastic#195863 ### Checklist - [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 --------- Co-authored-by: Davis McPhee <davis.mcphee@elastic.co> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
…and non-time-based ES|QL queries (elastic#200583) - Closes elastic#199608 ## Summary This PR changes the logic around when suggestions from lens API are used. Previously for non-transformational query and non-time-based data it would try to render one of lens suggestions supplying chart data via `table` prop. Now, it would not render any chart. Before: - Data view mode => Static histogram configuration - ES|QL mode and non-transformational query => _**Gets lens suggestions.**_ If histogram chart is not possible, **_takes the first lens suggestion for rendering the chart_** - ES|QL mode and transformational query => Gets lens suggestions. Takes the first lens suggestion for rendering the chart. After: - Data view mode => Static histogram configuration (same) - ES|QL mode and non-transformational query => ~~_**Gets lens suggestions.**_~~ If histogram chart is not possible, **_renders nothing_** (updated) - ES|QL mode and transformational query => Gets lens suggestions. Takes the first lens suggestion for rendering the chart. (same) ### Testing As per originally reported case: 1. `node scripts/es_archiver --kibana-url=http://elastic:changeme@localhost:5601 --es-url=http://elastic:changeme@localhost:9200 load test/functional/fixtures/es_archiver/many_fields` 2. Navigate to Discover, switch to ES|QL mode and enter `FROM indices-stats | LIMIT 10` 3. No chart is expected. Also there should be no regression for elastic#195863 ### Checklist - [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 --------- Co-authored-by: Davis McPhee <davis.mcphee@elastic.co> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
This appears to be a UI performance problem in the Lens Embeddable, surfacing when using ES|QL in Discover. When adding the visualization on a Dashboard, it's fast and snappy. But when using a many fields index pattern in Discover with ES|QL, I've waited about a minute to have the histogram rendered, and the browser was blocked then.
How to reproduce, first you need to add the
many_fields
functional test data to Elasticsearchnode scripts/es_archiver --kibana-url=http://elastic:changeme@localhost:5601 --es-url=http://elastic:changeme@localhost:9200 load test/functional/fixtures/es_archiver/many_fields
Navigate to Discover, switch to ES|QL mode and enter
FROM indices-stats | LIMIT 10
In my local instance, while documents returned fast, the histogram was stuck in loading state, for >60s. It was not waiting for any request to be returned, there were some calculations goint on :
In the chrome performance problem it looked like this
The browser is busy with Javascript calculations, sady, it's not clear what the browser is doing
Trying the performance tools in Firefox, gave me a better hint:
So it seems to be about
src/plugins/chart_expressions/expression_xy/public/helpers/data_layers.tsx
, which contains many iteration on all columns a.k.a files. in themany_fields
data we hava about 10k fields. This is definitely an edge case, but also a smaller amount of fields should benefit from less iterations in this part of the code. I've addedconsole.count
for iteration counting, and when there are 10k fields, it appears that there are > 140k iterations.Here's a draft PR that can be used for iterations:
#199589
The text was updated successfully, but these errors were encountered: