[Discover] [Unified Data Table] Improve Discover / Unified Data Table performance in ES|QL mode #191249

davismcphee · 2024-08-26T00:26:33Z

Summary

This PR includes several performance improvements for ES|QL mode in Discover:

Memoize UnifiedDataTableRenderCellValue to reduce re-renders.
Show the loading spinner immediately on state changes that should trigger a refetch so that changed state isn't applied to the grid before the fetch completes. This also helps to reduce re-renders on state changes.
Replace EuiDataGrid in-memory sorting with a custom implementation using the @kbn/sort-predicates package (the same one Lens uses for its data grid). EUI in-memory sorting has a drastic impact on performance because it renders the entire grid off screen in order to parse cell values, detect their schema, and then sort the rows. This can cause rendering delays of several seconds for larger datasets on each render of the data grid. The new approach will sort in memory based on actual field values and pass the sorted rows to the data grid, which is both much more performant as well as improving the sorting behaviour across field types.
Use the overscanRowCount option from react-window to render some additional rows outside of view in the data grid, which minimizes pop-in of rows while scrolling quickly.

There are also a couple of bug fixes included in the PR as a result of the new sorting behaviour:

Numeric columns are now sorted correctly where previously they used alphabetic sorting.
The "Copy column" action in data grid columns now correctly copies the column values in the current sort order instead of copying them unsorted as it used to.

Before - 66,904 UnifiedDataTableRenderCellValue render calls:

render_old.mp4

After - 424 UnifiedDataTableRenderCellValue render calls:

render_new.mp4

Before profile:

After profile:

Before scrolling:

pop_in_old.mp4

After scrolling:

pop_in_new.mp4

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
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

davismcphee · 2024-08-26T00:29:26Z

/ci

davismcphee · 2024-08-26T00:51:34Z

/ci

davismcphee · 2024-08-27T03:20:23Z

/ci

davismcphee · 2024-08-28T01:01:54Z

/ci

davismcphee · 2024-08-28T03:13:15Z

/ci

elasticmachine · 2024-08-28T04:05:27Z

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

elasticmachine · 2024-08-28T04:05:28Z

Pinging @elastic/kibana-esql (Team:ESQL)

davismcphee · 2024-08-28T04:05:51Z

/ci

kertal · 2024-08-28T07:59:19Z

EUI in-memory sorting has a drastic impact on performance because it renders the entire grid off screen in order to parse cell values, detect their schema, and then sort the rows.

This looks impressive! one note a bit unrelated to this PR, but this perfomance impact is also what we need to consider if we implement in-table search based on rendered values and not on raw values

davismcphee · 2024-08-28T17:56:47Z

one note a bit unrelated to this PR, but this perfomance impact is also what we need to consider if we implement in-table search based on rendered values and not on raw values

True, we'll definitely have to consider performance impact for this, but I'm hopeful we'll be able to do it more efficiently than the data grid in-memory sorting. We may have to render certain cells in memory like React based ones, but hopefully we can deal with string renderers for most, and use caching, etc. to help speed things up.

davismcphee · 2024-08-29T03:38:16Z

...y_solution/public/timelines/components/timeline/unified_components/data_table/index.test.tsx

+  // SKIP: This test runs forever and never completes
+  // after https://github.com/elastic/kibana/pull/191249
+  // but the actual functionality works fine
+  it.skip(


@elastic/security-threat-hunting-investigations (maybe @logeekal or @michaelolo24 specifically?) For some reason after these changes, this test consistently fails and hangs until CI times out (locally it just ran forever until I killed the process). It seems to have something to do with the fireEvent.change call below, but I couldn't pinpoint what was happening. As far as I can tell the actual functionality isn't impacted and Timeline works as expected, plus it's the only Jest test failing, so maybe you could confirm things work as expected and this could be investigated as a followup if all seems good?

Thanks @davismcphee , I will pull down the branch and check what is the issue. If something needed to be fixed in the test, I will push that..

Just to add-on, I think it might EUI data grid test mode creating issue with memoized components. I did try to memoize render cell value before when we were working on improving the performance of unified data table but I had to remove it becuase Euidatagrid was not playing nice with it in jest as you can see here.

But I will check again to confirm that it is the same issue.

I looked into this more and found the warnings were related to incorrect prop type validation generated by EUI for Jest testing components, specifically from this file. I addressed it in this commit by skipping memoization for Jest environments, which gets rid of the warnings: b6ceeb1.

But unfortunately it looks like this wasn't the source of the original issue and this test still runs forever, so I'm really not sure what could be going on here. I even resolved all of the warnings in our Unified Data Table tests to be sure I wasn't missing something, but they're warning free now with no similar issues occurring.

Thanks @davismcphee for looking into it and adding JEST clause. I should have done that initially but it did not occur to me.

I have fixed the test here: d93e90d.

Issue was huge amount of data in jsdom. Since changing rowHeight re-renders the complete table, it was causing jsdom to take huge amount of time to re-render and thus firevent.change never reports that the value has been changed.

I changed the passed data to just one row and test passed.

Regarding NaN warning, I will check it separately, it comes everytime the data grid renders, probably will need to mock some EuiDataGrid height calculation function.

Thanks for looking into that and fixing the test @logeekal!

stratoula · 2024-08-29T06:04:54Z

I wonder what change raises the async bundle 100+KB 🤔

Update: I played with it and indeed feels lighter 🎉 I just wonder if we can do something for the async bundle size bump

kertal

You're a smooth (UI) operator, Davis, great to focus on performance!
Sorting in my testing was snappier

Loading a result with many fields in the table, I wouldn't call this snappier, it was like riding a VW beetle (no offense!) vs a Koenigsegg Jesko Absolut (didn't know, just googled fastest car in the world

), a few seconds vs a minute

Nice bugfixes under the hood also, I agree with @stratoula that reducing the bundle size would be the icing on the cake, but given this improvements I'd take the cake, since the cake wasn't added / eaten when loading Kibana.

When in doubt that memoization causes problems with tests that can't be fixed, I'd suggest to extract this to another PR, so it would stay a beetle for a little longer. better beetle than a bug.

stratoula · 2024-08-30T10:28:08Z

Nice bugfixes under the hood also, I agree with @stratoula that reducing the bundle size would be the icing on the cake, but given this improvements I'd take the cake, since the cake wasn't added / eaten when loading Kibana.

Let's understand what causes the bump first though, is a significant bump. I would love to understand what change caused it

davismcphee · 2024-08-31T01:03:39Z

/ci

davismcphee · 2024-08-31T03:05:53Z

@stratoula The increase is 100kb overall because packages are imported separately to each plugin, but the individual increase to Unified Data Table was about ~20kb. I looked into it and a majority of the increase is from node_modules due to imports in @kbn/sort-predicates for semver and IP address sorting. I tried optimizing the imports but it didn't have an impact, I guess it just takes a lot of code to sort these things 😅

Main:

This PR:

stratoula · 2024-09-02T06:35:55Z

Thanx for looking into this Davis! This works for me, just wanted to be sure that this increase is reasonable

logeekal

Thanks for making these changes. 👌

davismcphee · 2024-09-03T04:30:51Z

test/functional/apps/discover/esql/_esql_view.ts

+          const state = kbnRison.encode({
+            dataSource: { type: 'esql' },
+            query: { esql: 'from kibana_sample_data_flights' },
+          });
+          await PageObjects.common.navigateToActualUrl('discover', `?_a=${state}`, {
+            ensureCurrentUrl: false,
+          });


@lukasolson I'm not sure exactly why this test started failing, but as far as I can tell it shouldn't have passed originally since from kibana_sample_data_flights doesn't show the chart and should only trigger 1 request for Table. I suspect we have some race condition (which we should investigate further separately) and the performance improvements cause this scenario to behave as expected, but WDYT?

Yeah, you must be right. Here's a recording showing that it was broken before:

Screen.Recording.2024-09-03.at.3.03.57.PM.mov

With this fix, it looks like things are working properly.

kibana-ci · 2024-09-03T05:52:37Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cloudSecurityPosture`	636	640	+4
`discover`	957	970	+13
`esqlDataGrid`	345	358	+13
`lens`	1438	1439	+1
`logsExplorer`	546	559	+13
`securitySolution`	5654	5658	+4
`slo`	827	840	+13
total			+61

Async chunks

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

id	before	after	diff
`cloudSecurityPosture`	480.7KB	493.6KB	+12.9KB
`discover`	769.8KB	792.4KB	+22.6KB
`esqlDataGrid`	129.5KB	152.1KB	+22.6KB
`securitySolution`	17.9MB	18.0MB	+61.6KB
`slo`	829.0KB	851.7KB	+22.6KB
total			+142.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`esqlDataGrid`	9.3KB	9.3KB	+50.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @davismcphee

lukasolson · 2024-09-03T22:15:26Z

test/functional/apps/discover/esql/_esql_view.ts

+          const state = kbnRison.encode({
+            dataSource: { type: 'esql' },
+            query: { esql: 'from kibana_sample_data_flights' },
+          });
+          await PageObjects.common.navigateToActualUrl('discover', `?_a=${state}`, {
+            ensureCurrentUrl: false,
+          });


Yeah, you must be right. Here's a recording showing that it was broken before:

Screen.Recording.2024-09-03.at.3.03.57.PM.mov

With this fix, it looks like things are working properly.

…when Document column isn't visible (#192332) ## Summary Part of #191249 included using the `overscanRowCount` property of EUI data grid to render some additional data grid rows off screen, which greatly reduces pop in when scrolling through the grid. While the performance cost of this low in most cases, it has an impact in certain situations such as when documents have many fields and the Document column is visible (e.g. Discover's `many_fields` performance journey). This is because the Document column can render _many_ DOM elements in each of its cells, which EUI data grid struggles to handle. This PR updates Unified Data Table to only use `overscanRowCount` when the Document column isn't visible, so we'll still benefit from the scrolling improvements when columns are selected, but won't take the performance hit for the Document column. Perf journey run: https://buildkite.com/elastic/kibana-single-user-performance/builds/14355. <img width="842" alt="Screenshot 2024-09-09 at 14 07 46" src="https://github.com/user-attachments/assets/b074fd7a-ef8a-4917-ac5e-0675b0e60b7d"> ### Checklist - [ ] 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/packages/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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] 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 renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

…when Document column isn't visible (elastic#192332) ## Summary Part of elastic#191249 included using the `overscanRowCount` property of EUI data grid to render some additional data grid rows off screen, which greatly reduces pop in when scrolling through the grid. While the performance cost of this low in most cases, it has an impact in certain situations such as when documents have many fields and the Document column is visible (e.g. Discover's `many_fields` performance journey). This is because the Document column can render _many_ DOM elements in each of its cells, which EUI data grid struggles to handle. This PR updates Unified Data Table to only use `overscanRowCount` when the Document column isn't visible, so we'll still benefit from the scrolling improvements when columns are selected, but won't take the performance hit for the Document column. Perf journey run: https://buildkite.com/elastic/kibana-single-user-performance/builds/14355. <img width="842" alt="Screenshot 2024-09-09 at 14 07 46" src="https://github.com/user-attachments/assets/b074fd7a-ef8a-4917-ac5e-0675b0e60b7d"> ### Checklist - [ ] 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/packages/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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] 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 renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

davismcphee self-assigned this Aug 26, 2024

davismcphee force-pushed the unified-data-table-performance branch from 7275c69 to 0942114 Compare August 27, 2024 03:20

davismcphee force-pushed the unified-data-table-performance branch from 30c105f to eb58d4d Compare August 28, 2024 03:13

davismcphee marked this pull request as ready for review August 28, 2024 04:05

davismcphee requested review from a team as code owners August 28, 2024 04:05

kertal requested a review from a team August 28, 2024 07:56

davismcphee and others added 4 commits August 28, 2024 14:57

Improve Discover / Unified Data Table performance in ES|QL mode

e41f6be

[CI] Auto-commit changed files from 'node scripts/notice'

543505c

Fix tests

7ff2245

Cleanup and adding comments

cae0eb6

davismcphee force-pushed the unified-data-table-performance branch from eb58d4d to cae0eb6 Compare August 28, 2024 17:57

Skip failing test

c14b12c

davismcphee commented Aug 29, 2024

View reviewed changes

Fix warnings in tests

b6ceeb1

logeekal added 2 commits August 30, 2024 11:17

fix: hung test because of huge data in jsdom

d93e90d

chore: remove unnecessary comment

bb030d0

kertal approved these changes Aug 30, 2024

View reviewed changes

davismcphee added 3 commits August 30, 2024 20:41

Merge branch 'main' into unified-data-table-performance

bca2a64

Add extended Jest timeouts

7dfa8c5

Add more extended Jest timeouts

adbbb9c

davismcphee added the ci:build-webpack-bundle-analyzer label Aug 31, 2024

Merge branch 'main' into unified-data-table-performance

3453786

davismcphee added 2 commits August 31, 2024 02:01

Fix functional test

82e75d2

Merge branch 'main' into unified-data-table-performance

193176e

logeekal approved these changes Sep 2, 2024

View reviewed changes

davismcphee commented Sep 3, 2024

View reviewed changes

Fix test

492c366

lukasolson approved these changes Sep 3, 2024

View reviewed changes

davismcphee merged commit 2629fa8 into elastic:main Sep 4, 2024
41 checks passed

davismcphee deleted the unified-data-table-performance branch September 4, 2024 03:26

kibanamachine added the v8.16.0 label Sep 4, 2024

davismcphee mentioned this pull request Sep 9, 2024

[Discover] [Unified Data Table] Conditionally use overscanRowCount when Document column isn't visible #192332

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] [Unified Data Table] Improve Discover / Unified Data Table performance in ES|QL mode #191249

[Discover] [Unified Data Table] Improve Discover / Unified Data Table performance in ES|QL mode #191249

davismcphee commented Aug 26, 2024 •

edited

Loading

davismcphee commented Aug 26, 2024

davismcphee commented Aug 26, 2024

davismcphee commented Aug 27, 2024

davismcphee commented Aug 28, 2024

davismcphee commented Aug 28, 2024

elasticmachine commented Aug 28, 2024

elasticmachine commented Aug 28, 2024

davismcphee commented Aug 28, 2024

kertal commented Aug 28, 2024

davismcphee commented Aug 28, 2024

davismcphee Aug 29, 2024

logeekal Aug 29, 2024

logeekal Aug 29, 2024

davismcphee Aug 30, 2024

logeekal Aug 30, 2024

davismcphee Sep 2, 2024

stratoula commented Aug 29, 2024 •

edited

Loading

kertal left a comment

stratoula commented Aug 30, 2024 •

edited

Loading

davismcphee commented Aug 31, 2024

davismcphee commented Aug 31, 2024

stratoula commented Sep 2, 2024

logeekal left a comment

davismcphee Sep 3, 2024

lukasolson Sep 3, 2024

kibana-ci commented Sep 3, 2024

lukasolson Sep 3, 2024

[Discover] [Unified Data Table] Improve Discover / Unified Data Table performance in ES|QL mode #191249

[Discover] [Unified Data Table] Improve Discover / Unified Data Table performance in ES|QL mode #191249

Conversation

davismcphee commented Aug 26, 2024 • edited Loading

Summary

Checklist

For maintainers

davismcphee commented Aug 26, 2024

davismcphee commented Aug 26, 2024

davismcphee commented Aug 27, 2024

davismcphee commented Aug 28, 2024

davismcphee commented Aug 28, 2024

elasticmachine commented Aug 28, 2024

elasticmachine commented Aug 28, 2024

davismcphee commented Aug 28, 2024

kertal commented Aug 28, 2024

davismcphee commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stratoula commented Aug 29, 2024 • edited Loading

kertal left a comment

Choose a reason for hiding this comment

stratoula commented Aug 30, 2024 • edited Loading

davismcphee commented Aug 31, 2024

davismcphee commented Aug 31, 2024

stratoula commented Sep 2, 2024

logeekal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Sep 3, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Choose a reason for hiding this comment

davismcphee commented Aug 26, 2024 •

edited

Loading

stratoula commented Aug 29, 2024 •

edited

Loading

stratoula commented Aug 30, 2024 •

edited

Loading