-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Grouping - fix null group missing #155763
Conversation
@@ -132,10 +132,6 @@ const GroupedAlertsTableComponent: React.FC<AlertsTableComponentProps> = (props) | |||
setPageIndex((curr) => curr.map(() => DEFAULT_PAGE_INDEX)); | |||
}, []); | |||
|
|||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an unrelated change, but a follow up from the last PR.
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
packages/kbn-securitysolution-grouping/src/components/accordion_panel/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Have a minor comment about nullGroupMessage
packages/kbn-securitysolution-grouping/src/containers/query/index.ts
Outdated
Show resolved
Hide resolved
@@ -24,13 +22,13 @@ export const mockGroupingProps = { | |||
sum_other_doc_count: 0, | |||
buckets: [ | |||
{ | |||
key: [rule1Name, rule1Desc], | |||
key_as_string: `${rule1Name}|${rule1Desc}`, | |||
key: [host1Name], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to be a host query since its more realistic for testing, rule name will not have a null group.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome!! Thanks @stephmilovic
I wrote a couple of minor suggestions, feel free to apply them.
LGTM! 🚀
...lugins/security_solution/public/detections/components/alerts_table/grouping_settings/mock.ts
Outdated
Show resolved
Hide resolved
packages/kbn-securitysolution-grouping/src/containers/query/helpers.ts
Outdated
Show resolved
Hide resolved
packages/kbn-securitysolution-grouping/src/containers/query/index.ts
Outdated
Show resolved
Hide resolved
packages/kbn-securitysolution-grouping/src/containers/query/index.ts
Outdated
Show resolved
Hide resolved
packages/kbn-securitysolution-grouping/src/containers/query/index.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Sergi Massaneda <[email protected]>
Co-authored-by: Sergi Massaneda <[email protected]>
Co-authored-by: Sergi Massaneda <[email protected]>
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit 1a39471)
…#156115) # Backport This will backport the following commits from `main` to `8.8`: - [[Security Solution] Grouping - fix null group missing (#155763)](#155763) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Steph Milovic","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-04-27T20:29:34Z","message":"[Security Solution] Grouping - fix null group missing (#155763)","sha":"1a39471214af322cdd66713a8eaae6494b651830","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Threat Hunting","Team: SecuritySolution","Team:Threat Hunting:Explore","v8.8.0","Feature:Alerts Grouping","v8.9.0"],"number":155763,"url":"https://github.com/elastic/kibana/pull/155763","mergeCommit":{"message":"[Security Solution] Grouping - fix null group missing (#155763)","sha":"1a39471214af322cdd66713a8eaae6494b651830"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/155763","number":155763,"mergeCommit":{"message":"[Security Solution] Grouping - fix null group missing (#155763)","sha":"1a39471214af322cdd66713a8eaae6494b651830"}}]}] BACKPORT--> Co-authored-by: Steph Milovic <[email protected]>
Adjusts the grouping query to include a "missing" group in the
groupByFields
bucket results. The "missing" field in the terms aggregation needs to match the field's es type. A helper function,getFieldTypeMissingValue
, assigns this value according to the es type. In order to avoid leaving out events that are equivalent with the default value, we are checking 2 default values. This looks like:Once we get the response, we use
parseGroupingQuery
. For each group, it checks if the elements of thekey
property are equal. If they are, it sets thekey
/key_as_string
properties to an array with the first element ofkey
, the grouping field, and sets theisNullGroup
property to false. If they are not equal, it sets thekey
to-
, and isNullGroup to true. This is so that we have a flag in the UI for the "missing" group.This is what it looks like in the UI:
user.name
source.ip