[Security Solution] Event Renderer Virtualization #193316
Conversation
Screen.Recording.2024-09-23.at.13.57.43.movOverall this seems to be working :) I noticed two small issues:
|
Hey @janmonschke Thanks for the feedback.
|
Avoid height calculation for row renderer, leads to great performance. This was done by passing `auto` height.
Both of these things are fixed and table should feel much faster than before. Could you please take another look? Note you will need to download a new attached version of EUI tarball |
|
@logeekal Just tested it and both issues are gone, wohooo 🎉 I noticed another thing where the table stays blank for quite a while when scrolling. This feels like it comes from the virtualization and I wonder if it performs better when React is not running in dev mode. Do you see the same? Screen.Recording.2024-09-30.at.09.56.14.mov |
Great. Thanks Jan for giving it a shot. To answer your question, if the delay in rows appearing is production grade or not, it is difficult to answer in absolute terms. Could you compare below 3 scenarios and see if the delay is still too much?
When comparing we can actually measure where these new virtualization changes actually stand. |
|
Thanks @logeekal. I tested it again and this time around it was much faster. Not sure what was happening on my machine earlier. The simple rows render much faster than the event renderers but both feel fast enough. |
| }, []); | ||
|
|
||
| const innerRowContainer = useMemo(() => { | ||
| const InnerComp = React.forwardRef<HTMLDivElement, PropsWithChildren<{}>>( |
There was a problem hiding this comment.
can probably pull this definition to the top level of this file, and that way this is never recreated.
There was a problem hiding this comment.
yes.. I was just waiting for the EUI teams opinion and then i will refactor code to be put in production. Currently, this is just working code.
I wanted to get everyone opinions mainly on performance. But what you say makes sense.
There was a problem hiding this comment.
@kqualters-elastic , I just checked, it has depenendencies headerRow and footerRow and hence it cannot be pulled out.
…ity timeline table
… no longer has text - text is now entirely SR only
…body_list_virtualization
logeekal
added
the
release_note:skip
No. The alert table height was intentionally made limited to enable virtualization because otherwise performance was pretty bad. This was done in PR : #192827 |
|
/ci |
...y_solution/public/timelines/components/timeline/tabs/shared/use_timeline_control_columns.tsx
Show resolved
Hide resolved
...melines/components/timeline/unified_components/data_table/custom_timeline_data_grid_body.tsx
Show resolved
Hide resolved
| indicatorRuleMatchingDoc.atomic | ||
| }${accessibilityText}threat.enrichments.matched.typeindicator_match_rule${accessibilityText}provided` + |
There was a problem hiding this comment.
Hey, why we remove this accessibilityText?
There was a problem hiding this comment.
The accessibility text was related to draggables in Event Renderers and they are no longer draggables. I have created a follow-up issue to add a new text for all Event Renderers and not just this : #196505
💛 Build succeeded, but was flaky
Failed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
|
|
Starting backport for target branches: 8.x |
## Summary This PR implements virtualization when Event Renderers are enabled. Ideally from UX pespective nothing should change but from performance perspective, the event renderers should be scalable. ### Testing checklist 1. UX is working same as before when Event Renderers are enabled. 2. Operations such as increasing page size from `10` to `100` are not taking as much time as before. Below operations can be used to test. a. Closing / Opening Timeline b. Changes `Rows per page` c. Changes tabs from query to any other and back. ### Before In below video, you will notice how long it took to change `pageSize` to 100 and all 100 rows are rendered at once. https://github.com/user-attachments/assets/106669c9-bda8-4b7d-af3f-b64824bde397 ### After https://github.com/user-attachments/assets/356d9e1f-caf1-4f88-9223-0e563939bf6b > [!Note] > 1. Also test in small screen. The table should be scrollable but nothing out of ordinary. > 2. Additionally, try to load data which has `network_flow` process so as to create bigger and varied Event Renderers. --------- Co-authored-by: Cee Chen <[email protected]> (cherry picked from commit fa92a8e)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…96587) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Event Renderer Virtualization (#193316)](#193316) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Jatin Kathuria","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-16T17:53:49Z","message":"[Security Solution] Event Renderer Virtualization (#193316)\n\n## Summary\r\n\r\nThis PR implements virtualization when Event Renderers are enabled.\r\nIdeally from UX pespective nothing should change but from performance\r\nperspective, the event renderers should be scalable.\r\n\r\n### Testing checklist\r\n\r\n1. UX is working same as before when Event Renderers are enabled.\r\n2. Operations such as increasing page size from `10` to `100` are not\r\ntaking as much time as before. Below operations can be used to test.\r\n a. Closing / Opening Timeline\r\n b. Changes `Rows per page`\r\n c. Changes tabs from query to any other and back.\r\n\r\n### Before\r\nIn below video, you will notice how long it took to change `pageSize` to\r\n100 and all 100 rows are rendered at once.\r\n\r\n\r\nhttps://github.com/user-attachments/assets/106669c9-bda8-4b7d-af3f-b64824bde397\r\n\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/356d9e1f-caf1-4f88-9223-0e563939bf6b\r\n\r\n\r\n\r\n> [!Note]\r\n> 1. Also test in small screen. The table should be scrollable but\r\nnothing out of ordinary.\r\n> 2. Additionally, try to load data which has `network_flow` process so\r\nas to create bigger and varied Event Renderers.\r\n\r\n---------\r\n\r\nCo-authored-by: Cee Chen <[email protected]>","sha":"fa92a8ede7bce32456e3d6a6307761b4209248f9","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["backport","release_note:skip","v9.0.0","Team:Threat Hunting:Investigations","ci:project-deploy-security","v8.16.0"],"title":"[Security Solution] Event Renderer Virtualization","number":193316,"url":"https://github.com/elastic/kibana/pull/193316","mergeCommit":{"message":"[Security Solution] Event Renderer Virtualization (#193316)\n\n## Summary\r\n\r\nThis PR implements virtualization when Event Renderers are enabled.\r\nIdeally from UX pespective nothing should change but from performance\r\nperspective, the event renderers should be scalable.\r\n\r\n### Testing checklist\r\n\r\n1. UX is working same as before when Event Renderers are enabled.\r\n2. Operations such as increasing page size from `10` to `100` are not\r\ntaking as much time as before. Below operations can be used to test.\r\n a. Closing / Opening Timeline\r\n b. Changes `Rows per page`\r\n c. Changes tabs from query to any other and back.\r\n\r\n### Before\r\nIn below video, you will notice how long it took to change `pageSize` to\r\n100 and all 100 rows are rendered at once.\r\n\r\n\r\nhttps://github.com/user-attachments/assets/106669c9-bda8-4b7d-af3f-b64824bde397\r\n\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/356d9e1f-caf1-4f88-9223-0e563939bf6b\r\n\r\n\r\n\r\n> [!Note]\r\n> 1. Also test in small screen. The table should be scrollable but\r\nnothing out of ordinary.\r\n> 2. Additionally, try to load data which has `network_flow` process so\r\nas to create bigger and varied Event Renderers.\r\n\r\n---------\r\n\r\nCo-authored-by: Cee Chen <[email protected]>","sha":"fa92a8ede7bce32456e3d6a6307761b4209248f9"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193316","number":193316,"mergeCommit":{"message":"[Security Solution] Event Renderer Virtualization (#193316)\n\n## Summary\r\n\r\nThis PR implements virtualization when Event Renderers are enabled.\r\nIdeally from UX pespective nothing should change but from performance\r\nperspective, the event renderers should be scalable.\r\n\r\n### Testing checklist\r\n\r\n1. UX is working same as before when Event Renderers are enabled.\r\n2. Operations such as increasing page size from `10` to `100` are not\r\ntaking as much time as before. Below operations can be used to test.\r\n a. Closing / Opening Timeline\r\n b. Changes `Rows per page`\r\n c. Changes tabs from query to any other and back.\r\n\r\n### Before\r\nIn below video, you will notice how long it took to change `pageSize` to\r\n100 and all 100 rows are rendered at once.\r\n\r\n\r\nhttps://github.com/user-attachments/assets/106669c9-bda8-4b7d-af3f-b64824bde397\r\n\r\n\r\n### After\r\n\r\n\r\nhttps://github.com/user-attachments/assets/356d9e1f-caf1-4f88-9223-0e563939bf6b\r\n\r\n\r\n\r\n> [!Note]\r\n> 1. Also test in small screen. The table should be scrollable but\r\nnothing out of ordinary.\r\n> 2. Additionally, try to load data which has `network_flow` process so\r\nas to create bigger and varied Event Renderers.\r\n\r\n---------\r\n\r\nCo-authored-by: Cee Chen <[email protected]>","sha":"fa92a8ede7bce32456e3d6a6307761b4209248f9"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Jatin Kathuria <[email protected]>
## Summary This PR implements virtualization when Event Renderers are enabled. Ideally from UX pespective nothing should change but from performance perspective, the event renderers should be scalable. ### Testing checklist 1. UX is working same as before when Event Renderers are enabled. 2. Operations such as increasing page size from `10` to `100` are not taking as much time as before. Below operations can be used to test. a. Closing / Opening Timeline b. Changes `Rows per page` c. Changes tabs from query to any other and back. ### Before In below video, you will notice how long it took to change `pageSize` to 100 and all 100 rows are rendered at once. https://github.com/user-attachments/assets/106669c9-bda8-4b7d-af3f-b64824bde397 ### After https://github.com/user-attachments/assets/356d9e1f-caf1-4f88-9223-0e563939bf6b > [!Note] > 1. Also test in small screen. The table should be scrollable but nothing out of ordinary. > 2. Additionally, try to load data which has `network_flow` process so as to create bigger and varied Event Renderers. --------- Co-authored-by: Cee Chen <[email protected]>
Summary
This PR implements virtualization when Event Renderers are enabled. Ideally from UX pespective nothing should change but from performance perspective, the event renderers should be scalable.
Testing checklist
10to100are not taking as much time as before. Below operations can be used to test.a. Closing / Opening Timeline
b. Changes
Rows per pagec. Changes tabs from query to any other and back.
Before
In below video, you will notice how long it took to change
pageSizeto 100 and all 100 rows are rendered at once.event_renderers_without_virtualization.mp4
After
event_renderers_with_virtualization.mp4
Note
network_flowprocess so as to create bigger and varied Event Renderers.