Avoid dupe labels in prom metrics #2194

blakebarnett · 2019-03-07T22:38:35Z

Since the # of containers shouldn't be massive on a single machine this is probably fine for memory allocation.

googlebot · 2019-03-07T22:38:40Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

k8s-ci-robot · 2019-03-07T22:38:55Z

Hi @blakebarnett. Thanks for your PR.

I'm waiting for a google or kubernetes 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

blakebarnett · 2019-03-07T22:42:13Z

I signed it!

googlebot · 2019-03-07T22:42:16Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

dashpole · 2019-03-07T22:45:21Z

/ok-to-test

dashpole · 2019-03-07T22:52:59Z

The following files are not properly formatted:
metrics/prometheus.go

dashpole · 2019-03-07T22:55:42Z

metrics/prometheus.go

+			sl := sanitizeLabelName(l)
+			for _, x := range labels {
+			   if sl != x {
+				   duplicate = true


I think this will stay permanently true, and we will skip all subsequent labels

oops, fixing

dashpole · 2019-03-07T22:56:41Z

metrics/prometheus.go

+					break
+				}
+			}
+			if duplicate != true {


s/duplicate != true/!duplicate

dashpole · 2019-03-07T23:37:57Z

metrics/prometheus.go

@@ -1155,8 +1155,19 @@ func (c *PrometheusCollector) collectContainersInfo(ch chan<- prometheus.Metric)
 		values := make([]string, 0, len(rawLabels))
 		labels := make([]string, 0, len(rawLabels))
 		containerLabels := c.containerLabelsFunc(cont)
+		duplicate := false


maybe just declare duplicate inside the for l := range rawLabels block? Then you don't need to reset it to false each loop.

Yeah, confused myself by doing this a different way before submitting the PR. Thanks :)

dashpole · 2019-03-07T23:45:52Z

metrics/prometheus.go

+				}
+			}
+			if !duplicate {
+				labels = append(labels, sl)


can we end up with fewer labels than values? Should we move the values = append(values, containerLabels[l]) statement inside here as well?

I was wondering that after the test failure, but this shouldn't change that behavior right? It will only exclude a label if a duplicate label already exists and the value will still get set for that label.

I would be surprised if prometheus didn't yell at us if we tried to, for example, use a description with 3 labels, but then provide 4 label values when creating the metric. What you are implicitly doing here is just using the first occurrence of a given sanitized label, and ignoring subsequent ones.

Hmm, yeah I see. It could happen if someone provides multiple permutations of a label that all normalize to the same thing. Should we just throw them out in that case? I can't think of a great default behavior there.

^ on the same container, it should hopefully be a rare edge-case...

I think just picking the first is a fine behavior.

No #2181 was node-wide. 2 separate containers (separate pods) with annotations that normalize to the same thing caused the panic.

Ok, I get the difference now. I think we would still always get more values than labels, since if the label isn't present, we still add it with the value "".

That's true, in fact I noticed when running without --store_container_labels=false all labels that were present on any of the containers on the host showed up with empty values for all container metrics in prometheus, that was what made me look into the cgroup whitelisting initially and then I noticed this crash behavior.

Yeah... Prometheus requires that all metric streams in a given scrape have the same set of labels. So our workaround is just to add empty values for all labels we don't have.

blakebarnett · 2019-03-14T00:12:10Z

We noticed a something similar to this: #2183 with --store_container_labels=false, the first container in the cgroup has all the kubernetes-provided label values, the rest don't. I can't figure if this is a regression of #1704 or something new...

This is without any changes and without this PR applied, I discovered it when validating this change.

dashpole · 2019-06-21T18:37:55Z

Feel free to close if you are no longer working on this. IIRC, this re-introduces #1704 in its current form. Let me know if that is not the case.

blakebarnett · 2019-06-21T19:20:17Z

Sorry, lost track of this one. This should be fine to go in. The issue I mentioned above about store_cotnainer_labels=false seems to be the bigger problem for us, I'll open another issue/PR for it.

dashpole

lgtm

nightah · 2019-10-15T04:54:13Z

@dashpole is this likely to make it in a release for Docker anytime soon?
I am experiencing the same issue that is exhibited in #2181 and have rolled back to v0.32.0 in the interim.

Blake added 2 commits March 7, 2019 13:54

Add a check for the existence of a label after sanitization

e346926

Do a check in-line for duplicate labels

c1e29ad

Since the # of containers shouldn't be massive on a single machine this is probably fine for memory allocation.

k8s-ci-robot added the needs-ok-to-test label Mar 7, 2019

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 7, 2019

Oops, forgot to run go fmt after redoing things

517a475

dashpole reviewed Mar 7, 2019

View reviewed changes

metrics/prometheus.go Outdated

break

}

}

if duplicate != true {

Copy link

Collaborator

dashpole Mar 7, 2019

Choose a reason for hiding this comment

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

s/duplicate != true/!duplicate

Blake added 2 commits March 7, 2019 14:59

Reset duplicate on next iteration

6001820

Fix logic derp

a0d73dd

dashpole reviewed Mar 7, 2019

View reviewed changes

Move duplicate defintion

e05e69f

dashpole reviewed Mar 7, 2019

View reviewed changes

dashpole self-assigned this Jun 21, 2019

Blake added 2 commits June 21, 2019 12:07

Ensure we append values only when no duplicate labels are found

4657b62

formatting

673e803

dashpole approved these changes Jun 21, 2019

View reviewed changes

dashpole merged commit e8b24bf into google:master Jun 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid dupe labels in prom metrics #2194

Avoid dupe labels in prom metrics #2194

blakebarnett commented Mar 7, 2019

googlebot commented Mar 7, 2019

k8s-ci-robot commented Mar 7, 2019

blakebarnett commented Mar 7, 2019

googlebot commented Mar 7, 2019

dashpole commented Mar 7, 2019

dashpole commented Mar 7, 2019

dashpole Mar 7, 2019

blakebarnett Mar 7, 2019

dashpole Mar 7, 2019

dashpole Mar 7, 2019

blakebarnett Mar 7, 2019

dashpole Mar 7, 2019

blakebarnett Mar 8, 2019 •

edited

Loading

dashpole Mar 8, 2019

blakebarnett Mar 8, 2019

blakebarnett Mar 8, 2019

dashpole Mar 8, 2019

blakebarnett Mar 8, 2019

dashpole Mar 9, 2019

blakebarnett Mar 9, 2019

dashpole Mar 9, 2019

blakebarnett commented Mar 14, 2019 •

edited

Loading

dashpole commented Jun 21, 2019

blakebarnett commented Jun 21, 2019

dashpole left a comment

nightah commented Oct 15, 2019

Avoid dupe labels in prom metrics #2194

Avoid dupe labels in prom metrics #2194

Conversation

blakebarnett commented Mar 7, 2019

googlebot commented Mar 7, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

k8s-ci-robot commented Mar 7, 2019

blakebarnett commented Mar 7, 2019

googlebot commented Mar 7, 2019

dashpole commented Mar 7, 2019

dashpole commented Mar 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakebarnett Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakebarnett commented Mar 14, 2019 • edited Loading

dashpole commented Jun 21, 2019

blakebarnett commented Jun 21, 2019

dashpole left a comment

Choose a reason for hiding this comment

nightah commented Oct 15, 2019

blakebarnett Mar 8, 2019 •

edited

Loading

blakebarnett commented Mar 14, 2019 •

edited

Loading