-
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 count bug #156206
Conversation
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
@@ -106,9 +99,6 @@ const GroupingComponent = <T,>({ | |||
const nullGroupMessage = isNullGroup | |||
? NULL_GROUP(selectedGroup, unit(groupBucket.doc_count)) | |||
: undefined; | |||
if (isNullGroup) { | |||
setNullCount({ unit: groupBucket.doc_count, group: 1 }); |
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 was the dumb thing 🙄
groupByFields: { buckets: groupByFields }, | ||
groupsCount: { | ||
value: | ||
(aggs.unitsCount?.value !== aggs.unitsCountWithoutNull?.value |
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.
if these are not equal, there is one extra group to add to the count
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! Works as expected after fix
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
(cherry picked from commit 7a0620f)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.8`: - [[Security solution] Grouping count bug (#156206)](#156206) <!--- 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-05-01T21:58:23Z","message":"[Security solution] Grouping count bug (#156206)","sha":"7a0620f132fe34c03c5ffe70cc118d52b5ec4dbd","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":156206,"url":"https://github.com/elastic/kibana/pull/156206","mergeCommit":{"message":"[Security solution] Grouping count bug (#156206)","sha":"7a0620f132fe34c03c5ffe70cc118d52b5ec4dbd"}},"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/156206","number":156206,"mergeCommit":{"message":"[Security solution] Grouping count bug (#156206)","sha":"7a0620f132fe34c03c5ffe70cc118d52b5ec4dbd"}}]}] BACKPORT--> Co-authored-by: Steph Milovic <[email protected]>
Summary
This PR fixes 2 bugs:
justifyContent="center"
was added to the Host name group renderer, but it shouldn't be there. Removed it to fix stylingunitsCount
andgroupsCount
) from security solution into the grouping package, I think it should have been there in the first place since we seek these values ingrouping.tsx
.missing
property to theunitsCount
value_count
aggregation. Unlike theterms
agg, this does not overwrite the values of fields named the same value as our missing value (the count returned is accurate). I could not do the same with thegroupsCount
cardinality aggregation. So instead, I added aunitsCountWithoutNull
aggregation. If there is a null group,unitsCount !== unitsCountWithoutNull
. When this condition is true, I added1
to thegroupsCount
value.Before
bad.mov
After
good.mov