-
Notifications
You must be signed in to change notification settings - Fork 431
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
Introduce the initial values for ECS categorization #692
Conversation
This comment has been minimized.
This comment has been minimized.
…tegorization section
bc4858c
to
3db941d
Compare
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.
It looks like I'm self-reviewing here, which may appear strange :-)
This PR is the culmination of the work of many people (thank you all ❤️), and I'm simply the "messenger", in that I'm the one making it happen in the ECS repo. This self-review is therefore intended as discussion with the collaborators on the categorization project, and of course with anyone else who wants to provide feedback on this.
Overall I'm good with this initial release.
But there's one aspect I'd like to settle first, that I see as a blocker:
- I think we should flag the "Categorization" section as beta and still subject to change. (example)
- I think it should explicitly state that some values for some fields are still missing and being worked on, and that:
- more allowed values will be released over time
- people can take a look at this public feedback document (https://ela.st/ecs-categories-draft) to get a glimpse at what else is coming, and provide feedback.
The mechanics described in the points above is already how we plan to approach the follow-up to this initial release. But I think it's specifically important that the documentation itself mentions this. The documentation is what the majority of users and implementers will see. The issues, PRs and so on are known by only a very small subset of our users.
I've also entered a few PR comments to discuss minor adjustments. None of them are blockers if we agree on the points above on marking this as beta and/or having explanation in the docs of this gradual release process.
What are your thoughts?
Of course I need official thumbs up on the PR as well. No matter how many people contributed to this, the PR is the official record of whether people agree with going forward with this initial release of the categorization fields.
schemas/event.yml
Outdated
description: > | ||
The signal value is used by Elastic Kibana apps, such as SIEM, for app-specific purposes. | ||
`event.kind:signal` is thus reserved and should not be used for the ingestion | ||
of events into Elasticsearch. |
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.
Even if users should not be populating this value in their pipelines, I think we should explain what this value actually means. This description doesn't do that.
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.
Agreed, and we should communicate all reserved values similarly.
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.
OK, How's this text?
This value is similar to metric, except that the entity being measured does not | ||
provide a numeric metric value, but rather one of a fixed set of conditions or states. | ||
For example a periodic event reporting a "fin_wait" state of a TCP connection | ||
on a host might use `event.type:state`. |
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 wonder if we should hold off on releasing this one until we've confirmed the semantics a little more. Here are two reservations I still have, at this time:
The state that can be observed at one moment in time (the cluster is green). The state transitions (e.g. cluster goes from yellow to green). This definition should address whether both kinds of events belong in event.kind:state. My understanding is that only "point in time" observations, and not the state transitions should belong here. I'm not sure if we have agreement on that yet.
Another worry I have is that in many cases, checking on the state of a system can actually report back metrics. So in these cases, these state checks would be event.kind:metric
because they contain metrics. If that's the case, how useful/reliable will event.kind:state
be? Perhaps it's better to have all state checks in event kind metric?
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 there's not clarity in what this category is used for, I'd tend to agree. But isn't there agreement on what this field is used for? If the answer is "no" we should strip it out, but if others have been thinking of this the same way I don't see a need to remove it from the initial release.
As you state, the language seems to indicate any sort of data state falls under data involving "one of a fixed set of conditions or states." I'm not sure that there's really a meaningful distinction between "heartbeat" style periodic events or events that have the same exact data, but are only fired on state transitions--so I would imagine that this would cover either use-case.
Just for illustration purposes, what I've been thinking of using this category for is for any sort of Endpoint internal state transitions--i.e. policy applications, upgrades, lifecycle, etc.--so besides the surrounding metadata of things like versions, categorization fields, and host-level information, I envision that implementations that would use this category similarly to how I envisioned would contain a lot of hybrid data, with much "application-specific state" in custom fields. That's how I see this differ from the idea of "metrics."
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.
Now that we've agreed on adding a beta warning to the section, I'm less worried here.
Let's leave it in for now and see if we take it out later.
Since we have the beta warning on the section, we'll be able to keep clarifying and improving the definitions and the wording over the next little while, and backport to the 1.4 docs (although additional allowed values would be in subsequent releases). I'd like to request a final review, make sure there's no blocker left. |
Still not perfect, as they're all still self-referential. But at least the parens are gone.
I'll reword the warnings on each of the 4 allowed values pages. Right now it's contradictory with the warning at the top of the whole section. Currently, the gist is Top of section: "these values may change in the future" On each page: "warning if you don't use these values bad things will happen" I'll reword the warning on each page to the effect of Proposed warning for each page: "warning, once we remove the beta label, if you don't use these values bad things will happen" So not a big change, just a nod to the effect that this is still being worked on and may still change. |
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.
Latest changes looking great!
This PR LGTM 👍
Not a blocker, but I think we need to adjust the definition of event.action based on these new categorization fields.
CURRENT:
This describes the information in the event. It is more specific than
event.category
. Examples are group-add
, process-started
,
file-created
. The value is normally defined by the implementer.
example: user-password-change
PROPOSED:
This describes detailed information about the action described in the event. It is more specific than
event.category
and event.type
and is defined by the implementer. A common example of the use of these three fields is event.category:process AND event.type:end AND event.action:abend
to indicate a process that has ended abnormally.
Yeah I'll start collecting this feedback today, and perhaps even start a follow-up PR |
This pull request introduces the first set of official values for the 4 ECS categorization fields, which were reserved up to this point (event.kind, event.category, event.type, event.outcome).
Note that more values are currently being considered for some of these fields. Users and implementers can preview the other values and offer feedback on them in this public document: https://ela.st/ecs-categories-draft
TODO
Preview