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

Control whether container labels are exported as prometheus metrics. #1984

Merged
merged 1 commit into from
Jul 4, 2018
Merged

Control whether container labels are exported as prometheus metrics. #1984

merged 1 commit into from
Jul 4, 2018

Conversation

jaloren
Copy link
Contributor

@jaloren jaloren commented Jun 28, 2018

See #1951 for context.

when cadvisor exports metrics for docker containers, there is a root cgroup (/) and cgroup for a docker container (/docker/uuid).
If docker container has a label on it, then this label is applied to all containers including the root container. Because some containers don't have that label, the label will have an empty value. The reason for this is that Prometheus does not allow sending a metric with the same name, but different labels, so cadvisor uses empty label values based on the set of all labels for a given metric. This can result in many docker containers getting a large number of empty labels because another container has that label.

If large number of docker labels vary a lot across images, then the set of labels will be enormous, where most of the labels will be empty and have no value as prometheus metrics. To avoid this problem, a flag is provided that allows a user to disable exporting docker labels as metrics.

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@k8s-ci-robot
Copy link
Collaborator

Hi @jaloren. Thanks for your PR.

I'm waiting for a kubernetes or google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jaloren
Copy link
Contributor Author

jaloren commented Jun 28, 2018

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@dashpole
Copy link
Collaborator

/ok-to-test

cadvisor.go Outdated
@@ -58,6 +59,8 @@ var enableProfiling = flag.Bool("profiling", false, "Enable profiling via web in
var collectorCert = flag.String("collector_cert", "", "Collector's certificate, exposed to endpoints for certificate based authentication.")
var collectorKey = flag.String("collector_key", "", "Key for the collector's certificate")

var dockerLabels = flag.Bool("docker_labels", true, "convert docker labels and environment variables into labels on prometheus metrics for each docker container. If flag set to false, then only metrics exported are container name, first alias, and image name")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't limited to docker labels (as it works for other container runtimes that expose labels), lets rename this to store_container_labels or something similar.

@dashpole dashpole self-assigned this Jul 3, 2018
when cadvisor exports metrics for docker containers, there is a root cgroup (/) and cgroup for a docker container (/docker/uuid).
If docker container has a label on it, then this label is applied to all containers including the root container.
Because some containers don't have that label, the label will have an empty value. The reason for this is that Prometheus
does not allow sending a metric with the same name, but different labels, so cadvisor uses empty label values based on
the set of all labels for a given metric. This can result in many docker containers getting a large number of empty labels
because another container has that label.

If large number of docker labels vary a lot across images, then the set of labels will be enormous, where most of the labels
will be empty and have no value as prometheus metrics. To avoid this problem, a flag is provided that allows a user to
disable exporting docker labels as metrics.
@@ -58,6 +59,8 @@ var enableProfiling = flag.Bool("profiling", false, "Enable profiling via web in
var collectorCert = flag.String("collector_cert", "", "Collector's certificate, exposed to endpoints for certificate based authentication.")
var collectorKey = flag.String("collector_key", "", "Key for the collector's certificate")

var storeContainerLabels = flag.Bool("store_container_labels", true, "convert container labels and environment variables into labels on prometheus metrics for each container. If flag set to false, then only metrics exported are container name, first alias, and image name")
Copy link
Contributor Author

@jaloren jaloren Jul 4, 2018

Choose a reason for hiding this comment

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

@dashpole flag name store_container_labels seems unnecessarily verbose for someone executing this on a command line, especially since its a boolean. But to be consistent with the other flags, I followed the "flag name should match variable name but underscores instead of camel case"

Copy link
Collaborator

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

lgtm!

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.

4 participants