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

fix(sdk-metrics-base): coerce histogram boundaries to be implicit Infinity #2859

Merged
merged 6 commits into from
Mar 31, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 24, 2022

Which problem is this PR solving?

Refs: #2824 (comment)

Removes leading -Infinity lower bound and trailing +Infinity upper bound for ExplicitBucketHistogramAggregation. Also, move the boundaries sort to Aggregation since a HistogramAggregator could be created with the same boundaries array repetitively.

This relax the Exporter implementation requirements -- they don't need to be aware of the original user inputs of missing the last +Inf or not.

Also fixes an unexpected change (#2829) on HistogramAggregation that is confused with ExplicitBucketsHistogramAggregation.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • HistogramAggregation
  • ExplicitBucketHistogramAggregation
  • HistogramAggregator

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@legendecas legendecas requested a review from a team March 24, 2022 06:37
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #2859 (51fe32e) into main (7870e95) will increase coverage by 1.38%.
The diff coverage is n/a.

❗ Current head 51fe32e differs from pull request most recent head a804a1d. Consider uploading reports for the commit a804a1d to get more accurate results

@@            Coverage Diff             @@
##             main    #2859      +/-   ##
==========================================
+ Coverage   93.13%   94.52%   +1.38%     
==========================================
  Files         153       20     -133     
  Lines        5229      803    -4426     
  Branches     1119      158     -961     
==========================================
- Hits         4870      759    -4111     
+ Misses        359       44     -315     
Impacted Files Coverage Δ
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...metry-instrumentation-grpc/src/grpc/clientUtils.ts
...ry-instrumentation-grpc/src/grpc-js/clientUtils.ts
...ry-sdk-metrics-base/src/state/SyncMetricStorage.ts
...telemetry-sdk-trace-node/src/NodeTracerProvider.ts
...y-instrumentation-grpc/src/enums/AttributeNames.ts
...pentelemetry-core/src/platform/node/performance.ts
...dk-metrics-base/src/state/MetricStorageRegistry.ts
...ackages/opentelemetry-core/src/trace/TraceState.ts
...sdk-metrics-base/src/state/DeltaMetricProcessor.ts
... and 128 more

@dyladan
Copy link
Member

dyladan commented Mar 25, 2022

Isn't this the opposite of what @aabmass suggested in his comment. Aaron suggested to always return "OTLP way" which I thought was implicit +Inf. This makes it add +Inf as the last bucket automatically? Unless i've misunderstood the OTLP

@legendecas legendecas changed the title fix(sdk-metrics-base): coerce histogram boundaries last element to be Infinity fix(sdk-metrics-base): coerce histogram boundaries to be implicit Infinity Mar 28, 2022
@legendecas
Copy link
Member Author

@dyladan right, I'm having trouble understanding the statements. Updated with implicit lower and upper Infinity bound.

@legendecas legendecas force-pushed the metrics-ff/histogram branch from 2a82fea to ca6c77f Compare March 28, 2022 02:56
@legendecas legendecas mentioned this pull request Mar 31, 2022
@legendecas legendecas merged commit b6cb40f into open-telemetry:main Mar 31, 2022
@legendecas legendecas deleted the metrics-ff/histogram branch March 31, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants