-
Notifications
You must be signed in to change notification settings - Fork 427
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
Add event.reason field #907
Conversation
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.
Looking good so far. I agree with adding this as extended.
schemas/event.yml
Outdated
- name: reason | ||
level: extended | ||
type: keyword | ||
short: The reason captured by the event |
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.
Perhaps we should phrase this more naturally. E.g.
"Reason why this event happened, according to the source"
schemas/event.yml
Outdated
multi_fields: | ||
- type: text | ||
name: text |
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.
Do we expect event.reason
to be complex often enough to warrant this? Not opposed to this, just wondering out loud :-)
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 had a similar internal debate...
I can see value in having the text
indexing available since event.reason
could possibly be longer string (e.g. something like this Okta event), but the extra complexity may not be warranted the majority of the time.
As I type this out, I'm leaning towards removing the multi-field. If a user was trying to search for allowed events across sources using event.reason.text:allow
, using the categorization fields like `event.type:"allowed" would be the better practice to encourage I think.
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.
Let's remove it for now. Adding .text
later is non-breaking, so it'll be fine if it becomes necessary.
Also, I think this will be one of the fields we move to wildcard
ultimately. I'm sure we'll get to wildcard
before there's a need to add the .text
interim solution.
CI fails because you're missing a |
@webmat should be good to merge if you wanna give it once last 👀 |
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
Summary
Adds a new field under
event.*
,event.reason
, which provides a location to capture the reason why an event occurred.Example usage
event.action:"request-denied"
describes the action captured and why the action took place is captured inevent.reason:"Blocked site: example.com"
event.action:"process-terminated" with reason describing why as
event.reason:"Terminated an unexpected process"`Discussion Points
Two items I wanted to call to attention:
event.reason
is currently defined astype: extended
. This felt most appropriate starting off.text
multi-field is included.event.reason
could possibly hold a longer string (e.g.File Sharing site URL blocked
).Closes #613