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

veneur-prometheus bugfix: Specifying multiple tags/labels with -a causes sporadic incorrect metric emission #1052

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmarriner
Copy link

@dmarriner dmarriner commented Feb 28, 2023

Summary

Sort the labels on a given metric before adding them to the cache.

Motivation

This fixes a bug in veneur-prometheus that caused diff calculations to sporadically break when multiple labels were added via the -a flag. Labels were stored as key/values in a map and returned in a potentially different order each time since map iteration order is random in golang. This caused the translator to sometimes emit the cumulative value instead of the diffed value, because it would seem as though the metric was new when in fact it had been previously cached with a different label ordering. For example, the translator might look for "counter-key2:value2-key1:value1" when it had been previously cached as "counter-key1:value1-key2:value2"

Test plan

Wrote an integration test.

Rollout/monitoring/revert plan

N/A

…rect metric calculations

This fixes a bug that caused diff calculations to sporadically break when multiple labels
were added via the -a flag.  Labels were stored as key/values in a map and returned in a potentially different order
each time since map iteration order is random.  This caused the translator to emit the cumulative value instead of
the diffed value, because it would seem as though the metric was new when in fact it had been previously cached
with a different label ordering.  For example, the translator might look for "counter-key2:value2-key1:value1" when
it had been previously cached as "counter-key1:value1-key2:value2"
@CLAassistant
Copy link

CLAassistant commented Feb 28, 2023

CLA assistant check
All committers have signed the CLA.

@mimran-stripe
Copy link
Contributor

Hey @dmarriner!

I'm peaceful with this. The probability a map iteration would pass all 100 asserts is very low if I'm not mistaken. Assuming that iteration is truly random:
(1 * 0.5 * 0.5) ^ (100) = 6.22301528e-61

One nit: could you update that PR comment to have

Summary
Sort the labels on a given metric before adding them to the cache.

Motivation
This fixes a bug in veneur-prometheus that caused diff calculations to sporadically break when multiple labels were added via the -a flag. Labels were stored as key/values in a map and returned in a potentially different order each time since map iteration order is random in golang. This caused the translator to sometimes emit the cumulative value instead of the diffed value, because it would seem as though the metric was new when in fact it had been previously cached with a different label ordering. For example, the translator might look for "counter-key2:value2-key1:value1" when it had been previously cached as "counter-key1:value1-key2:value2"

It'll make viewing git blame history a bit easier.

@dmarriner
Copy link
Author

Updated the PR comment!

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