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

[SecuritySolution] Use correct queries and filters for prevalence calls #154544

Conversation

janmonschke
Copy link
Contributor

@janmonschke janmonschke commented Apr 6, 2023

Summary

Bug ticket #131967 describes an issue where the alert prevalence count is not correct for fields that have array values (such as process.args).

Solution

Getting the correct count for those fields involved adding more term conditions to the prevalence query and the timeline filter. This ensures that only alerts with the exact same array values match instead of partial matches as before.

Screen.Recording.2023-04-12.at.10.11.35.mov

Checklist

Comment on lines +99 to +111
// For fields with multiple values we need add an extra filter that makes sure
// that only fields that match ALL the values are queried later on.
let filters: Filter[] = [];
if (arrayValues.length > 1) {
filters = [
{
meta: {},
query: {
bool: {
must: arrayValues.map((value) => ({ term: { [field]: value } })),
},
},
},
];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure this logic should apply in cases other than alert prevalence. But I might just leave this here instead of in PrevalenceCell so that in future this logic can be used in other places.

@janmonschke janmonschke force-pushed the fix-prevalence-counts-for-fields-with-multiple-values branch from cb53239 to 1c401fa Compare April 6, 2023 13:00
@janmonschke janmonschke self-assigned this Apr 12, 2023
@janmonschke janmonschke added Team:Threat Hunting:Investigations Security Solution Investigations Team release_note:fix backport:prev-minor Backport to the previous minor version (i.e. one version back from main) v8.8.0 labels Apr 12, 2023
@janmonschke janmonschke marked this pull request as ready for review April 12, 2023 08:15
@janmonschke janmonschke requested review from a team as code owners April 12, 2023 08:15
@janmonschke janmonschke requested a review from a team as a code owner April 12, 2023 09:53
@janmonschke
Copy link
Contributor Author

@elasticmachine merge upstream

@janmonschke
Copy link
Contributor Author

@elasticmachine merge upstream

@janmonschke
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

LGTM

Left some minor/nit comments :)

@janmonschke
Copy link
Contributor Author

@elasticmachine merge upstream

@janmonschke janmonschke enabled auto-merge (squash) April 19, 2023 14:38
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #23 / analytics instrumented events from the browser Event "viewport_resize" should emit a "viewport_resize" event when the browser is resized

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 16.0MB 16.0MB +16.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 394 397 +3

Total ESLint disabled count

id before after diff
securitySolution 474 477 +3

History

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

cc @janmonschke

@janmonschke janmonschke merged commit c3c55e7 into elastic:main Apr 19, 2023
19 checks passed
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.7 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 154544

Questions ?

Please refer to the Backport tool documentation

nikitaindik pushed a commit to nikitaindik/kibana that referenced this pull request Apr 25, 2023
…ls (elastic#154544)

## Summary

Bug ticket elastic#131967 describes an
issue where the alert prevalence count is not correct for fields that
have array values (such as `process.args`).

## Solution

Getting the correct count for those fields involved adding more `term`
conditions to the prevalence query and the timeline filter. This ensures
that only alerts with the *exact* same array values match instead of
partial matches as before.




https://user-images.githubusercontent.com/68591/231395154-b5a1c968-8308-49fb-a218-f3611f8331c3.mov


### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Get approval from the product team

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 21, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 154544 locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport missing Added to PRs automatically when the are determined to be missing a backport. backport:prev-minor Backport to the previous minor version (i.e. one version back from main) release_note:fix Team:Threat Hunting:Investigations Security Solution Investigations Team v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants