-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Disable labels dedotting in default docker module configuration #7485
Conversation
Default docker module configuration made metricbeat to replace dots with underscores in labels. This conflicted with `add_docker_metadata` that also adds labels, but without replacing dots. This makes docker module and `add_docker_metadata` processor to have a consistent behaviour, but there can be still cases where type mapping conflicts appear. For these cases there are some options using the `rename` or `drop_fields` processors.
56e5312
to
7ba5aae
Compare
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.
WFG
@@ -42,8 +42,8 @@ metricbeat.modules: | |||
period: 10s | |||
enabled: true | |||
|
|||
# Replace dots in labels with `_`. Set to false to keep dots | |||
labels.dedot: true | |||
# If set to true, replace dots in labels with `_`. |
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.
We should be careful here as this ends up in 6.4 and docker module is in GA. So this change can only happen for 7.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.
Yeah, @exekias and I talked about this, but we still thought that this could happen in 6.4, there are some reasons:
- The behaviour between the docker module and the
add_docker_metadata
[beta] processor is inconsistent with the provided configurations, and produces mapping conflicts. This alone could be considered a bug, as it breaks the initial experience. - We think that it is better to don't dedot by default. When dedotting, the user cannot use the labels directly to make queries, what is a bit counterintuitive. Also information in the label name is lost (different labels could generate the same field name and it is not always possible to rebuild the original label from the dedotted string).
- An alternative to fix the "inconsistency bug" would be to add dedot capabilities to the processor (that is still beta), but then we'd be adding two changes, one in 6.4, and another one in 7.0, as we'd like the default to be not-dedotting.
- This BC/"fix" changes the provided and reference configurations, it doesn't change the implementation of the module.
On the cons side, not dedotting can still produce mapping conflicts if a source uses labels with a subpath of other source (e.g. foo.bar
and foo.bar.baz
), so the user may still need some additional processors to handle these cases.
We can also leave it as is, and point the users having problems to the options they have.
The ideal solution would be to be able to store the labels as they are, or with some kind of escaping for the dots that still allows to make queries in an intuitive way. But I don't know if something like this is possible with the current Stack.
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 agree this is a breaking change, but it's also a soft one. Take into account we are only changing default configuration for the module, so already deployed modules won't change their behavior, this change only affects to new deployments.
I believe this should be the solution going forward, still, it is debatable if this should go to 6.4 or 7.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.
The good news for me is that it was uncommented previously so everyone using the default config will not experience any change. I think that is also the reason we had it there to have a simpler migration path in 7.0.
The broken user experience here is definitively a very strong point. I'm in general on board with making the breaking change especially as we have a config to keep the old behaviour. If we go forward, lets make sure we mention it not only in the changelog but also in the breaking changelog and what users need to change and some explanation.
As the processor is still in beta, could we bring it to GA? I think that would enforce our arguments on why we changed the defaults.
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.
Sounds like a plan to me 👍
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.
So I prepare another PR to make add_docker_metadata
GA? 😄
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.
@jsoriano let's make sure we put a line in the breaking changes section of release notes |
Default docker module configuration made metricbeat to replace dots
with underscores in labels. This conflicted with
add_docker_metadata
that also adds labels, but without replacing dots.
This makes docker module and
add_docker_metadata
processor to have aconsistent behaviour, but there can be still cases where type mapping
conflicts appear. For these cases there are some options using the
rename
ordrop_fields
processors.