-
Notifications
You must be signed in to change notification settings - Fork 25.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[CI] Adding continuous testing for ECS dynamic templates #97901
Conversation
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
Pinging @elastic/es-delivery (Team:Delivery) |
Pinging @elastic/es-data-management (Team:Data Management) |
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
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.
@felixbarny @ruflin see if you agree with my comment about not failing if we create multi-field mapping even for fields of which ECS definition does not enforce such.
For example, we define a multi-field mapping for the *.name pattern. If you look into the ECS definitions, you will find that many *.name have multi-field mappings, while many others don't. Trying to be very accurate will result with many more dynamic templates.
Seems like erring on the side of always creating a match_only_text
subfield, even for cases where ECS only defines a keyword
field for *.name
fields.
I think that's fine. While on the one hand, this creates a bit more indexing overhead than strictly required, it brings a nice consistency via the naming convention *.name
so that users know they can always do a full text search on fields ending with *.name
.
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
x-pack/plugin/core/template-resources/src/main/resources/ecs-dynamic-mappings.json
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Show resolved
Hide resolved
I'm not a fan of the ECS multi fields but I agree we should be consistent. Do we have an understand on how much the overhead on storage is? I guess hard to tell? Assuming a user would want to disable this, I assume they could overwrite it in |
I can't say anything clever about the actual overhead, only that since the default mapping for strings in Elasticsearch is multi-field, then I assume it is at least acceptable. We are only extending ECS a bit here 🙂
Exactly! And since we added |
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
.../plugin/stack/src/javaRestTest/java/org/elasticsearch/xpack/stack/EcsDynamicTemplatesIT.java
Outdated
Show resolved
Hide resolved
@rjernst @mark-vieira would you be able to review or assign someone to review this PR? |
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 think the current review comments covers anything I would have added already. I see there was the discussion around multi-fields for .name, which was indeed something annoying for me when testing this out earlier, I did create an issue a long time ago in the ECS repo, but nothing really came of it.
I don't think the concept of multi-fields itself adds overhead, but text fields itself adds alot of overhead for sure, so if we had to choose, I rather not have them be multi-field but that decision is not up to me :)
Anything else seems LGTM.
Related to work done by #97901
I've added a daily ci job with notifications to slack (#es-delivery) and email (logs-plus@elastic.co) |
Closes #96713
All three tests cases are covered:
subobjects: false
In addition, I added verification that all ECS multi-field definitions are covered by the ECS dynamic templates (revealing two that actually were not).
@felixbarny @ruflin see if you agree with my comment about not failing if we create multi-field mapping even for fields of which ECS definition does not enforce such.
For example, we define a multi-field mapping for the
*.name
pattern. If you look into the ECS definitions, you will find that many*.name
have multi-field mappings, while many others don't. Trying to be very accurate will result with many more dynamic templates.@P1llus I assigned you as a reviewer because I think you were waiting for this in order to start migrating some ECS mappings to rely on the builtin dynamic templates.