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.

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

Added container_labels parameter #1270

Closed
wants to merge 2 commits into from
Closed

Conversation

noxer
Copy link

@noxer noxer commented May 25, 2016

Fixes #1263

@@ -56,6 +57,9 @@ var sampleConfig = `
endpoint = "unix:///var/run/docker.sock"
## Only collect metrics for these containers, collect all if empty
container_names = []
## Only collect these container labels from docker daemon, collect all if empty but note that
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove the mention of the prometheus output, this is too specific for an option that is in fact general.

@sparrc
Copy link
Contributor

sparrc commented May 25, 2016

you should have the container_labels array support glob matching using https://github.com/gobwas/glob.

If the user specifies multiple patterns in container_labels you can construct a list of matches similar to how we do filtering here: https://github.com/influxdata/telegraf/blob/master/internal/models/filter.go#L87

you should only compile the filter once.

@noxer
Copy link
Author

noxer commented May 25, 2016

How would you implement inserting placeholders with those filters?

@sparrc
Copy link
Contributor

sparrc commented May 25, 2016

@noxer what do you mean by that?

@noxer
Copy link
Author

noxer commented May 25, 2016

@sparrc I'm inserting placeholders ("-") for labels that are in d.ContainerLabels but not in container.Labels: tags[l] = "-". Using glob would make that impossible (I can't insert something for "*xyz"). I would suggest inserting such filters as a separate parameter.

@sparrc
Copy link
Contributor

sparrc commented May 25, 2016

hmm I see what you mean. I'd actually rather not merge this, those placeholder labels will appear in every output plugin but don't mean anything except in the prometheus case. I think that you need to come up with a solution that is specific to the prometheus output.

@noxer
Copy link
Author

noxer commented May 25, 2016

@sparrc This seems to be a problem specific to docker input + prometheus output, prometheus needs a fixed set of tags (per input plugin) and docker generates a variable set of tags (due to the container labels). At least in our setup docker is the only input plugin that does that.

@sparrc
Copy link
Contributor

sparrc commented May 25, 2016

this is true for your particular use-case, but in reality it could be any of the more than 70 input plugins that generate variable numbers of tags.

@lightblu
Copy link
Contributor

These placeholders will only appear for nonexistent labels which where specified in container_labels, if you do not specify container_labels you get completely the current behavior and no placeholders at all.

I am still not sure if it is generally ok for a plugin to provide inconsistent and arbitrary labels for a plugin/metric, because no other input plugin I used so far does this (mongo, rabbitmq, and all the default ones like mem, cpu, disk etc.).

I just found that what you propose may be possible with Telegrafs configuration (taginclude) alone, e.g. specifying taginclude = ['network', and all other ones that are not extracted from the container labels, because I need those] on the docker plugin, still it feels wrong for me that you have to specify this to not break an output plugin and to allow for inconsistent and arbitrary labels. Will try this now.

@noxer
Copy link
Author

noxer commented May 25, 2016

Turns out taginclude does exactly what we need, making this PR obsolete.

@noxer noxer closed this May 25, 2016
@sparrc
Copy link
Contributor

sparrc commented May 25, 2016

@lightblu the labels are not "arbitrary", you are specifying them as docker labels 😑

as I said before, this is only a problem for prometheus. It doesn't make sense and it's very bad programming design to have 100 little workarounds in every single input plugin just for the sake of a single output plugin.

@lightblu
Copy link
Contributor

I agree its fine if this is the general approach within Telegraf/Influx, but to me it looked like bad design to have inconsistent label sets on the same metric (biased by prometheus) and the docker plugin to be the only input plugin which shows this behaviour (although I am wrong with this it seems), if this is the general approach I'm sorry.
But the labels are arbitrary in the sense that you usually do not build up all your containers from scratch but use prebuilt ones from registries, where you are bound to the arbitrary labels the creators have already added (see examples in the issue), you cannot remove these when running containers but just add/overwrite.

powersj added a commit to powersj/telegraf that referenced this pull request Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants