-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
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. What to do if you already signed the CLAIndividual signers
Corporate signers
|
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 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. |
I signed it! |
CLAs look good, thanks! |
/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") |
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 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.
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") |
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.
@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"
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.
lgtm!
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.