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): allow single bucket histograms #4456

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Feb 1, 2024

Which problem is this PR solving?

When creating a histogram with an empty bucket via the API, the SDK throws. Such a histogram can be useful if just min/max/sum/count are needed. Since previously, the only way to set up an aggregation would have been via Views, it was fine for ExplicitBucketHistogramAggregation to throw (View creation happens on SDK setup). Now that the same thing can happen via the API we need to ensure that we do not throw.

Further, reading through the spec it looks like single-bucket histograms are actually permitted. Therefore, I didn't change it to log a warning instead.

I added a few more tests to ensure that this special case is covered works with the the existing aggregators.

Type of change

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

How Has This Been Tested?

  • Added unit tests

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Merging #4456 (5d9da2b) into main (f6712fd) will decrease coverage by 0.20%.
The diff coverage is 100.00%.

❗ Current head 5d9da2b differs from pull request most recent head ce239b6. Consider uploading reports for the commit ce239b6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4456      +/-   ##
==========================================
- Coverage   93.12%   92.93%   -0.20%     
==========================================
  Files          82      259     +177     
  Lines        1658     6991    +5333     
  Branches      341     1459    +1118     
==========================================
+ Hits         1544     6497    +4953     
- Misses        114      494     +380     
Files Coverage Δ
packages/sdk-metrics/src/view/Aggregation.ts 100.00% <100.00%> (ø)

... and 178 files with indirect coverage changes

@pichlermarc pichlermarc marked this pull request as ready for review February 1, 2024 12:50
@pichlermarc pichlermarc requested a review from a team February 1, 2024 12:50
@pichlermarc pichlermarc added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc labels Feb 6, 2024
@pichlermarc pichlermarc merged commit 63845ec into open-telemetry:main Feb 6, 2024
18 checks passed
@pichlermarc pichlermarc deleted the fix/allow-emtpy-bucket-advice branch February 6, 2024 13:35
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
* fix(sdk-metrics): allow single bucket histograms

* test(sdk-metrics): undefined and null inputs for bucket boundaries

* fixup! test(sdk-metrics): undefined and null inputs for bucket boundaries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants