Remove migrated not-ECS migrated fields#327
Conversation
| type: group | ||
| description: > | ||
| Client stats. | ||
| description: | |
There was a problem hiding this comment.
Not related directly to this PR but something we should discuss for the future: We have these group descriptions for historical reasons but not 100% sure if these still will make sense in the future. Especially as these are for docs only and were always a challenge to visualise.
There was a problem hiding this comment.
I didn't know about this. Any opinion on this @exekias ?
There was a problem hiding this comment.
I'm definitely ok with removing something we don't really show in the docs (or somewhere else)
There was a problem hiding this comment.
I've updated the PR, removed all descriptions as you suggested.
| type: alias | ||
| path: message | ||
| migration: true | ||
| - name: level |
There was a problem hiding this comment.
@mtojek I think I don't fully understand the change. Why is thread_id removed but not level here?
There was a problem hiding this comment.
As the PR title suggests I removed all fields marked with "migration" which are not ECS fields. In the other PR, you asked for removing it: #323 (comment)
If I should remove all fields with migration: true, don't we need ecs.yml in every fields folder? As we don't have it there, I left ECS fileds in place.
There was a problem hiding this comment.
We need to remove all migration fields, also the ones that point to ECS fields.
We will need a subset of the ecs.yml field in most datasets, but not all of ecs fields. We only need the fields from ECS that are actually used by the dataset. For example if log.level is used, it needs to be in, but if we don't use source.ip, it shouldn't be.
There was a problem hiding this comment.
I've updated the PR.
Used ECS fields are stored in dedicated ecs.yml. Also, all used fields are in corresponding README files.
ruflin
left a comment
There was a problem hiding this comment.
Did you by chance remove for descriptions then you wanted? Only the group ones should be removed.
For the code related to migration fields from ECS. This only works for modules where we actually migrated fields to ECS and did not use ECS from day one. Newer modules have used ECS from day one so you can't find the ECS fields based on a migration flag. But I must also confess I didn't fully deep dive into the code itself, so perhaps this is already in there.
| description: > | ||
| Name of the group. | ||
| - name: terminal | ||
| type: keyword |
There was a problem hiding this comment.
The description is now removed here and this is not a group. Not sure if this is on purpose?
There was a problem hiding this comment.
Did you by chance remove for descriptions then you wanted? Only the group ones should be removed.
Misunderstanding. I corrected the source code. Only group fields have stripped description now.
This only works for modules where we actually migrated fields to ECS and did not use ECS from day one. Newer modules have used ECS from day one so you can't find the ECS fields based on a migration flag.
I adjusted the PR:
- If the field is an alias and has
migration: true, then skip it. - If the field is an alias and the path is same as ECS, then skip it.
| path: source.geo.region_iso_code | ||
| migration: true | ||
| - name: error | ||
| - name: geoip |
There was a problem hiding this comment.
This is interesting. We have a group with no entries. I wonder what EPM does with it 🤔 @skh @neptunian
ruflin
left a comment
There was a problem hiding this comment.
Lets get this in. As soon as we have the full mysql package and probably 1-2 others in, we will see if we need some more adjustements.
This PR addresses comment reported in https://github.com/elastic/package-registry/pull/323/files/b4ffdfb6aafe5a70f2ff8ee6ed60c9788dd7f1cb#r402212487
All fields files have been parsed and re-marshalled.
Difference can be also spotted in MySQL README file.