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

[Security Solution][Detections][Threshold Rules][7.12] Threshold summary view #94345

Merged
merged 7 commits into from
Mar 21, 2021

Conversation

madirey
Copy link
Contributor

@madirey madirey commented Mar 10, 2021

Summary

Users have complained that matching threshold fields were removed from the generated synthetic signals (here is one example: https://discuss.elastic.co/t/threshold-detection-ignoring-group-by-field/264873/5). The fields were removed in 7.11 for 2 reasons: 1) we attempted to add fields from the rule's KQL query to the signal, but the query can contain wildcards. Thus you could end up with a set of matching events with cardinality(field) > 1, which makes it unclear which value to add to the synthetic signal; and 2) a user could group by signal.rule.building_block_type and generate a signal that differs semantically from the intended behavior.

In this PR, I've attempted to address those concerns in a different way.

  1. Adds threshold fields to summary view. Marks "generated" threshold fields as [threshold] for clarity. Some fields may be repeated if they were already in the summary view (host.name for example).
    image

  2. Adds back threshold fields to the synthetic signal, and thus back to the alerts table view. We don't attempt to add fields from the query, as this can be difficult to ascertain and also can contain wildcards which results in a set of values with cardinality > 1. Fields starting with signal. are excluded, so that a user can't override functionality with signal.rule.building_block_type for example.
    image

Checklist

For maintainers

@madirey madirey added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Detection Alerts Security Solution Detection Alerts Feature v7.12 labels Mar 10, 2021
@madirey madirey requested a review from a team as a code owner March 10, 2021 18:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@madirey madirey added v7.12.0 and removed v7.12 labels Mar 10, 2021
@madirey madirey requested a review from yctercero March 10, 2021 18:56
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Just a random idea regarding UX (if I'm getting it correctly): in this PR you take some data specific to signals generated by threshold rules, take it from a signal, map it to a bunch of artificial fields and show in this summary table. As an idea, what do you think about showing these rule-specific parameters in a separate view, let's say right below the generic summary view?

Signal fields
=====
field | value
field | value
...

Threshold stuff
=====
whatever view would be the best for this data

@madirey madirey added v7.12.1 and removed v7.12.0 labels Mar 11, 2021
@kibanamachine
Copy link
Contributor

💚 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 7.8MB 7.8MB +1.8KB

History

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

Copy link
Contributor

@banderror banderror 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 addressing my silly comments :) It looks good to me 👍
I will need more time to debug threshold rule execution and this summary view to really connect all the dots. So it would be great if someone else could look into it, give more meaningful feedback than me and give an additional approval.

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

This is gonna be a big ➕ for users! This LGTM. There are a few things I saw that can be followed up on as it's more UX and may be worth getting some feedback from @dontcallmesherryli . I like @banderror 's suggestions too.

A few notes:

  • when query does not include group by fields, it still includes the threshold value which could be useful to include in the summary.
  • May be worth adding tooltips to the threshold fields so someone can look at the summary and know exactly what it means (like that the fields listed are the group by fields)
  • It looks like on hover of the threshold field values, it's giving the field, not the value
    image

@madirey madirey merged commit c0f9bfc into elastic:master Mar 21, 2021
@madirey madirey deleted the threshold-summary branch March 21, 2021 14:02
madirey added a commit to madirey/kibana that referenced this pull request Mar 21, 2021
…ary view (elastic#94345)

* Add threshold summary view items

* Add threshold field desgination

* Add threshold fields to signal doc

* Fix unit test

* Handle error
madirey added a commit to madirey/kibana that referenced this pull request Mar 21, 2021
…ary view (elastic#94345)

* Add threshold summary view items

* Add threshold field desgination

* Add threshold fields to signal doc

* Fix unit test

* Handle error
madirey added a commit that referenced this pull request Mar 21, 2021
…ary view (#94345) (#95026)

* Add threshold summary view items

* Add threshold field desgination

* Add threshold fields to signal doc

* Fix unit test

* Handle error
madirey added a commit that referenced this pull request Mar 21, 2021
…ary view (#94345) (#95025)

* Add threshold summary view items

* Add threshold field desgination

* Add threshold fields to signal doc

* Fix unit test

* Handle error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Alerts Security Solution Detection Alerts Feature release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.12.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants