Allow non-monontonic and empty sums on Histogram data points. #320
Conversation
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.
|
FYI - I think this is what we discussed in the last metrics SiG: @jmacd @victlu @bogdandrutu |
| enum SumType { | ||
| // The histogram sum is a monotonic value and will not decrease. | ||
| // Note: This is the default for histograms if unspecified. | ||
| MONOTONIC = 0; |
There was a problem hiding this comment.
Should we avoid 0 as we may not know if it's set or missing?
Maybe...
NONE = 0
MONOTONIC = 1
NON_MONOTONIC = 2
UNKNOWN_SUM = 3
There was a problem hiding this comment.
Backwards-commpatiblity means previous users would NOT be specifying this value, which defaults to 0. So we should treat that as MONOTONIC.
There was a problem hiding this comment.
Was it that before? Did we have sum as monotonic?
There was a problem hiding this comment.
Yes, sum was monotonic before
There was a problem hiding this comment.
From here, that is not clear:
// Note: Sum should only be filled out when measuring non-negative discrete
// events, and is assumed to be monotonic over the values of these events.
// Negative events *can* be recorded, but sum should not be filled out when
// doing so. This is specifically to enforce compatibility w/ OpenMetrics,
// see: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#histogram
There was a problem hiding this comment.
See the linked bug. Basically we can't not fill out a sum, so due to the phrasing, we're always providing a sum and it's always monotonic.
There was a problem hiding this comment.
Instead of making "sum" special in terms of having an extra metadata, have we consider the following:
- Deprecating the current "sum" - keep it for backwards compatibility for 1.5y as we did for StatusCode in trace.
- Add a new sum with the type "google.protobuf.DoubleValue"?
The advantage is that this is a point scope property not a metric scope property? Is this important?
Didn't entertain removing sum completely. AFAICT something like it is expected as a value here. Care to comment on the viablity concerns to that approach?
This doesn't solve the issue of understanding if the sum is monotonic or non-monotonic (which lead to the existing wonky wording we have). We'd like to keep sum, we just also need to know how to export it to prometheus so we need some kind of monotonicity marker.
@jmacd pushed for the semantic of sum to be a metric-scope property vs. point scope (which was an earlier proposal). I could go either way myself, and see enough flexibility in metric-scope (given InstrumentationLibrary) that I think we're ok here. |
| // The histogram sum is a monotonic value and will not decrease. | ||
| // Note: This is the default for histograms if unspecified. | ||
| MONOTONIC = 0; | ||
| // The histogram sum may increasee or decrease. |
There was a problem hiding this comment.
| // The histogram sum may increasee or decrease. | |
| // The histogram sum may increase or decrease. |
For me this seems to be inconsistent with the flags. Why is a difference there? |
|
In #316, the one new If there is a distinction to be made about the monotonicity of the Sum, then it appears to be a metric-level property, since it relates to the difference between points. I can't imagine what it means to have a stream of points where some declare their sum to be monotonic and some do not--those appear to be different streams, so I think the distinction belongs in the metric not the point. I have in the past attempted to optimize the number of fields in these protocols by taking a cross-product of existing enums. We already have an @bogdandrutu would that alleviate your concern about creating a new field? |
|
Going to propose we migrate to optional in proto, with justifcation. PR pending. |
Fixes #303