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

kqualters-elastic · 2022-05-03T04:32:25Z

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

Unit or functional tests were updated or added to match the most common scenarios

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.

…props

…r-actions

…hover-actions

janmonschke · 2022-05-12T15:51:38Z

x-pack/plugins/security_solution/public/common/components/authentication/helpers.tsx

@@ -97,6 +97,8 @@ const FAILURES_COLUMN: Columns<AuthenticationsEdges, AuthenticationsEdges> = {
            operator: IS_OPERATOR,
          },
        }}
+        isAggregatable={true}
+        fieldType={'keyword'}


fieldType="keyword" 💅

michaelolo24 · 2022-05-12T18:24:04Z

.../plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.tsx

-  //    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 =


Thanks for cleaning this up

michaelolo24 · 2022-05-12T18:25:25Z

x-pack/plugins/security_solution/public/common/components/ml/score/draggable_score.tsx

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


We could do isAggregatable and fieldType="keyword"

michaelolo24 · 2022-05-12T18:27:16Z

x-pack/plugins/security_solution/public/common/components/tables/helpers.tsx

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


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

removed the default values

michaelolo24 · 2022-05-12T18:32:19Z

x-pack/plugins/security_solution/public/timelines/components/timeline/body/events/index.tsx

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


merge conflict change?

michaelolo24 · 2022-05-12T18:34:40Z

...urity_solution/public/timelines/components/timeline/body/renderers/plain_column_renderer.tsx

@@ -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 ?? ''}


michaelolo24

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 · 2022-05-12T19:32:28Z

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 · 2022-05-12T19:32:59Z

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

janmonschke

🎉

janmonschke · 2022-05-16T09:21:13Z

...s/security_solution/public/timelines/components/timeline/body/renderers/bytes/index.test.tsx

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


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

angorayc

Thanks for the improvement!

kibana-ci · 2022-05-16T15:41:09Z

💚 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

💚 Build #44606 succeeded f20041d
💛 Build #44330 was flaky dfd636a
💔 Build #44295 failed 530ff2e
💔 Build #43370 failed db52dd2
💛 Build #43076 was flaky 125d9d0
💚 Build #42678 succeeded d2df97d

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

kibanamachine · 2022-05-16T15:44:43Z

💔 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

…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

…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>

kqualters-elastic added 3 commits May 2, 2022 10:31

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

1ddf8c0

Remove sourcerer hook from useHoverActions and pass needed fields as …

959b049

…props

Merge remote-tracking branch 'upstream/main' into browser-fields-hove…

ccbb9eb

…r-actions

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

Merge remote-tracking branch 'upstream/main' into browser-fields-hove…

40465da

…r-actions

kqualters-elastic force-pushed the browser-fields-hover-actions branch from 73e1a56 to 40465da Compare May 3, 2022 04:35

michaelolo24 added the ci:deploy-cloud label May 3, 2022

kqualters-elastic added 3 commits May 3, 2022 13:00

Update snapshots, remove ReactDOM batching

df7aeaf

Merge remote-tracking branch 'upstream/main' into browser-fields-hove…

57f3f98

…r-actions

Make row renderers aggregatable where appropriate

45c13c0

kqualters-elastic marked this pull request as ready for review May 3, 2022 17:24

kqualters-elastic requested review from a team as code owners May 3, 2022 17:24

kqualters-elastic and others added 2 commits May 3, 2022 13:25

Merge branch 'main' of github.com:elastic/kibana into browser-fields-…

1617ae0

…hover-actions

add pagination to details table

4214fa3

michaelolo24 mentioned this pull request May 3, 2022

[WIP][Security Solution] #131452

Closed

9 tasks

kqualters-elastic and others added 9 commits May 3, 2022 16:19

Fix hover actions on host/network details

3e171e3

Remove unneeded props

6f55873

fix table pagination tests

45ef2b7

update test

5a09f1a

Merge branch 'main' into browser-fields-hover-actions

d2df97d

Show top n for authentications and threat indicator match rules

125d9d0

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

990e15f

Pass props on wrapper and not data provider

db52dd2

Merge branch 'main' into browser-fields-hover-actions

5903ef9

kqualters-elastic added 2 commits May 12, 2022 02:04

Add missing row renderer draggables to use top N props

530ff2e

Update snapshots

dfd636a

janmonschke reviewed May 12, 2022

View reviewed changes

michaelolo24 reviewed May 12, 2022

View reviewed changes

michaelolo24 approved these changes May 12, 2022

View reviewed changes

Pr feedback

f20041d

janmonschke approved these changes May 16, 2022

View reviewed changes

angorayc approved these changes May 16, 2022

View reviewed changes

Merge branch 'main' into browser-fields-hover-actions

0ff6ed5

oatkiller enabled auto-merge (squash) May 16, 2022 14:39

oatkiller merged commit f3c1ad7 into elastic:main May 16, 2022

oatkiller mentioned this pull request May 16, 2022

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

Merged

michaelolo24 added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels May 19, 2022

nastasha-solomon mentioned this pull request May 19, 2022

[DOCS] 8.2.1 Release Notes elastic/security-docs#1962

Closed

michaelolo24 assigned kqualters-elastic May 31, 2022

yctercero mentioned this pull request Aug 2, 2022

[Security Solution][Data Views][Discuss] - Updating sourcerer/data view state management #137885

Open

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kqualters-elastic commented May 3, 2022 •

edited

Loading

janmonschke May 12, 2022 •

edited

Loading

michaelolo24 May 12, 2022

michaelolo24 May 12, 2022

michaelolo24 May 12, 2022

kqualters-elastic May 12, 2022

michaelolo24 May 12, 2022

michaelolo24 May 12, 2022

michaelolo24 left a comment •

edited

Loading

kqualters-elastic commented May 12, 2022

kqualters-elastic commented May 12, 2022

janmonschke left a comment

janmonschke May 16, 2022

angorayc left a comment

kibana-ci commented May 16, 2022

kibanamachine commented May 16, 2022

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

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

Conversation

kqualters-elastic commented May 3, 2022 • edited Loading

Summary

Areas of the app affected:

How This Improves Performance

Checklist

8.2.1 Perf PR Checklist

janmonschke May 12, 2022 • edited Loading

Choose a reason for hiding this comment

michaelolo24 May 12, 2022

Choose a reason for hiding this comment

michaelolo24 May 12, 2022

Choose a reason for hiding this comment

michaelolo24 May 12, 2022

Choose a reason for hiding this comment

kqualters-elastic May 12, 2022

Choose a reason for hiding this comment

michaelolo24 May 12, 2022

Choose a reason for hiding this comment

michaelolo24 May 12, 2022

Choose a reason for hiding this comment

michaelolo24 left a comment • edited Loading

Choose a reason for hiding this comment

kqualters-elastic commented May 12, 2022

kqualters-elastic commented May 12, 2022

janmonschke left a comment

Choose a reason for hiding this comment

janmonschke May 16, 2022

Choose a reason for hiding this comment

angorayc left a comment

Choose a reason for hiding this comment

kibana-ci commented May 16, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented May 16, 2022

💔 All backports failed

Manual backport

Questions ?

kqualters-elastic commented May 3, 2022 •

edited

Loading

janmonschke May 12, 2022 •

edited

Loading

michaelolo24 left a comment •

edited

Loading