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.

Sign up for GitHub

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

Jump to bottom

Allow non-monontonic and empty sums on Histogram data points. #320

Closed
wants to merge 3 commits into from
Closed

Allow non-monontonic and empty sums on Histogram data points. #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c034d91
Allow non-monontonic and empty sums on Histogram data points.
jsuereth Jul 20, 2021
5ba13e4
Update changelog.
jsuereth Jul 20, 2021
8805f11
Merge branch 'main' into fix-sums-in-histograms
jsuereth Jul 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Full list of differences found in [this compare.](https://github.com/open-teleme

### Added

* Remove if no changes for this section before release.
* Added an enum to Histogram to denote the meaning of the sum field. (#320)

### Removed

Expand Down
21 changes: 15 additions & 6 deletions opentelemetry/proto/metrics/v1/metrics.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,18 @@ message Histogram {
// aggregation_temporality describes if the aggregator reports delta changes
// since last report time, or cumulative changes since a fixed start time.
AggregationTemporality aggregation_temporality = 2;

enum SumType {
// The histogram sum is a monotonic value and will not decrease.
// Note: This is the default for histograms if unspecified.
MONOTONIC = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backwards-commpatiblity means previous users would NOT be specifying this value, which defaults to 0. So we should treat that as MONOTONIC.

Copy link
Member

@bogdandrutu bogdandrutu Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it that before? Did we have sum as monotonic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sum was monotonic before

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// The histogram sum may increasee or decrease.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The histogram sum may increasee or decrease.
// The histogram sum may increase or decrease.

NON_MONOTONIC = 1;
// The histogram sum is not meaningful and should be ignored.
UNKNOWN_SUM = 2;
}
// How to interpret the sum field on histogram data points.
SumType sum_type = 3;
}

// Summary metric data are used to convey quantile summaries,
Expand Down Expand Up @@ -384,13 +396,10 @@ message HistogramDataPoint {

// sum of the values in the population. If count is zero then this field
// must be zero. This value must be equal to the sum of the "sum" fields in
// buckets if a histogram is provided.
// buckets of a histogram if provided.
//
// 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
// Additionally, this field should abide by the interpretation specified
// in Histogram.sum_type.
double sum = 5;

// bucket_counts is an optional field contains the count values of histogram
Expand Down