-
Notifications
You must be signed in to change notification settings - Fork 442
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
[elasticsearch] Align mappings with metricbeat #3928
[elasticsearch] Align mappings with metricbeat #3928
Conversation
elasticsearch: | ||
index_template: | ||
mappings: | ||
dynamic: 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.
cluster_stats is the offending metricset that caused the initial issue in #3918 so I'd be inclined to close it at the same time, unless we want to review all metricsets and apply dynamic: false
where necessary
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 ended up disabling dynamic mappings on every data streams
🌐 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.
Tested locally and didn't run into any errors, LGTM 👍🏼
@@ -1,6 +1,10 @@ | |||
type: metrics | |||
title: Elasticsearch ccr metrics | |||
release: experimental | |||
elasticsearch: |
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.
Why is it elasticsearch
here? What does that represent?
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.
That's the section allowing more granular control over the assets installed, more options in the package-spec
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.
Ah I see, thanks!
@@ -261,10 +261,10 @@ | |||
- name: control_group | |||
type: keyword | |||
- name: limit.bytes | |||
type: long | |||
type: keyword |
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.
Should we add some ignore_above here?
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.
this didn't surface as a diff surprisingly but I'll add that where needed
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.
It might not be in the original mappings at all but maybe it's needed, I see it in some places and some places not so I never know
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 was thinking about creating tests for these data streams, but given the fact that we'll deprecate this package, I'm not sure of it's worth the effort. Definitely something to take into consideration for the new integration packages
@@ -13,7 +13,7 @@ format_version: 1.0.0 | |||
license: basic | |||
categories: ["elastic_stack", "datastore"] | |||
conditions: | |||
kibana.version: ^7.15.0 | |||
kibana.version: ^8.5.0 |
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.
Should we increment the package semver too? if so, I would also add an entry to the changelog file
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.
let's keep that for #3917 so that we can pack all the changes in a single changelog entry
What kind of tests ? Since we have the SM ui as an official consumer of these streams I thought integration tests in Kibana would cover everything we need. PO will be a different story though |
When I wrote the comment I had pipeline test in mind, but here we only have data streams, so it's not the case. But digging into integration packages doc it seems that it's possible to create tests for data streams too https://github.com/elastic/elastic-package/blob/main/docs/howto/system_testing.md. I've never tried it, so I don't know what the possibilities are. |
I'll merge this now as the ignore_above task may take a while, followup here #3959 |
Summary
Closes #3914
Align the elasticsearch data streams' mappings with the metricbeat ones. Methodology described in this issue
Testing
elastic-package build
elastic-package stack up -v --version 8.5.0-SNAPSHOT
Known issues