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

Deprecate automatic histogram method for removal. #4218

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Feb 28, 2022

As discussed at the SIG, we don't see a way to actually change the histogram that is returned by this method in the future, in which case it currently overlaps with explicitBucketsHistogram, and constrains us if we do end up wanting to encourage a different one, such as exponential, as the de-facto.

open-telemetry/opentelemetry-specification#2382

@anuraaga anuraaga requested a review from a user February 28, 2022 03:33
@anuraaga anuraaga requested a review from Oberon00 as a code owner February 28, 2022 03:33
@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #4218 (2895c91) into main (963bc38) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4218      +/-   ##
============================================
- Coverage     90.29%   90.25%   -0.04%     
+ Complexity     4754     4751       -3     
============================================
  Files           556      556              
  Lines         14609    14609              
  Branches       1402     1402              
============================================
- Hits          13191    13186       -5     
- Misses          959      963       +4     
- Partials        459      460       +1     
Impacted Files Coverage Δ
...io/opentelemetry/sdk/metrics/view/Aggregation.java 87.50% <ø> (-12.50%) ⬇️
...entelemetry/exporter/jaeger/PostSpansResponse.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/io/opentelemetry/sdk/internal/RateLimiter.java 94.11% <0.00%> (-5.89%) ⬇️
...exporter/jaeger/MarshalerCollectorServiceGrpc.java 85.71% <0.00%> (-4.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 963bc38...2895c91. Read the comment docs.

@jkwatson
Copy link
Contributor

approved, but should we wait on some commentary from spec folks before merging?

@anuraaga
Copy link
Contributor Author

I suspect it's worth removing now and adding back later if for some reason it survives the spec but either works.

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.

2 participants