-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Adds Rule Execution Log table #124198
Changes from all commits
d8ddc0b
c336868
2187823
9de216d
729b272
defe453
1fec617
bd96ce4
de5ec8d
a675d98
5a6d237
eb66e1a
90c1ef6
5a736ab
cdb317b
825b103
b5e50a5
404b2bb
0ae6f0d
b492e77
d703131
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -292,6 +292,12 @@ | |
"number_of_triggered_actions": { | ||
"type": "long" | ||
}, | ||
"total_alerts": { | ||
"type": "long" | ||
}, | ||
"total_hits": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "hits" isn't a general alerting thing, so I think we'll need a comment somewhere describing what it is - perhaps in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If "hits" isn't a general alerting thing, I don't think it should be part of the event-log. How do the "hits" differ from the "alerts"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see these two issues for details #120678 #120668, but gist here is I totally agree about naming (perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, interesting. We'll have an issue open soon-ish (from some previous research) about having a maximum number of actions and alerts (separate values) per rule execution. And we would want to track the total number as being different if we cap the number via the maximums. So it isn't a thing now, but probably will be. So ... maybe just some word-smithing on this. Candidate doesn't seem quite right. Something like "total" as the number the rule produced, and "triggered" instead of "candidate", so "totalAlerts" and "triggeredAlerts"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We already have a few metrics stored in the event log which are not a general alerting thing (strictly speaking) but which make sense from the Security Solution and AAD perspective. These are I agree that additional documentation and/or better naming would help though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation, @spong, that makes a lot of sense to me. What about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Those two sound good to me @kobelb! 🙂 I'll wait a moment to see if anyone else has additional input and will then make the change. In addition to adding documentation within There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any docs about the fields in the event-log or any docs that recommend that users search these indices. We've treated them as a traditional hidden index where users might find it, but they can't rely on the names of fields. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pulled this feature out to a dedicated PR (#126210), will ping reviewers when ready for review. |
||
"type": "long" | ||
}, | ||
"total_indexing_duration_ms": { | ||
"type": "long" | ||
}, | ||
|
@@ -347,4 +353,4 @@ | |
} | ||
} | ||
} | ||
} | ||
} |
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.
Saw this was just added in #123567, shall we add it to the table as well, or is it getting too congested and better to wait till we allow expanding rows with additional data?