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

Is knowing that a Histogram/Summary sum is monotonic important? #187

Closed
bogdandrutu opened this issue Jul 30, 2020 · 6 comments · Fixed by #302
Closed

Is knowing that a Histogram/Summary sum is monotonic important? #187

bogdandrutu opened this issue Jul 30, 2020 · 6 comments · Fixed by #302
Labels
area:data-model release:allowed-for-ga Editorial changes that can still be added to the GA spec since they don't require action by SIGs spec:metrics

Comments

@bogdandrutu
Copy link
Member

Currently there is a TODO in the metrics proto that needs a decision:

// TODO: Decide if knowing that the Sum is monotonic is important or not

@jmacd jmacd added the release:after-ga Not required before GA release, and not going to work on before GA label Jul 30, 2020
@hdost
Copy link
Contributor

hdost commented Feb 10, 2021

This looks as if in the v1 of the protospec that this comment is gone which makes me think that this has been decided?

@bogdandrutu bogdandrutu added area:data-model and removed release:after-ga Not required before GA release, and not going to work on before GA labels Feb 19, 2021
@bogdandrutu
Copy link
Member Author

@jmacd I think we need this for the Histogram (probably for Summary as well), because currently OpenMetrics requires us to not send the Sum if values can be negative. We may want to discuss this in the Prometheus wg.

@jsuereth
Copy link
Contributor

@bogdandrutu Can you comment on (a) if this is still an issue given the TODO is now gone and (b) how we resolve it.

Proposal from DataModel SiG:

  • For OM / Prometheus export, ignore SUM if there are negative bucket values.

@bogdandrutu
Copy link
Member Author

@jsuereth that is not 100% correct for the export. Reason is that I may not have a negative bucket defined but I may still have negative measurements, so the sum may still not be a Counter.

@jsuereth jsuereth added release:allowed-for-ga Editorial changes that can still be added to the GA spec since they don't require action by SIGs and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Apr 6, 2021
@jsuereth
Copy link
Contributor

jsuereth commented Apr 6, 2021

We discussed this in the DataModel SiG.

  • For now sums will be exported as Counters (high compatibility with prometheus)
  • We'd like to allow the use case of negative values in Histograms.
  • We think we can provide some kind of semantic hint/detail outlining this scenario in the future in a non-breaking way.
    e.g. a is_non_monotonic boolean field that defaults to false.

@jsuereth
Copy link
Contributor

jsuereth commented May 8, 2021

So diving into this further, OpenMetrics defines two types of histograms with subtle differnces:

  • Histograms, those that measure distributions of discrete events (e.g. latency of HTTP requests)
  • GaugeHistograms, those that measure current distributions (e.g. how long items have been waiting in a queue).

For the first, prometheus/OpenMetrics requires that all sums are monotonic and all measurements positive values from discrete events. For the second, you just grab what you found, which can include negative values. Aggregation for "GaugeHistogram" matches aggregation for Gauge

I believe @jmacd wants to model the second type of histogram as an "instantaneous temporality" histogram. It's also possible we could allow a histogram-data-point to be exported from Gauge to accommodate this. However, I want to make it clear we are NOT solving that problem with this bug. We are only focused on making sure we know when it is safe to convert from an OTLP Histogram to an OpenMetrics Histogram.

So, re-affirming the SiGs decision and casting a few lights on things that weren't clear to me until I got caught up on details from Start-Time-Discussions:

  1. Whether or not we allow negative measurements to participate in a distribution of a Histogram is an orthogonal discussion to instantaneous temporality or the notion of "GaugeHistogram" from open metrics. We can and should figure out how to support those, but we can do so in a non-breaking way.
  2. If we have a Histogram of discrete events with negative measurements we cannot export this to OpenMetrics/Prometheus. I'm not sure we know what to do here. The SiG decided that we enforce Histograms have positive values for now and we can figure out how to support negative measurements going forward, in a non-breaking way. This means (for now) all OTLP cumulative histograms can convert directly to OpenMetrics/Prometheus.

To resolve this bug, I'm adding description to the histogram OTLP proto to require non-negative measurements for now. We can expand support for more histogram types going forward.

jsuereth added a commit to jsuereth/opentelemetry-proto that referenced this issue May 8, 2021
…patibility

Fixes open-telemetry#187.

- Summary data points must record non-negative values
- Histogram sum can only be present if underlying measurements are non-negative. (Opening a bug to expand our API to allow this to present in more scenarios later)
bogdandrutu added a commit that referenced this issue May 11, 2021
…pat (#302)

* Refine descriptions of Histogram+Summary for explicit OpenMetrics compatibility

Fixes #187.

- Summary data points must record non-negative values
- Histogram sum can only be present if underlying measurements are non-negative. (Opening a bug to expand our API to allow this to present in more scenarios later)

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Joshua MacDonald <[email protected]>

* Call out openmetrics specification

* Update metrics.proto

Co-authored-by: Bogdan Drutu <[email protected]>
Co-authored-by: Bogdan Drutu <[email protected]>
Co-authored-by: Joshua MacDonald <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:data-model release:allowed-for-ga Editorial changes that can still be added to the GA spec since they don't require action by SIGs spec:metrics
Projects
None yet
4 participants