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

Move exponential histogram mapping functions into top-level module #3159

Closed
wants to merge 3 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Sep 9, 2022

The exponential histogram mapping functions are not SDK-specific and have been until now included in the sdk/metric module, which is being swapped soon. Removing this non-SDK component allows it to be released without disruption as the metrics SDK is replaced.

Part of #2328 because that way the sdk/metric/aggregator directory can be removed completely.

@jmacd jmacd mentioned this pull request Sep 9, 2022
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #3159 (13d8ea0) into main (7aba25d) will decrease coverage by 0.0%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3159     +/-   ##
=======================================
- Coverage   76.3%   76.2%   -0.1%     
=======================================
  Files        180     180             
  Lines      11992   11992             
=======================================
- Hits        9153    9149      -4     
- Misses      2597    2601      +4     
  Partials     242     242             
Impacted Files Coverage Δ
data/exponential/mapping/exponent/exponent.go 100.0% <ø> (ø)
data/exponential/mapping/internal/float64.go 100.0% <ø> (ø)
data/exponential/mapping/logarithm/logarithm.go 100.0% <ø> (ø)
sdk/trace/batch_span_processor.go 80.2% <0.0%> (-0.9%) ⬇️
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️

@jmacd
Copy link
Contributor Author

jmacd commented Sep 9, 2022

Note that we agreed to make public exponential-histogram mapping functions available in #2501; the code in sdk/metric/aggregator/exponential/mapping is not meant to disappear with the new metrics SDK.

@jmacd
Copy link
Contributor Author

jmacd commented Sep 9, 2022

If we accept this PR, I will apply the same change of path to #3022 -- the mapping and structure packages are meant to be siblings.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

I do not think these packages have enough review or use to be included in a stable module just yet.

I think this package could fit into the new metric SDK.

@jmacd jmacd closed this Sep 9, 2022
@jmacd
Copy link
Contributor Author

jmacd commented Sep 9, 2022

FWIW, we are using these mapping functions extensively (and exclusively) inside Lightstep.

@jmacd
Copy link
Contributor Author

jmacd commented Sep 9, 2022

enough review

For the record, those reviews were #2982 and #2502.

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.

None yet

2 participants