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

Allow sum to present with negative measurements in Histogram #303

Open
jsuereth opened this issue May 8, 2021 · 3 comments
Open

Allow sum to present with negative measurements in Histogram #303

jsuereth opened this issue May 8, 2021 · 3 comments
Labels
area:data-model enhancement New feature or request release:after-ga Not required before GA release, and not going to work on before GA spec:metrics

Comments

@jsuereth
Copy link
Contributor

jsuereth commented May 8, 2021

Follow on to #187 - We currently limit the sum field in Histogram to be 100% compatible with OpenMetrics/Prometheus histograms so we know we can do a conversion when it exists. However, we think there's value to having sum available when there are negative measurements as well.

Tentative proposal:

message Histogram {.
  .. 
  // When this is true, a histogram allows negative measurements.  This means
  // the sum is non monotonic, and may be 0 even if there are recorded events.
  bool has_negative_measurements = ?;
}
@jsuereth jsuereth added enhancement New feature or request spec:metrics release:after-ga Not required before GA release, and not going to work on before GA area:data-model labels May 8, 2021
@jmacd
Copy link
Contributor

jmacd commented May 10, 2021

I do not feel enthusiastic about about has_negative_measurements.

There could be finer-grain statements people might like to specify about their instruments, like a range between [0, 100] for example, so maybe we'll find other solutions.

If the definition for repeated double explicit_bounds = 7; were slightly different, we could avoid the indefinite-size buckets and address this problem at the same time. The exponential histogram option can just get this right, e.g., an exponential histogram with no negative buckets implies the desired property. If this is extremely important for explicit boundaries, it can be solved with a new even-more-explicit boundaries array (one extra element to erase this ambiguity).

@jmacd
Copy link
Contributor

jmacd commented May 24, 2021

Referring to the contents of open-telemetry/oteps#156, there are three semantics for the Sum field of a Histogram, corresponding with a Histogram of values to synchronous Counter, UpDownCounter, and Gauge API events, where the sum is:

  • Monotonic (Counter events)
  • Non-monotonic (UpDownCounter events)
  • Non-meaningful (Gauge events)

This suggests an enum value with three states, indicating which of the primitive instruments the measurements in the histogram semantically correspond with.

@jmacd
Copy link
Contributor

jmacd commented May 24, 2021

Taken together with #308, the pair of proposals to me suggest a new field that contains extra semantic bits, two states for Count being monotonic or not, and three states for Sum being monotonic, non-monotonic, or non-meaningful.

jsuereth added a commit to jsuereth/opentelemetry-proto that referenced this issue Jul 20, 2021
Fixes open-telemetry#303

- Add a new enumeration to Histogram denoting the type of sum for that histogram.
- Update documentation to denote this new enumeration is used for interpretataion of the sum field.
- Default the value of this enumeration to "monotonic sum" for backwards compatiblity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:data-model enhancement New feature or request release:after-ga Not required before GA release, and not going to work on before GA spec:metrics
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants