-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Discover] [Unified Data Table] Improve Discover / Unified Data Table…
… performance in ES|QL mode (#191249) ## 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](https://eui.elastic.co/#/tabular-content/data-grid-advanced#data-grid-in-memory) 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](https://github.com/bvaughn/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: https://github.com/user-attachments/assets/04caca0d-35e7-421c-b8e3-5a37a1d65177 After - 424 `UnifiedDataTableRenderCellValue` render calls: https://github.com/user-attachments/assets/dba1a106-6bd6-4f1b-b60e-4741228c488b Before profile: <img width="300" alt="profile_old" src="https://github.com/user-attachments/assets/44c07f30-55d2-44ab-a9b7-a912fa7beca1"> After profile: <img width="300" alt="profile_new" src="https://github.com/user-attachments/assets/d61cfd2a-8ac0-45bb-9afa-d3d97b0d03ce"> Before scrolling: https://github.com/user-attachments/assets/8d9dbdbd-df3e-48df-af9a-539deda8d5ea After scrolling: https://github.com/user-attachments/assets/d6309275-e00d-44ee-a59f-2d1e5d30bb60 ### 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 - [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 - [ ] [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) --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Jatin Kathuria <[email protected]>
- Loading branch information
3 people
authored
Sep 4, 2024
1 parent
8b2cb75
commit 2629fa8
Showing
11 changed files
with
1,137 additions
and
700 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1,508 changes: 914 additions & 594 deletions
1,508
packages/kbn-unified-data-table/src/components/data_table.test.tsx
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
113 changes: 113 additions & 0 deletions
113
packages/kbn-unified-data-table/src/hooks/use_sorting.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import type { DataView } from '@kbn/data-views-plugin/public'; | ||
| import type { DataTableRecord } from '@kbn/discover-utils'; | ||
| import { getSortingCriteria } from '@kbn/sort-predicates'; | ||
| import { useMemo } from 'react'; | ||
| import type { EuiDataGridColumnSortingConfig, EuiDataGridProps } from '@elastic/eui'; | ||
| import type { SortOrder } from '../components/data_table'; | ||
| import type { DataTableColumnsMeta } from '../types'; | ||
|
|
||
| export const useSorting = ({ | ||
| rows, | ||
| visibleColumns, | ||
| columnsMeta, | ||
| sort, | ||
| dataView, | ||
| isPlainRecord, | ||
| isSortEnabled, | ||
| defaultColumns, | ||
| onSort, | ||
| }: { | ||
| rows: DataTableRecord[] | undefined; | ||
| visibleColumns: string[]; | ||
| columnsMeta: DataTableColumnsMeta | undefined; | ||
| sort: SortOrder[]; | ||
| dataView: DataView; | ||
| isPlainRecord: boolean; | ||
| isSortEnabled: boolean; | ||
| defaultColumns: boolean; | ||
| onSort: ((sort: string[][]) => void) | undefined; | ||
| }) => { | ||
| const sortingColumns = useMemo(() => { | ||
| return sort | ||
| .map(([id, direction]) => ({ id, direction })) | ||
| .filter((col) => visibleColumns.includes(col.id)) as EuiDataGridColumnSortingConfig[]; | ||
| }, [sort, visibleColumns]); | ||
|
|
||
| const comparators = useMemo(() => { | ||
| if (!isPlainRecord || !rows || !sortingColumns.length) { | ||
| return; | ||
| } | ||
|
|
||
| return sortingColumns.reduce<Array<(a: DataTableRecord, b: DataTableRecord) => number>>( | ||
| (acc, { id, direction }) => { | ||
| const field = dataView.fields.getByName(id); | ||
|
|
||
| if (!field) { | ||
| return acc; | ||
| } | ||
|
|
||
| const sortField = getSortingCriteria( | ||
| columnsMeta?.[id]?.type ?? field.type, | ||
| id, | ||
| dataView.getFormatterForField(field) | ||
| ); | ||
|
|
||
| acc.push((a, b) => sortField(a.flattened, b.flattened, direction as 'asc' | 'desc')); | ||
|
|
||
| return acc; | ||
| }, | ||
| [] | ||
| ); | ||
| }, [columnsMeta, dataView, isPlainRecord, rows, sortingColumns]); | ||
|
|
||
| const sortedRows = useMemo(() => { | ||
| if (!rows || !comparators) { | ||
| return rows; | ||
| } | ||
|
|
||
| return [...rows].sort((a, b) => { | ||
| for (const comparator of comparators) { | ||
| const result = comparator(a, b); | ||
|
|
||
| if (result !== 0) { | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| return 0; | ||
| }); | ||
| }, [comparators, rows]); | ||
|
|
||
| const sorting = useMemo<EuiDataGridProps['sorting']>(() => { | ||
| if (!isSortEnabled) { | ||
| return { | ||
| columns: sortingColumns, | ||
| onSort: () => {}, | ||
| }; | ||
| } | ||
|
|
||
| // in ES|QL mode, sorting is disabled when in Document view | ||
| // ideally we want the @timestamp column to be sortable server side | ||
| // but it needs discussion before moving forward like this | ||
| if (isPlainRecord && defaultColumns) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return { | ||
| columns: sortingColumns, | ||
| onSort: (sortingColumnsData) => { | ||
| onSort?.(sortingColumnsData.map(({ id, direction }) => [id, direction])); | ||
| }, | ||
| }; | ||
| }, [isSortEnabled, isPlainRecord, defaultColumns, sortingColumns, onSort]); | ||
|
|
||
| return { sortedRows, sorting }; | ||
| }; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.