[Response Ops] [Alerts table] Cell context alert table performance #180016
Conversation
kqualters-elastic
added
release_note:skip
|
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
botelastic
bot
added
the
Team:obs-ux-management
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
…ic/kibana into cell-context-alert-table
x-pack/plugins/ml/public/alerting/anomaly_detection_alerts_table/render_cell_value.tsx
Show resolved
Hide resolved
...ty_solution/public/common/components/toolbar/bulk_actions/use_bulk_alert_assignees_items.tsx
Show resolved
Hide resolved
| browserFields={browserFields} | ||
| columnId={columnId} | ||
| data={finalData} | ||
| ecsData={ecsData} | ||
| eventId={eventId} | ||
| header={myHeader} | ||
| isDetails={isDetails} | ||
| isDraggable={isDraggable} | ||
| isDraggable={false} |
There was a problem hiding this comment.
Is this correct? I see isDraggable is still a valid prop in EuiDataGridCellProps['cellContext'] type
|
remaining test failures seem to be occurring in main as well |
x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.tsx
Show resolved
Hide resolved
| return valuesField ? values : value; | ||
| case ALERT_REASON: | ||
| const dataFieldEs = data.reduce((acc, d) => ({ ...acc, [d.field]: d.value }), {}); | ||
| if (!observabilityRuleTypeRegistry) return <>{value}</>; |
There was a problem hiding this comment.
Looks like git picked up this whole switch block because it changed indentation, but I think this is the only line that is actually different between the old switch and this one (this line has been added)
| return ( | ||
| <EuiLink | ||
| data-test-subj="o11yGetRenderCellValueLink" | ||
| css={{ display: 'contents' }} | ||
| onClick={() => setFlyoutAlert && setFlyoutAlert(alert.fields[ALERT_UUID])} | ||
| > | ||
| {alert.reason} | ||
| </EuiLink> | ||
| ); | ||
| case ALERT_RULE_NAME: | ||
| const ruleCategory = getMappedNonEcsValue({ | ||
| data, | ||
| fieldName: ALERT_RULE_CATEGORY, | ||
| }); | ||
| const tooltipContent = getRenderValue(ruleCategory); | ||
| return <CellTooltip value={value} tooltipContent={tooltipContent} />; | ||
| default: | ||
| return <>{value}</>; | ||
| } |
There was a problem hiding this comment.
Just for reference, seems to be no substantive differences between these blocks, just indentation change.
There was a problem hiding this comment.
the react.memo is doing that I think
There was a problem hiding this comment.
Yeah something above changed the indentation, it happens. I just wanted to flag it for whoever on my team takes a look at this tomorrow. 👍
There was a problem hiding this comment.
code LGTM and desk tested. What an improvement! On tables with 100 rows displayed the difference is truly huge, awesome work @kqualters-elastic!! I couldn't find anything broken during testing!
There was a problem hiding this comment.
sorry for the stupid question, but could you explain (for my own knowledge) why making the changes in this file? Why moving the constant within each useCallback is a good thing here? Thanks!
| alertTagsItems, | ||
| alertTagsPanels, | ||
| }; | ||
| return useMemo(() => { |
There was a problem hiding this comment.
feel free to ignore this comment but for those simple returns I like writing them this way
return useMemo(
() => ({
alertTagsItems,
alertTagsPanels,
}),
[alertTagsItems, alertTagsPanels]
);There are many similar changes in this PR
There was a problem hiding this comment.
I think we should undo this change, while it's not bad, it doesn't really bring anything valuable to this PR
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
…e in the useGetGroupSelector hook (#184165) ## Summary This PR fixes a regression from PR #180016 where the support to the custom title on the `useGetGroupSelector` hook was dropped and the Findings DataTables was displaying "Group alerts by" instead of "Group findings by". Unit tests were added to prevent further regressions. ## Screenshots   
…e in the useGetGroupSelector hook (elastic#184165) ## Summary This PR fixes a regression from PR elastic#180016 where the support to the custom title on the `useGetGroupSelector` hook was dropped and the Findings DataTables was displaying "Group alerts by" instead of "Group findings by". Unit tests were added to prevent further regressions. ## Screenshots   
Summary
This pr makes use of a new prop (and some generic memoization fixes) in 2 eui prs merged recently (elastic/eui#7556 and elastic/eui#7374) to improve the performance of the alerts table. Mainly, the cellContext prop is now available to consumers of the triggersActionsUi alerts table to pass in custom props in a way that allows the renderCellValue/rowCellRender functions of the various EuiDataGrid prop apis to remain referentially stable across re-renders. There are also some various changes to various hooks and props found throughout plugins that use the table to improve performance. There should be no change in functionality, just a moderate to substantial reduction in time the app spends rendering the alerts table in various scenarios. Below will be some react dev tools performance profiles, main compared to this branch, with the exact same set of generated data.
Main, switching from 10-100 rows:
This branch 10-100 rows:
Pretty modest gain here, 1 render is slower than any others on main, but overall less time spent rendering by about a second.
Main, opening the cell popover on 2 cells
This branch, opening cellpopover
Again nothing crazy here, modest improvement.
Main opening timeline and hitting refresh
This branch, opening timeline and hitting refresh
This is the case that brought this about in the first place, as security solution shares a lot of code between tables, the alerts table recreating all cells on every render was destroying performance of the timeline rendering in a flyout/modal while users were on alerts page or the rule detail page, which is probably the most common use case. 93ms in this branch vs 2500+ ms in main. This type of performance hit happens when almost any action is taken in timeline.
Main, selecting 3 alerts 1 at a time
This branch selecting 3 alerts 1 at a time
Pretty substantial improvement here as well, ~2000ms vs 67ms.
Apologies if some of the gifs are cut off, please try the branch vs main on your own! This was done on a local kibana in dev mode, so things like emotion are eating up more cpu than they would for a user, but there still should be improvement, and I can't think of any circumstance where things will be worse. Also this branch was a little longer lived than I would have liked, so if you are reviewing and changed something around these files recently please double check I didn't miss anything.
Checklist