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] Fix topN grouping when field type is boolean #131958

Merged
merged 10 commits into from
May 16, 2022
Merged

[Security Solution] Fix topN grouping when field type is boolean #131958

merged 10 commits into from
May 16, 2022

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented May 10, 2022 •
edited
Loading

Summary

#131216

Before:
The legend displays 0, 1 when field type is boolean.

field.value.mp4

After:
The legend displays true, false when field type is boolean.

topN.mp4

Steps to verify:

  1. Create a rule and generate some alerts.
  2. Find the alerts table, click on fields, click Create field and create a runtime field for testing.
  3. Name process_interactive, type Boolean, Set value: emit(true)
  4. Close the modal, hover onto any process.interactive cell, expand the hover actions and click Show top values
  5. The legend should display true as group name instead of number.
  6. Check the topN in timeline and alerts table as well.
  7. Check topN for other field and make sure they display correctly as well.

Checklist

Delete any items that are not applicable to this PR.

@angorayc angorayc changed the title fix grouping [Security Solution] Fix topN grouping when field type is boolean May 11, 2022
@angorayc angorayc added bug Fixes for quality problems that affect the customer experience Team:Threat Hunting Security Solution Threat Hunting Team Team:Threat Hunting:Explore v8.3.0 v8.2.0 auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes labels May 11, 2022
@angorayc angorayc marked this pull request as ready for review May 11, 2022 17:03
@angorayc angorayc requested review from a team as code owners May 11, 2022 17:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

machadoum
machadoum approved these changes May 12, 2022
View reviewed changes
Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

LGTM!
I tested it locally and couldn't find any issue.

Thank you Angela!

@angorayc
Copy link
Contributor Author

@elasticmachine merge upstream

michaelolo24
michaelolo24 reviewed May 16, 2022
View reviewed changes
...curity_solution/server/search_strategy/security_solution/factory/matrix_histogram/helpers.ts Outdated
@@ -18,7 +18,8 @@ export const getGenericData = <T>(
): MatrixHistogramData[] => {
let result: MatrixHistogramData[] = [];
data.forEach((bucketData: unknown) => {
const group = get('key', bucketData);
// if key is not a string, use key_as_string instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this if key_as_string is present use it, else default to the existing key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I mean. Updated the wording as well, thank you!

michaelolo24
michaelolo24 approved these changes May 16, 2022
View reviewed changes
Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

Just one comment about the wording of a comment in the helpers file . Tested locally and looks good. Thanks!

YulNaumenko
YulNaumenko approved these changes May 16, 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

@angorayc angorayc enabled auto-merge (squash) May 16, 2022 17:10
@kibana-ci
Copy link
Collaborator

💚 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 +105.0B

History

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

@angorayc angorayc merged commit 3f5197a into elastic:main May 16, 2022
kibanamachine pushed a commit that referenced this pull request May 16, 2022
@angorayc
[Security Solution] Fix topN grouping when field type is boolean (#13…
2b5a628 
…1958)

* fix grouping

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* fix grouping

* unit tests

* fix types

* update comment

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 3f5197a)
@kibanamachine kibanamachine mentioned this pull request May 16, 2022
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.2

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request May 16, 2022
@kibanamachine @angorayc
[Security Solution] Fix topN grouping when field type is boolean (#13…
3165cb4 
…1958) (#132273)

* fix grouping

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* fix grouping

* unit tests

* fix types

* update comment

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 3f5197a)

Co-authored-by: Angela Chuang <6295984+angorayc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@michaelolo24 michaelolo24 michaelolo24 approved these changes

@machadoum machadoum machadoum approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.2.0 v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@angorayc @elasticmachine @kibana-ci @kibanamachine @machadoum @michaelolo24 @YulNaumenko