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

Counter with histogram suffix will clash with histogram #461

Merged

Conversation

pedro-stanaka
Copy link
Contributor

Summary

While running the exporter in production a colegue faced an error when a counter clashes with a previously collected histogram.
I managed to reproduce the bug with some tests cases and this PR fixes the problem.

Bug description

The problem can be reproduced in the following manner.

  1. A sample foo:1|h (a histogram - to be translated to a prometheus histogram) is sent to the exporter.
  2. The exporter will register foo_count, foo_sum and foo_bucket.
  3. Later a sample foo_count:1|c arrives at the exporter, the exporter will also register the counter.
  4. The gatherer will detect that this counter is not part of the histogram, but a separate counter and will crash.

Changes

  • Fix the described bug, by checking for existing histograms before registering the counters.
  • Improve isolation of test case TestConflictingMetrics() by using a new Prometheus registry for each test run.

pedro-stanaka and others added 2 commits August 26, 2022 11:40
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
…stogram

Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
@pedro-stanaka pedro-stanaka marked this pull request as draft August 26, 2022 09:54
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Copy link
Contributor

@matthiasr matthiasr left a comment

Choose a reason for hiding this comment

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

This is great, thank you! Do we need the same check for gauges?

Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
@pedro-stanaka
Copy link
Contributor Author

This is great, thank you! Do we need the same check for gauges?

Good catch. Added a few more test cases and I am checking now for gauges as well.

Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
@pedro-stanaka
Copy link
Contributor Author

From what I see, the rest should be covered.

@pedro-stanaka
Copy link
Contributor Author

Just checking, anything else needed here @matthiasr ?

@matthiasr
Copy link
Contributor

Sorry, my bad, I'm underwater with work. Will review as soon as possible.

Copy link
Contributor

@matthiasr matthiasr left a comment

Choose a reason for hiding this comment

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

Thank you!

@matthiasr matthiasr merged commit edaa338 into prometheus:master Sep 13, 2022
matthiasr added a commit that referenced this pull request Sep 13, 2022
with changelog for #461 & #463

Signed-off-by: Matthias Rampke <matthias@prometheus.io>
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.

None yet

2 participants