-
Notifications
You must be signed in to change notification settings - Fork 116
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 common dynamic mappings and properties from ECS automatically #1073
Conversation
🌐 Coverage report
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -160,3 +160,5 @@ require ( | |||
sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be removed before merging
/test |
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 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_ip Tested 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_mappings
in_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
_meta
field. Thereby, they are marked in case it is needed to know which ones were automatically added. Properties and dynamic templates are added toecs_properties_added
andecs_dynamic_templates_added
keys, respectively.Example: