[Security Solution][Investigations][Tech Debt] - Remove React.memo deepEqual checks #124151
Closed
2 tasks
Assignees
Labels
Team:Threat Hunting:Investigations
Security Solution Investigations Team
Team:Threat Hunting
Security Solution Threat Hunting Team
technical debt
Improvement of the software architecture and operational architecture
Comments
michaelolo24
added
Team:Threat Hunting:Investigations
2 tasks
|
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
andrew-goldstein
added a commit
to andrew-goldstein/kibana
that referenced
this issue
Mar 21, 2022
…ewer`: remove `deepEqual` checks and replace `connect` + `mapStateToProps` with `useSelector` This tech debt PR updates the `StatefulEventsViewer` component to: - Remove `deepEqual` checks, as detailed in <elastic#124151> - Replace the usage of legacy Redux APIs `connect` and `mapStateToProps` with `useSelector`, as detailed in <elastic#124146> ### Methodology Before making changes, the Alerts page, which uses the `StatefulEventsViewer` was profiled with the `Record why each component rendered wile profiling` setting in the React dev tools Profiler, shown in the screenshot below:  _Above: The `Record why each component rendered wile profiling` setting in the React dev tools Profiler_ After the page fully loaded, the profiler was started before clicking the `Refresh` button, and stopped after the page completed the refresh. With a baseline of performance established, the code was refactored to: - Remove the custom equality check passed to `React.memo`, and all the calls to `deepEqual` it contained - Remove `mapStateToProps`, which contained multiple invocations of selectors created by [reselect](https://github.com/reduxjs/reselect)'s [createSelector](https://github.com/reduxjs/reselect#createselectorinputselectors--inputselectors-resultfunc-selectoroptions) API - Using [reselect](https://github.com/reduxjs/reselect)'s [createSelector](https://github.com/reduxjs/reselect#createselectorinputselectors--inputselectors-resultfunc-selectoroptions) API, combine those multiple selectors into a single selector: ```ts export const eventsViewerSelector = createSelector( globalFiltersQuerySelector(), getTimelineSelector(), globalQuerySelector(), globalQuery(), timelineQueryByIdSelector(), getTimelineByIdSelector(), (filters, input, query, globalQueries, timelineQuery, timeline) => ({ /** an array representing filters added to the search bar */ filters, /** an object containing the timerange set in the global date picker, and other page level state */ input, /** a serialized representation of the KQL / Lucence query in the search bar */ query, /** an array of objects with metadata and actions related to queries on the page */ globalQueries, /** an object with metadata and actions related to the table query */ timelineQuery, /** a specific timeline from the state's timelineById collection, or undefined */ timeline, }) ); ``` - Replace the usage of `connect` with a single invocation of `useSelector`, which is provided the new `eventsViewerSelector` selector: ```ts const { filters, input, // ... } = useSelector((state: State) => eventsViewerSelector(state, id)); ``` - A unit test was created for the new `eventsViewerSelector`, using mock Redux state from the Alerts page - After making the changes above, the Alerts page was profiled again to verify `StatefulEventsViewer` re-renders as expected, per the screenshot below:  _Above: In the profiler, each `Why did this render?` entry for the `StatefulEventsViewer` was examined after the changes_
andrew-goldstein
added a commit
that referenced
this issue
Mar 21, 2022
…r`: remove `deepEqual` checks and replace `connect` + `mapStateToProps` with `useSelector` (#128028) ## [Security Solution] [Investigations] [Tech Debt] `StatefulEventsViewer`: remove `deepEqual` checks and replace `connect` + `mapStateToProps` with `useSelector` This tech debt PR updates the `StatefulEventsViewer` component to: - Remove `deepEqual` checks, as detailed in <#124151> - Replace the usage of legacy Redux APIs `connect` and `mapStateToProps` with `useSelector`, as detailed in <#124146> ### Methodology Before making changes, the Alerts page, which uses the `StatefulEventsViewer` was profiled with the `Record why each component rendered wile profiling` setting in the React dev tools Profiler, shown in the screenshot below:  _Above: The `Record why each component rendered wile profiling` setting in the React dev tools Profiler_ After the page fully loaded, the profiler was started before clicking the `Refresh` button, and stopped after the page completed the refresh. With a baseline of performance established, the code was refactored to: - Remove the custom equality check passed to `React.memo`, and all the calls to `deepEqual` it contained - Remove `mapStateToProps`, which contained multiple invocations of selectors created by [reselect](https://github.com/reduxjs/reselect)'s [createSelector](https://github.com/reduxjs/reselect#createselectorinputselectors--inputselectors-resultfunc-selectoroptions) API - Using [reselect](https://github.com/reduxjs/reselect)'s [createSelector](https://github.com/reduxjs/reselect#createselectorinputselectors--inputselectors-resultfunc-selectoroptions) API, combine those multiple selectors into a single selector: ```ts export const eventsViewerSelector = createSelector( globalFiltersQuerySelector(), getTimelineSelector(), globalQuerySelector(), globalQuery(), timelineQueryByIdSelector(), getTimelineByIdSelector(), (filters, input, query, globalQueries, timelineQuery, timeline) => ({ /** an array representing filters added to the search bar */ filters, /** an object containing the timerange set in the global date picker, and other page level state */ input, /** a serialized representation of the KQL / Lucence query in the search bar */ query, /** an array of objects with metadata and actions related to queries on the page */ globalQueries, /** an object with metadata and actions related to the table query */ timelineQuery, /** a specific timeline from the state's timelineById collection, or undefined */ timeline, }) ); ``` - Replace the usage of `connect` with a single invocation of `useSelector`, which is provided the new `eventsViewerSelector` selector: ```ts const { filters, input, // ... } = useSelector((state: State) => eventsViewerSelector(state, id)); ``` - A unit test was created for the new `eventsViewerSelector`, using mock Redux state from the Alerts page - After making the changes above, the Alerts page was profiled again to verify `StatefulEventsViewer` re-renders as expected, per the screenshot below:  _Above: In the profiler, each `Why did this render?` entry for the `StatefulEventsViewer` was examined after the changes_
andrew-goldstein
added a commit
to andrew-goldstein/kibana
that referenced
this issue
Apr 4, 2022
…emove `deepEqual` checks and replace `connect` + `mapStateToProps` with `useSelector` This tech debt PR updates the Timeline `StatefulBody` component as part of a series of PRs to: - Remove `deepEqual` checks, as detailed in <elastic#124151> - Replace the usage of legacy Redux APIs `connect` and `mapStateToProps` with `useSelector`, as detailed in <elastic#124146> There are no visual changes associated with this PR.
andrew-goldstein
added a commit
that referenced
this issue
Apr 4, 2022
…ve `deepEqual` checks and replace `connect` + `mapStateToProps` with `useSelector` (#129160) ## [Security Solution] [Investigations] [Tech Debt] `StatefulBody`: remove `deepEqual` checks and replace `connect` + `mapStateToProps` with `useSelector` This tech debt PR updates the Timeline `StatefulBody` component as part of a series of PRs to: - Remove `deepEqual` checks, as detailed in <#124151> - Replace the usage of legacy Redux APIs `connect` and `mapStateToProps` with `useSelector`, as detailed in <#124146> There are no visual changes associated with this PR.
andrew-goldstein
added a commit
to andrew-goldstein/kibana
that referenced
this issue
Apr 21, 2022
…l` checks in column headers and data providers This tech debt PR is another entry in a series to remove `React.memo` `deepEqual` checks, per the details in <elastic#124151> - It removes `deepEqual` checks in Timeline's column headers and data providers - Files made redundant by the `timelines` plugin adopting `EuiDataGrid` are deleted ### Methodology The following techniques were used to ensure that removing the `deepEqual` checks did NOT result in unexpected re-renders: - To understand why components re-rendered, Timeline was profiled with the `Record why each component rendered wile profiling` setting in the React dev tools Profiler enabled, shown in the (illustrative) screenshot below:  - Components were temporarily instrumented with counters that incremented every time the component was rendered. Log statements prefixed with `[pre]` were observed before making changes, per the screenshot below:  - After removing the `deepEqual` checks, the log prefix was updated to `[POST]`, per the screenshot below:  The `[pre]` and `[POST]` counters were compared to verify removing the `deepEqual` checks did NOT introduce unexpected re-renders.
andrew-goldstein
added a commit
that referenced
this issue
Apr 21, 2022
…ecks in column headers and data providers (#130740) ## [Security Solution] [Investigations] [Tech Debt] removes `deepEqual` checks in column headers and data providers This tech debt PR is another entry in a series to remove `React.memo` `deepEqual` checks, per the details in <#124151> - It removes `deepEqual` checks in Timeline's column headers and data providers - Files made redundant by the `timelines` plugin adopting `EuiDataGrid` are deleted ### Methodology The following techniques were used to ensure that removing the `deepEqual` checks did NOT result in unexpected re-renders: - To understand why components re-rendered, Timeline was profiled with the `Record why each component rendered wile profiling` setting in the React dev tools Profiler enabled, shown in the (illustrative) screenshot below:  - Components were temporarily instrumented with counters that incremented every time the component was rendered. Log statements prefixed with `[pre]` were observed before making changes, per the screenshot below:  - After removing the `deepEqual` checks, the log prefix was updated to `[POST]`, and the log entries were observed again, per the screenshot below:  The `[pre]` and `[POST]` counters were compared to verify removing the `deepEqual` checks did NOT introduce unexpected re-renders.
kertal
pushed a commit
to kertal/kibana
that referenced
this issue
May 24, 2022
…ecks in column headers and data providers (elastic#130740) ## [Security Solution] [Investigations] [Tech Debt] removes `deepEqual` checks in column headers and data providers This tech debt PR is another entry in a series to remove `React.memo` `deepEqual` checks, per the details in <elastic#124151> - It removes `deepEqual` checks in Timeline's column headers and data providers - Files made redundant by the `timelines` plugin adopting `EuiDataGrid` are deleted ### Methodology The following techniques were used to ensure that removing the `deepEqual` checks did NOT result in unexpected re-renders: - To understand why components re-rendered, Timeline was profiled with the `Record why each component rendered wile profiling` setting in the React dev tools Profiler enabled, shown in the (illustrative) screenshot below:  - Components were temporarily instrumented with counters that incremented every time the component was rendered. Log statements prefixed with `[pre]` were observed before making changes, per the screenshot below:  - After removing the `deepEqual` checks, the log prefix was updated to `[POST]`, and the log entries were observed again, per the screenshot below:  The `[pre]` and `[POST]` counters were compared to verify removing the `deepEqual` checks did NOT introduce unexpected re-renders.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Background:
In an effort to improve the performance of the application and better leverage the use of redux, selectors, and hooks, we would like to remove the use of
deepEqualchecks when usingReact.memowhere possible. The multiple deepEqual checks can have a negative performance impact on the speed of component rendering and be potentially unnecessary when compared against the default behavior of React.memo in conjunction with better organized props and redux use.Details:
The goal of this PR will be to remove the use of
deepEqualin React.memo prop checks. Within the Timelines plugin, those changes would need to take place in:x-pack/plugins/timelines/public/components/t_grid/body/column_headers/index.tsxx-pack/plugins/timelines/public/components/t_grid/body/column_headers/column_header.tsxAnd within Security Solution:
x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsxx-pack/plugins/security_solution/public/timelines/components/formatted_ip/index.tsxx-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/index.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/body/index.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/body/column_headers/column_header.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/body/column_headers/index.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/data_providers/providers.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/eql_tab_content/index.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/pinned_tab_content/index.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/query_bar/index.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/query_tab_content/index.tsxx-pack/plugins/security_solution/public/timelines/components/timeline/search_or_filter/index.tsxIt would be worth checking if the dependency function is needed at all for the components once these changes are made.
Acceptance Criteria:
The text was updated successfully, but these errors were encountered: