Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] Remove sourcerer browser fields hover actions to help performance #131363

Merged
merged 22 commits into from
May 16, 2022

Conversation

kqualters-elastic
Copy link
Contributor

@kqualters-elastic kqualters-elastic commented May 3, 2022

Summary

useHoverActions was previously using the useSourcererDataView in every single table cell being rendered on the page to derive information that could already be derived from existing props or a place where the behavior of cells is hard coded, as in most of the cells in basic tables in security solution. This had a massively negative impact on performance, as useSourcererDataView would make network requests when called in some cases, leading to the cluster to fallover, and was probably a tremendous use of memory as well given the size of browser fields in most cases. This pr removes use of that hook, and passes existing props down to the hover actions logic to help performance. Specifically, in event viewer based tables, the fieldFromBrowserField prop already contained the information needed to determine if a field should show the top N cell action. In timeline based views, the header: ColumnHeaderOptions prop (which is more or less a BrowserField with some optional fields and some fields renamed slightly, not sure why) is used to show the top N cell action. For the basic tables where column definitions are defined manually, the props are defined alongside the render behavior for the cell. I think the isAllowedType array we are currently using might need to be revisited, as some of those types are not valid elasticsearch field data types, but this pr is just to maintain the existing functionality.

Areas of the app affected:

Any part of the security solution that has a table cell that uses hover actions.

How This Improves Performance

The useHoverActions hook previously made use of sourcerer hooks directly, and since this hover actions hook is used in every single cell in a table, in some places this would fire off a network request to the index multiple times. For hover actions, even 1 request is too many, as the information that browserFields was used for was already available mostly as props, this created a whole lot of wasted memory in the app.

All variations of hover actions should be working as before:

hover_actions.mp4

Checklist

8.2.1 Perf PR Checklist

  • What areas of the app are affected? We want to focus testing on the areas that we know for sure will be affected, so please list pages and workflows that we should check.
  • Explain how this improves performance. Include a description of how you tested, along with any sample data needed. Include a video and Chrome Performance Profile showing the behavior both before and after the fix, if possible.

@kqualters-elastic kqualters-elastic added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.3.0 v8.2.1 labels May 3, 2022
@kqualters-elastic kqualters-elastic marked this pull request as ready for review May 3, 2022 17:24
@kqualters-elastic kqualters-elastic requested review from a team as code owners May 3, 2022 17:24
@michaelolo24 michaelolo24 mentioned this pull request May 3, 2022
9 tasks
@@ -97,6 +97,8 @@ const FAILURES_COLUMN: Columns<AuthenticationsEdges, AuthenticationsEdges> = {
operator: IS_OPERATOR,
},
}}
isAggregatable={true}
fieldType={'keyword'}
Copy link
Contributor

@janmonschke janmonschke May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fieldType="keyword" 💅

// this component is rendered in the context of the active timeline. This
// behavior enables the 'All events' view by appending the alerts index
// to the index pattern.
const activeScope: SourcererScopeName =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up

@@ -69,6 +69,8 @@ export const DraggableScoreComponent = ({
key={`draggable-score-draggable-wrapper-${id}`}
dataProvider={dataProviderProp}
render={render}
isAggregatable={true}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do isAggregatable and fieldType="keyword"

@@ -219,6 +239,8 @@ interface RowItemOverflowProps {
maxOverflowItems: number;
overflowIndexStart: number;
rowItems: string[];
fieldType?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be optional considering we're defaulting the values so they should always be defined? Just want to prevent any errors in the future by someone accidentally removing these

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the default values

@@ -72,37 +72,39 @@ const EventsComponent: React.FC<Props> = ({
tabType,
leadingControlColumns,
trailingControlColumns,
}) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge conflict change?

@@ -50,9 +50,10 @@ export const plainColumnRenderer: ColumnRenderer = {
asPlainText={asPlainText}
contextId={`plain-column-renderer-formatted-field-value-${timelineId}`}
eventId={eventId}
fieldFormat={field.format || ''}
fieldFormat={field.format ?? ''}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾

Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome work, thanks for dealing with the tedium of this 😓 LGTM! 🚀 Just had a few minor comments about code style:

Tested the presence and functionality of the hover actions in: Tgrid tables(alert, rule, hosts, network) pages, detail pages (rule, host, network, user), timeline (query, correlation, pinned), details flyout

@kqualters-elastic
Copy link
Contributor Author

This is awesome work, thanks for dealing with the tedium of this 😓 LGTM! 🚀 Just had a few minor comments about code style

If there's no lint rule for it I don't want to hear it!

@kqualters-elastic
Copy link
Contributor Author

Kidding, will address both your's and Jan's comments

Copy link
Contributor

@janmonschke janmonschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@@ -26,6 +26,8 @@ describe('Bytes', () => {
contextId="test"
eventId="abc"
fieldName="network.bytes"
fieldType="string"
isAggregatable={true}
isDraggable={true}
value={`1234567`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should really have a lint rule for this :D Is there some sort of styleguide for Kibana?

Copy link
Contributor

@angorayc angorayc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement!

@oatkiller oatkiller enabled auto-merge (squash) May 16, 2022 14:39
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 5.0MB 5.0MB +6.4KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@oatkiller oatkiller merged commit f3c1ad7 into elastic:main May 16, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.2 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 131363

Questions ?

Please refer to the Backport tool documentation

oatkiller pushed a commit to oatkiller/kibana that referenced this pull request May 16, 2022
…help performance (elastic#131363)

* Batch setState calls to make sure all state updates are applied evenly

* Remove sourcerer hook from useHoverActions and pass needed fields as props

* Update snapshots, remove ReactDOM batching

* Make row renderers aggregatable where appropriate

* add pagination to details table

* Fix hover actions on host/network details

* Remove unneeded props

* fix table pagination tests

* update test

* Show top n for authentications and threat indicator match rules

* Fix anomaly score, entity, influence, and user id show top N

* Pass props on wrapper and not data provider

* Add missing row renderer draggables to use top N props

* Update snapshots

* Pr feedback

Co-authored-by: Michael Olorunnisola <michael.olorunnisola@elastic.co>
Co-authored-by: Robert Austin <robert.austin@elastic.co>
(cherry picked from commit f3c1ad7)

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/authentication/helpers.tsx
#	x-pack/plugins/security_solution/public/common/lib/cell_actions/expanded_cell_value_actions.tsx
oatkiller pushed a commit that referenced this pull request May 16, 2022
…ns to help performance (#131363) (#132261)

* [Security Solution] Remove sourcerer browser fields hover actions to help performance (#131363)

* Batch setState calls to make sure all state updates are applied evenly

* Remove sourcerer hook from useHoverActions and pass needed fields as props

* Update snapshots, remove ReactDOM batching

* Make row renderers aggregatable where appropriate

* add pagination to details table

* Fix hover actions on host/network details

* Remove unneeded props

* fix table pagination tests

* update test

* Show top n for authentications and threat indicator match rules

* Fix anomaly score, entity, influence, and user id show top N

* Pass props on wrapper and not data provider

* Add missing row renderer draggables to use top N props

* Update snapshots

* Pr feedback

Co-authored-by: Michael Olorunnisola <michael.olorunnisola@elastic.co>
Co-authored-by: Robert Austin <robert.austin@elastic.co>
(cherry picked from commit f3c1ad7)

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/authentication/helpers.tsx
#	x-pack/plugins/security_solution/public/common/lib/cell_actions/expanded_cell_value_actions.tsx

* meant to delete this

* fix some lint rules i hope

Co-authored-by: Kevin Qualters <56408403+kqualters-elastic@users.noreply.github.com>
@michaelolo24 michaelolo24 added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels May 19, 2022
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed ci:cloud-deploy Create or update a Cloud deployment release_note:fix Team:Threat Hunting:Investigations Security Solution Investigations Team v8.2.1 v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants