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

For prometheus metrics without sums, leave the sum unset #2413

Merged
merged 3 commits into from
Mar 22, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Mar 10, 2022

Related issue: open-telemetry/opentelemetry-collector-contrib#7546

Changes

It is not possible to implement the spec as it is currently written, since the sum cannot be computed from buckets.

The OpenMetrics histogram spec says:

Negative threshold buckets MAY be used, but then the Histogram MetricPoint MUST NOT contain a sum value as it would no longer be a counter semantically.

We have two options for how to handle histograms without sums:

  • Set the sum to 0
  • Leave it unset
  • Drop the Histogram entirely.

Leaving it unset is the correct way to handle this case.

cc @Aneurysm9 @jmacd

@dashpole dashpole requested review from a team March 10, 2022 18:56
@jmacd
Copy link
Contributor

jmacd commented Mar 15, 2022

I thought we were intending to make the sum field optional to address this, which has been done:

open-telemetry/opentelemetry-proto#366

@dashpole
Copy link
Contributor Author

Thanks for pointing that out, Josh. I wasn't aware of that change, but it makes a lot of sense. I've updated this to say the sum must be unset.

@dashpole dashpole changed the title For prometheus metrics without sums, use a sum of 0 For prometheus metrics without sums, leave the sum unset Mar 16, 2022
@jmacd jmacd merged commit 7a2d451 into open-telemetry:main Mar 22, 2022
@dashpole dashpole deleted the prom_histograms_without_sums branch March 22, 2022 14:51
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.

8 participants