[RAM] Add option to disable virtualization in alerts table #169257
Conversation
umbopepato
force-pushed
the
168470-security-alerts-table-perf
branch
from
fa984ba to
0aaa207
Compare
| className={`euiDataGridRow ${ | ||
| rowIndex % 2 !== 0 ? 'euiDataGridRow--striped' : '' | ||
| }`} | ||
| key={rowIndex} |
There was a problem hiding this comment.
maybe key={${rowIndex},${pagination.pageIndex}} like below, rowIndex will be identical when changing pages
| <Row | ||
| role="row" | ||
| className={`euiDataGridRow ${ | ||
| rowIndex % 2 !== 0 ? 'euiDataGridRow--striped' : '' |
There was a problem hiding this comment.
[Optional heads up] If/when EUI converts EuiDataGrid from Sass to Emotion/CSS-in-JS*, this euiDataGridRow--striped className will stop working. You may want to consider baking this logic into your styled.div now, if that's an option - the color being used is euiColorLightestShade.
*Note: if another team ends up taking ownership of EuiDataGrid and it moves out of EUI, this migration may not end up happening, so totally up to you all as if you'd rather address this later.
x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.tsx
Outdated
Show resolved
Hide resolved
| @@ -560,6 +560,10 @@ export type AlertsTableProps = { | |||
| * Allows to consumers of the table to decide to highlight a row based on the current alert. | |||
| */ | |||
| shouldHighlightRow?: (alert: Alert) => boolean; | |||
| /** | |||
| * Enable when rows may have variable heights (disables virtualization) | |||
There was a problem hiding this comment.
[attaching to a semi random line for threading]
Are there any plans on limiting the potential number of rows per page for non-virtualized data grids? It's likely that you're going to run into major lag/perf issues rendering 1000 rows at once, for example 😅
There was a problem hiding this comment.
As of today, we can only show a maximum of 100 alert in the table
There was a problem hiding this comment.
👍 If you run into performance issues with large grids and 100 rows, it may be worth considering going down to a max of 50, or potentially another perf approach like lazy loading.
umbopepato
force-pushed
the
168470-security-alerts-table-perf
branch
from
350bb85 to
0f164d2
Compare
umbopepato
added
release_note:skip
|
Pinging @elastic/response-ops (Team:ResponseOps) |
| stripes?: boolean; | ||
| }; | ||
|
|
||
| const CustomGridBody = ({ |
There was a problem hiding this comment.
should make this a React.memo
…pato/kibana into 168470-security-alerts-table-perf
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Thank you @cee-chen for your help and feedback! 😊 |
Closes #168470
Summary
Adds a
dynamicRowHeightoption to the alerts table that replaces the virtualized table body with a custom non-virtualized one to improve the performance of variable row heights (even though the rows weren't recycled the auto height was causing a lot of unnecessary re-renderings).When enabled, this option reduces the amount of re-renders from ~500 to ~180 when expanding the table from 10 to 100 visible rows:
When combined with a possible solution to #169268 , the number of renders drastically reduces to around 30:
Checklist
Delete any items that are not applicable to this PR.