feat(sdk-metrics-base): implement min/max recording for Histograms #3032

pichlermarc · 2022-06-10T15:20:12Z

Which problem is this PR solving?

The Metrics SDK spec defines an option RecordMinMax to turn on or off min and max recordings on Explicit Bucket Histogram Aggregations.

This PR:

adds min/max recording to Histograms
makes this configurable via the ExplicitBucketHistogramAggregation
updates the proto to 0.18 so that min and max can be exported

Fixes #3010

Type of change

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
- Breaks OTLP/JSON Metric Exporter for all collector versions <0.52 (due to http receiver drops JSON requests with extra keys opentelemetry-collector#5312)

How Has This Been Tested?

Unit tests
Manual testing with different collector versions

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

codecov · 2022-06-10T15:23:40Z

Codecov Report

Merging #3032 (67785b1) into main (fa295a3) will decrease coverage by 0.00%.
The diff coverage is 93.10%.

@@            Coverage Diff             @@
##             main    #3032      +/-   ##
==========================================
- Coverage   92.65%   92.64%   -0.01%     
==========================================
  Files         187      187              
  Lines        6178     6198      +20     
  Branches     1304     1314      +10     
==========================================
+ Hits         5724     5742      +18     
- Misses        454      456       +2

Impacted Files	Coverage Δ
...telemetry-sdk-metrics-base/src/aggregator/types.ts	`100.00% <ø> (ø)`
...tal/packages/otlp-transformer/src/metrics/types.ts	`100.00% <ø> (ø)`
...metry-sdk-metrics-base/src/aggregator/Histogram.ts	`96.87% <90.90%> (-3.13%)`	⬇️
...telemetry-sdk-metrics-base/src/view/Aggregation.ts	`98.46% <100.00%> (ø)`
.../packages/otlp-transformer/src/metrics/internal.ts	`98.14% <100.00%> (+0.07%)`	⬆️

experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts

dyladan

Really makes me wish we had gone the route of having the histogram delta by default but oh well. Overall LGTM and i'm glad to have this feature in

legendecas · 2022-06-21T14:22:34Z

Really makes me wish we had gone the route of having the histogram delta by default but oh well.

@dyladan I'm not getting what you are pointing at. Would you mind elaborating on this? I'd be happy to find out if we can do it in a better way.

dyladan · 2022-06-21T14:26:40Z

experimental/packages/otlp-transformer/src/metrics/internal.ts

+    if(histogram.hasMinMax){
+      return {
+        attributes: toAttributes(dataPoint.attributes),
+        bucketCounts: histogram.buckets.counts,
+        explicitBounds: histogram.buckets.boundaries,
+        count: histogram.count,
+        sum: histogram.sum,
+        min: histogram.min,
+        max: histogram.max,
+        startTimeUnixNano: hrTimeToNanoseconds(dataPoint.startTime),
+        timeUnixNano: hrTimeToNanoseconds(
+          dataPoint.endTime
+        ),
+      };
+    } else {
+      return {
+        attributes: toAttributes(dataPoint.attributes),
+        bucketCounts: histogram.buckets.counts,
+        explicitBounds: histogram.buckets.boundaries,
+        count: histogram.count,
+        sum: histogram.sum,
+        startTimeUnixNano: hrTimeToNanoseconds(dataPoint.startTime),
+        timeUnixNano: hrTimeToNanoseconds(
+          dataPoint.endTime
+        ),
+      };
+    }


Why do we need this if/else? If min and max are not there won't they simply be undefined?

Thanks for catching this. For some reason I thought that this was necessary, but it is not. 😅
I simplified this in b23b6f1. 🙂
min/max are not undefined when they get here, though (they are Inf and -1), so I still need to check hasMinMax and set undefined explicitly.

dyladan · 2022-06-21T14:48:14Z

Really makes me wish we had gone the route of having the histogram delta by default but oh well.

@dyladan I'm not getting what you are pointing at. Would you mind elaborating on this? I'd be happy to find out if we can do it in a better way.

Sorry I confused myself a bit while reviewing. I was looking to see where merge is called and got a bit lost in the details of the SDK. I don't remember what I was specifically looking at when I made that comment sorry.

…x and without.

pichlermarc · 2022-06-21T15:46:34Z

Really makes me wish we had gone the route of having the histogram delta by default but oh well.

@dyladan I'm not getting what you are pointing at. Would you mind elaborating on this? I'd be happy to find out if we can do it in a better way.

Look like it's related to the way diff() is handled in this PR. Since we're diffing two cumulative histograms, there's no way to get the delta min/max without resetting min/max after each diff(). 🙁

It was very elegant before, but now it's kind of counfusing. 😓

legendecas · 2022-06-24T02:41:55Z

@pichlermarc I can not edit this PR so please update the PR with the latest main branch so that we can land this one :).

pichlermarc added 6 commits June 13, 2022 11:09

feat(sdk-metrics-base): prepare min/max collection.

9deadd6

chore: updating submodule for otlp-grpc-exporter-base

2fe9796

chore: updating submodule for otlp-proto-exporter-base

a02fbbe

feat(sdk-trace-base): implement min/max recording, add tests.

d276cc2

feat(docs): update metrics exporter compatibility.

70251f8

fix(changelog): add changelog entry.

cac6916

pichlermarc force-pushed the min-max branch from eeccb23 to cac6916 Compare June 13, 2022 09:14

pichlermarc added 2 commits June 13, 2022 11:28

fix(changelog): add more detail to changelog.

16a2f41

fix(changelog): reword, appease linter.

b5ece8a

pichlermarc marked this pull request as ready for review June 13, 2022 09:58

pichlermarc requested a review from a team June 13, 2022 09:58

pichlermarc mentioned this pull request Jun 13, 2022

[exporter/logging] log min and max for histograms open-telemetry/opentelemetry-collector#5520

Merged

legendecas approved these changes Jun 14, 2022

View reviewed changes

experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts Outdated Show resolved Hide resolved

experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts Outdated Show resolved Hide resolved

pichlermarc added 2 commits June 15, 2022 11:42

refactor(sdk-metrics-base): inline hasMinMax(), reformat.

031219f

Merge branch 'main' into min-max

9ea313c

dyladan approved these changes Jun 17, 2022

View reviewed changes

dyladan and others added 2 commits June 17, 2022 13:18

Merge branch 'main' into min-max

9688751

Merge branch 'main' into min-max

453b2ba

dyladan reviewed Jun 21, 2022

View reviewed changes

fix(otlp-transformer): simplify distinction with for data with min/ma…

b23b6f1

…x and without.

dyladan added this to the Metrics GA milestone Jun 22, 2022

Merge branch 'main' into min-max

67785b1

dyladan merged commit d2de661 into open-telemetry:main Jun 24, 2022

dyladan deleted the min-max branch June 24, 2022 12:52

dyladan mentioned this pull request Jun 29, 2022

release proposal 1.4.0/0.30.0 #3065

Merged

pichlermarc mentioned this pull request Jul 12, 2022

fix(histogram): fix maximum when only values < -1 are provided #3086

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk-metrics-base): implement min/max recording for Histograms #3032

feat(sdk-metrics-base): implement min/max recording for Histograms #3032

pichlermarc commented Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

dyladan left a comment

legendecas commented Jun 21, 2022

dyladan Jun 21, 2022

pichlermarc Jun 21, 2022

dyladan commented Jun 21, 2022

pichlermarc commented Jun 21, 2022

legendecas commented Jun 24, 2022

feat(sdk-metrics-base): implement min/max recording for Histograms #3032

feat(sdk-metrics-base): implement min/max recording for Histograms #3032

Conversation

pichlermarc commented Jun 10, 2022 • edited Loading

Which problem is this PR solving?

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Jun 10, 2022 • edited Loading

Codecov Report

dyladan left a comment

Choose a reason for hiding this comment

legendecas commented Jun 21, 2022

dyladan Jun 21, 2022

Choose a reason for hiding this comment

pichlermarc Jun 21, 2022

Choose a reason for hiding this comment

dyladan commented Jun 21, 2022

pichlermarc commented Jun 21, 2022

legendecas commented Jun 24, 2022

pichlermarc commented Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading