Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 4, 2018
Merged

Disable labels dedotting in default docker module configuration #7485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ https://github.com/elastic/beats/compare/v6.2.3...master[Check the HEAD diff]
- RabbitMQ management plugin path is now configured at the module level instead of having to do it in each of the metricsets. New `management_path_prefix` option should be used now {pull}7074[7074]
- RabbitMQ node metricset only collects metrics of the instance it connects to, `node.collect: cluster` can be used to collect all nodes as before. {issue}6556[6556] {pull}6971[6971]
- Change http/server metricset to put events by default under http.server and prefix config options with server.. {pull}7100[7100]
- Disable dedotting in docker module configuration. This will change the out-of-the-box behaviour, but not the one of already configured instances. {pull}7485[7485]

*Packetbeat*

Expand Down
4 changes: 2 additions & 2 deletions metricbeat/docs/modules/docker.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 `_`.
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

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 👍

Copy link
Member Author

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? 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7497

#labels.dedot: false
# To connect to Docker over TLS you must specify a client and CA certificate.
#ssl:
Expand Down
4 changes: 2 additions & 2 deletions metricbeat/metricbeat.reference.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,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 `_`.
#labels.dedot: false

# To connect to Docker over TLS you must specify a client and CA certificate.
#ssl:
Expand Down
4 changes: 2 additions & 2 deletions metricbeat/module/docker/_meta/config.reference.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
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 `_`.
#labels.dedot: false

# To connect to Docker over TLS you must specify a client and CA certificate.
#ssl:
Expand Down
4 changes: 2 additions & 2 deletions metricbeat/module/docker/_meta/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
period: 10s
hosts: ["unix:///var/run/docker.sock"]

# Replace dots in labels with `_`. Set to false to keep dots
labels.dedot: true
# If set to true, replace dots in labels with `_`.
#labels.dedot: false

# To connect to Docker over TLS you must specify a client and CA certificate.
#ssl:
Expand Down
4 changes: 2 additions & 2 deletions metricbeat/modules.d/docker.yml.disabled
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
period: 10s
hosts: ["unix:///var/run/docker.sock"]

# Replace dots in labels with `_`. Set to false to keep dots
labels.dedot: true
# If set to true, replace dots in labels with `_`.
#labels.dedot: false

# To connect to Docker over TLS you must specify a client and CA certificate.
#ssl:
Expand Down