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

feat: exponential histogram - part 1 - mapping functions #3504

Merged
merged 14 commits into from
Jan 27, 2023

Conversation

mwear
Copy link
Member

@mwear mwear commented Dec 22, 2022

Which problem is this PR solving?

This PR is part 1 in a series of PRs to add exponential histogram support.

Partially Fixes #3324

Short description of the changes

This PR adds the mapping functions that will be used map values to buckets across the range of usable scales. The scales range from -10 to 20 with higher numbers giving higher resolution. These are not user facing, and will be used internally by the ExponentialHistogramAccumulation to maintain the highest resolution given its max size and range of values observed. The histogram will use the ExponentMapping for scales [-10, 0] and the LogarithmMapping for scales [1, 20]. The Accumulation will start out at max resolution (scale 20) and downscale as needed. It's possible to fit the entire floating point range in two buckets at scale -10, which is the minimum size allowed size for the ExponentialHistogramAccumulation.

This code is heavily based on the Golang reference implementation. For other details see:

For the other PRs in this series see:

You can see all 3 PRs combined in the original draft PR

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Extensive unit testing
  • End to end test with the 3 PRs from this series adding ExponentialHistogram support

Checklist:

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

@mwear mwear requested a review from a team December 22, 2022 01:12
@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #3504 (94fd188) into main (b5ef0e4) will increase coverage by 0.07%.
The diff coverage is 99.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3504      +/-   ##
==========================================
+ Coverage   93.80%   93.88%   +0.07%     
==========================================
  Files         249      255       +6     
  Lines        7640     7754     +114     
  Branches     1589     1609      +20     
==========================================
+ Hits         7167     7280     +113     
- Misses        473      474       +1     
Impacted Files Coverage Δ
...ry-instrumentation-grpc/src/grpc-js/clientUtils.ts 92.85% <ø> (ø)
...elemetry-instrumentation-grpc/src/grpc-js/index.ts 92.30% <ø> (ø)
...metry-instrumentation-grpc/src/grpc/clientUtils.ts 89.18% <ø> (ø)
...entelemetry-instrumentation-grpc/src/grpc/index.ts 93.98% <ø> (ø)
...trics/src/aggregator/exponential-histogram/util.ts 87.50% <87.50%> (ø)
...r/exponential-histogram/mapping/ExponentMapping.ts 100.00% <100.00%> (ø)
.../exponential-histogram/mapping/LogarithmMapping.ts 100.00% <100.00%> (ø)
...egator/exponential-histogram/mapping/getMapping.ts 100.00% <100.00%> (ø)
...ggregator/exponential-histogram/mapping/ieee754.ts 100.00% <100.00%> (ø)
.../aggregator/exponential-histogram/mapping/types.ts 100.00% <100.00%> (ø)
... and 2 more

@mwear mwear force-pushed the expohisto-p1-mapping branch from 88cd0f3 to 7969c06 Compare December 22, 2022 01:17
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Mostly nits. Thanks for putting in the work on this.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Great work! 🚀 Thank you for implementing this 🙂
Was only able to come up with a few optional nits. 🙂

@dyladan dyladan merged commit 3bc93a9 into open-telemetry:main Jan 27, 2023
@dyladan dyladan mentioned this pull request Jan 27, 2023
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.

Add support for Exponential Histograms
3 participants