Add common dynamic mappings and properties from ECS automatically#1073
Add common dynamic mappings and properties from ECS automatically#1073mrodm merged 19 commits intoelastic:mainfrom
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
🌐 Coverage report
|
jsoriano
left a comment
There was a problem hiding this comment.
Looks good, but added a couple of comments that may help to simplify this feature.
Ideally we will also need to take these mappings into account when validating field definitions on tests. Tests will start failing if developers remove their ECS definitions after enabling this import, although the final template will be correct.
But let's think about this in a followup. Current workaround would be to keep definitions required by tests.
| "@timestamp": { | ||
| "type": "date", | ||
| "ignore_malformed": false | ||
| } |
There was a problem hiding this comment.
Was there a strong reason to keep this field as an static property?
This single field forces us to maintain the special case of having also static properties defined here, requiring for example the additional meta field and so on. Would it be possible to define this as a dynamic template instead? It would be something like this:
"dynamic_templates": [
{
"ecs_timestamp": {
"path_match": "@timestamp",
"mapping": {
"type": "date",
"ignore_malformed": false
},
},
And we could think on this feature as something like adds only dynamic templates. Also in the context of the spec change.
There was a problem hiding this comment.
I used the same json that was built here https://gist.github.com/P1llus/e0de7b3a7824a41a29660e253c6cce6b
I think it could be changed to be a dynamic template as you mentioned. @P1llus do you think in any inconvenient if it is changed ?
I would say there is no problem and it would simplify the code to just manage dynamic templates.
There was a problem hiding this comment.
For now I've moved that property to a dynamic template as suggested by @jsoriano.
If there is any inconvenient because of that, it could be re-introduced.
Currently, it has been followed the approach to add a prefix (_embedded_ecs.*) to all the names of the dynamic templates in the static file, so they can be identified easily. If properties are needed too, it should be added another way (using _meta field and lists as it was in a previous change?).
Review errors raised and their messages
go.mod
Outdated
| sigs.k8s.io/yaml v1.3.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/elastic/package-spec/v2 => github.com/mrodm/package-spec/v2 v2.0.0-20221220145202-70d4563fbf33 |
There was a problem hiding this comment.
to be removed before merging
|
/test |
hop-dev
left a comment
There was a problem hiding this comment.
I added one question but looks good to me, I like the new style of prefixing the name
|
/test |
In kibana versions < 8.6.0 the dot in the names caused that packages were not installed. It was interpreted that the string after the dot was an inner element of the map, causing validation errors.
633f842 to
8290c65
Compare
|
I've checked building and installing a test package in different versions of Kibana. In kibana 8.6.0-SNASPSHOT there was no issue. But in versions < 8.6.0 , the dot in the dynamic template names is causing this error:
I've updated how the name of each dynamic template is generated to replace the dot "." by a dash "-" @jsoriano @hop-dev Thereby, the dynamic template generated would be like: elasticsearch:
index_template:
mappings:
dynamic_templates:
- _embedded_ecs-ecs_timestamp:
mapping:
ignore_malformed: false
type: date
path_match: '@timestamp'
- _embedded_ecs-data_stream_to_constant:
mapping:
type: constant_keyword
path_match: data_stream.*
- _embedded_ecs-resolved_ip_to_ip:
mapping:
type: ip
match: resolved_ipTested with the following Kibana versions 7.17.8-SNAPSHOT, 8.0.0-SNAPSHOT, 8.5.0-SNAPSHOT and 8.6.0-SNAPSHOT |
Relates #1018
This PR adds support to include some dynamic templates and properties from ECS that are commonly used and can be a source of conflicts if they don't have any mapping.
These mappings are added if packages enable the flag
import_mappingsin_dev/build/build.yml. This flag is introduced in elastic/package-spec#455 , and it would be available in a future version 2.3.0 of the spec.Option 1:
All the dynamic templates names are prefixed with
_embedded_ecs, so they can be distinguished easily by Fleet in case it is needed. For instance:Option 2 (Discarded for now):
All the mappings added by this flag are added into the
_metafield. Thereby, they are marked in case it is needed to know which ones were automatically added. Properties and dynamic templates are added toecs_properties_addedandecs_dynamic_templates_addedkeys, respectively.Example: