Add ecs.version to each event #9284
Conversation
|
@urso For awareness |
|
I'm not entirely convinced we should populate this field automatically for all Beats. I agree the field definition should be global. But the value should be populated (or not) per Beat. It needs to be an explicit statement made per Beat, with no default value. This ensure that someone made that decision to update the value, and presumably made sure that the change was accurate. |
|
It does set the field for all Beats. Do you see a problem with that? |
ruflin
force-pushed
the
ecs-version
branch
2 times, most recently
from
e7831c2 to
1b13e22
Compare
Yes, I'd rather have each Beat have the duty to define this explicitly. This would represent more of a conscious statement that a given Beat actually is compliant to, or following a specific version of ECS. Setting it as a default globally incurs the risk of just slipping in -- below the radar -- and not being reviewed consistently. If we follow the approach I'm proposing, I'd review any PR setting or changing the value of a Beat for Right now, if we set this field globally, there's nothing to "reject", if a given Beat is doing things that don't conform to ECS. |
|
I don't think it will make a difference from if the ecs.version field is there or not. A Beat can't conflict with ECS definitions as each beat has the ECS fields defined and it's checked in CI that new fields do not conflict with these. Even if we add a field |
There was a problem hiding this comment.
I'm not worried about additional fields. They are indeed allowed, of course.
What I'm worried about is the more subtle ways not to follow ECS. Examples of this would be: using reserved fields, using ECS fields incorrectly, not following guidelines to do some data duplication (e.g. related.*, src/dst => cli/srv).
I get the feeling we're going to get bit by the fact that none of the Beats makes a conscious and specific statement that they do follow a certain version. This is a thing I would wish to have not only now, for the introduction of ECS, but also in the future, with every subsequent ECS release.
Perhaps I'm imagining a problem where there isn't one, though. If you think this is fine, we can move forward and refine this later, if we feel the need.
|
I think there are 2 things here: Following recommendations and following ECS. Following ECS for me is having the fields and not conflicting with it. Copies to related etc. are good to have but if not done does not mean it is not using ECS. If we follow the logic that an event can only have ecs.version set if it follows 100% of the recommendation of ECS, this field would never be used. For me this field means: There are not conflicts with ECS and ECS is used in as many place as possible, but there might be additional fields. This is based on our Guidelines here: https://github.com/elastic/ecs#implementing-ecs |
|
@ruflin Yeah I'm good to proceed 👍 |
ruflin
force-pushed
the
ecs-version
branch
2 times, most recently
from
7439924 to
1bf9874
Compare
ruflin
force-pushed
the
ecs-version
branch
2 times, most recently
from
730f539 to
3f6c281
Compare
This indicates with each event which ECS version is used. Currently it is all based on ecs 1.0.0-beta1. We must make sure to keep this value up-to-date. Removes event.version as does not exist anymore.
| "event.dataset": "eve", | ||
| "event.module": "suricata", | ||
| "event.type": "alert", | ||
| "http.request.method": "GET", | ||
| "http.response.status_code": "200", | ||
| "input.type": "log", | ||
| "log.offset": 7630, | ||
| "source.ip": "192.168.1.146", | ||
| "source.port": 37742, | ||
| "source.ip": "192.168.1.146", |
There was a problem hiding this comment.
@ycombinator Interesting, this now also happens in my PRs that spaces are added?!?
| @@ -88,6 +88,9 @@ func Load( | |||
| "host": common.MapStr{ | |||
| "name": name, | |||
| }, | |||
| "ecs": common.MapStr{ | |||
| "version": "1.0.0-beta2", | |||
There was a problem hiding this comment.
Just a thought, if we vendor the generated ECS Go package we could rely on that package’s constant (ecs.Version) for keep this value in sync. If you update the fields data you should also be updating the vendored ecs package.
There was a problem hiding this comment.
Interesting idea. To take this one step further would be nice to have a fields.go as part of this package that we could use too so no need to modify the ecs-fields.yml. This would assume that the global fields.yml is generated by export fields instead of collection in the future.
This indicates with each event which ECS version is used. Currently it is all based on ecs 1.0.0-beta1. We must make sure to keep this value up-to-date.
Removes event.version as does not exist anymore.