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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Remove a data fetching hook from the add to timeline action component #124331

Merged
merged 15 commits into from
Feb 8, 2022
Merged

[Security Solution] Remove a data fetching hook from the add to timeline action component #124331

merged 15 commits into from
Feb 8, 2022

Conversation

kqualters-elastic
Copy link
Contributor

Summary

Fixes #124307 for all alert types by removing a hook that conditionally fetched data in every row. Instead this logic is moved into the callback from the click, so the data is fetched once and only once and only when needed. This api call can and should be removed soon by updating the alerts table search strategy to return all needed fields for building the timeline filters/data providers etc. By adding all kibana.alert.* referenced in https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx to https://github.com/elastic/kibana/blob/main/x-pack/plugins/timelines/server/search_strategy/timeline/factory/helpers/constants.ts#L45 the request that was previously made in useFetchEcsAlertsData and moved to https://github.com/elastic/kibana/compare/main...kqualters-elastic:fetch-ecs-in-actions?expand=1#diff-972b23879f146e612234cef06ac25c143709767be35c69d621861d7b50d8ce24R402 can be removed entirely.

Checklist

@kqualters-elastic kqualters-elastic added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 Team:Threat Hunting:Investigations Security Solution Investigations Team labels Feb 2, 2022
@kqualters-elastic
Copy link
Contributor Author

/oblt-deploy

@kqualters-elastic kqualters-elastic marked this pull request as ready for review February 2, 2022 14:52
@kqualters-elastic kqualters-elastic requested a review from a team as a code owner February 2, 2022 14:52
@kqualters-elastic
Copy link
Contributor Author

/oblt-deploy

michaelolo24
michaelolo24 reviewed Feb 2, 2022
View reviewed changes
x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx Outdated
estypes.SearchResponse<{ '@timestamp': string; [key: string]: unknown }>
>(DETECTION_ENGINE_QUERY_SIGNALS_URL, {
method: 'POST',
body: JSON.stringify(buildAlertsQuery([ecsData._id] ?? [])),
Copy link
Contributor

Choose a reason for hiding this comment

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

is the nullish operator here necessary? The empty array will never be selected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya it's not, will remove.

andrew-goldstein
andrew-goldstein reviewed Feb 2, 2022
View reviewed changes
x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx
@@ -550,7 +613,7 @@ export const sendAlertToTimelineAction = async ({
});
}
} else if (isThresholdRule(ecsData)) {
createThresholdTimeline(ecsData, createTimeline, noteContent, {});
return createThresholdTimeline(ecsData, createTimeline, noteContent, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is an await required in this context, similar to the return await createThresholdTimeline(ecsData, createTimeline, noteContent, { above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's the other way actually, the await is redundant and not needed above. Not sure why the linter did not catch this one, it catches the other

andrew-goldstein
andrew-goldstein reviewed Feb 2, 2022
View reviewed changes
x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx
});
const from = DEFAULT_FROM_MOMENT.toISOString();
const to = DEFAULT_TO_MOMENT.toISOString();
return createTimeline({
Copy link
Contributor

Choose a reason for hiding this comment

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

consider not invoking createTimeline here, because in this state, we can't fully construct the search criteria

andrew-goldstein
andrew-goldstein approved these changes Feb 2, 2022
View reviewed changes
Copy link
Contributor

@andrew-goldstein andrew-goldstein 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 this fix @kqualters-elastic! 🙏
Desk tested locally with threshold, eql, and other rule types
LGTM

YulNaumenko
YulNaumenko reviewed Feb 3, 2022
View reviewed changes
...ublic/detections/components/alerts_table/timeline_actions/investigate_in_timeline_action.tsx Outdated
@@ -22,7 +22,6 @@ interface InvestigateInTimelineActionProps {
alertIds?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to do a proper clean up of alertIds usage. That would be great to clarify in the PR description why we don't need any more fetching the alerts and alertIds.

@YulNaumenko
Copy link
Contributor

YulNaumenko commented Feb 3, 2022 •
edited
Loading

Thank you for fixing this!
Please, add the minimal testing coverage where it is possible. I think it could be some tests for x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/use_investigate_in_timeline.tsx,
x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/investigate_in_timeline_action.tsx, x-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/footer.tsx

@FrankHassanabad FrankHassanabad mentioned this pull request Feb 4, 2022
Merged
cnasikas
cnasikas approved these changes Feb 7, 2022
View reviewed changes
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Cases changes LGTM

YulNaumenko
YulNaumenko approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM on green CI. Great work, thank you! It would be awesome to add the short notice about removing renderInvestigateInTimelineActionComponent in the PR description, just for improvement of maintaining history.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2871 2869 -2

Async chunks

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

id before after diff
cases 294.9KB 294.8KB -103.0B
securitySolution 4.7MB 4.7MB -1.1KB
timelines 226.6KB 226.5KB -147.0B
total -1.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 246.3KB 246.4KB +32.0B

History

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

@kqualters-elastic kqualters-elastic merged commit e312c36 into elastic:main Feb 8, 2022
@kqualters-elastic kqualters-elastic deleted the fetch-ecs-in-actions branch February 8, 2022 11:13
@kibanamachine
Copy link
Contributor

The following labels were identified as gaps in your version labels and will be added automatically:

  • v8.2.0

If any of these should not be on your pull request, please manually remove them.

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.1 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.1:
- Reverts PR 124310 (#124736)
❌ 8.0 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.0:
- [SECURITY] Introduce kibana nav (#68862)

How to fix

Re-run the backport manually:

node scripts/backport --pr 124331

Questions ?

Please refer to the Backport tool documentation

@kqualters-elastic kqualters-elastic mentioned this pull request Feb 8, 2022
Closed
kqualters-elastic added a commit to kqualters-elastic/kibana that referenced this pull request Feb 8, 2022
@kqualters-elastic
[Security Solution] Remove a data fetching hook from the add to timel…
d20de8c 
…ine action component (elastic#124331)

* Fetch alert ecs data in actions.tsx and not a hook in every table row

* Add error handling and tests for theshold timelines

* Fix bad merge

* Remove unused imports

* Actually remove unused file

* Remove usage of alertIds and dead code from cases

* Add basic sanity tests that ensure no extra network calls are being made

* Remove unused operator

* Remove unused imports

* Remove unused mock

(cherry picked from commit e312c36)
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124331 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 9, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124331 or prevent reminders by adding the backport:skip label.

4 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124331 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124331 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124331 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124331 or prevent reminders by adding the backport:skip label.

@kqualters-elastic kqualters-elastic mentioned this pull request Feb 16, 2022
Merged
kqualters-elastic added a commit to kqualters-elastic/kibana that referenced this pull request Feb 16, 2022
@kqualters-elastic
[Security Solution] Remove a data fetching hook from the add to timel…
9e17707 
…ine action component (elastic#124331)

* Fetch alert ecs data in actions.tsx and not a hook in every table row

* Add error handling and tests for theshold timelines

* Fix bad merge

* Remove unused imports

* Actually remove unused file

* Remove usage of alertIds and dead code from cases

* Add basic sanity tests that ensure no extra network calls are being made

* Remove unused operator

* Remove unused imports

* Remove unused mock

(cherry picked from commit e312c36)
@kqualters-elastic kqualters-elastic mentioned this pull request Feb 16, 2022
Merged
kqualters-elastic added a commit to kqualters-elastic/kibana that referenced this pull request Feb 16, 2022
@kqualters-elastic
[Security Solution] Remove a data fetching hook from the add to timel…
1261f21 
…ine action component (elastic#124331)

* Fetch alert ecs data in actions.tsx and not a hook in every table row

* Add error handling and tests for theshold timelines

* Fix bad merge

* Remove unused imports

* Actually remove unused file

* Remove usage of alertIds and dead code from cases

* Add basic sanity tests that ensure no extra network calls are being made

* Remove unused operator

* Remove unused imports

* Remove unused mock

(cherry picked from commit e312c36)

# Conflicts:
#	x-pack/plugins/cases/public/components/case_view/case_view_page.tsx
#	x-pack/plugins/cases/public/components/user_actions/types.ts
#	x-pack/plugins/security_solution/public/cases/pages/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/footer.tsx
kqualters-elastic added a commit that referenced this pull request Feb 16, 2022
@kqualters-elastic
[Security Solution] Remove a data fetching hook from the add to timel…
a42b3f4 
…ine action component (#124331) (#125800)

* Fetch alert ecs data in actions.tsx and not a hook in every table row

* Add error handling and tests for theshold timelines

* Fix bad merge

* Remove unused imports

* Actually remove unused file

* Remove usage of alertIds and dead code from cases

* Add basic sanity tests that ensure no extra network calls are being made

* Remove unused operator

* Remove unused imports

* Remove unused mock

(cherry picked from commit e312c36)
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 16, 2022
kqualters-elastic added a commit that referenced this pull request Feb 16, 2022
@kqualters-elastic
[8.0] [Security Solution] Remove a data fetching hook from the add to…
0dbedb5 
… timeline action component (#124331) (#125810)

* [Security Solution] Remove a data fetching hook from the add to timeline action component (#124331)

* Fetch alert ecs data in actions.tsx and not a hook in every table row

* Add error handling and tests for theshold timelines

* Fix bad merge

* Remove unused imports

* Actually remove unused file

* Remove usage of alertIds and dead code from cases

* Add basic sanity tests that ensure no extra network calls are being made

* Remove unused operator

* Remove unused imports

* Remove unused mock

(cherry picked from commit e312c36)

# Conflicts:
#	x-pack/plugins/cases/public/components/case_view/case_view_page.tsx
#	x-pack/plugins/cases/public/components/user_actions/types.ts
#	x-pack/plugins/security_solution/public/cases/pages/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/footer.tsx

* Fix types

* Fix failing tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelolo24 michaelolo24 michaelolo24 left review comments

@andrew-goldstein andrew-goldstein andrew-goldstein approved these changes

@cnasikas cnasikas cnasikas approved these changes

@YulNaumenko YulNaumenko YulNaumenko approved these changes

Assignees
No one assigned
Labels
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.0.1 v8.1.0 v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Security Solutions] Events viewer (timeline part) is making a excessive REST calls
7 participants
@kqualters-elastic @YulNaumenko @kibana-ci @kibanamachine @andrew-goldstein @cnasikas @michaelolo24