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

Factor the exponential histogram into a re-usable package #222

Merged
merged 11 commits into from
Jul 18, 2022

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jul 15, 2022

Description: The data structure is separated from the adapter used by the Lightstep metrics SDK. This will allow the entire structure package to be migrated into the upstream repository and referenced from the OTel Collector.

This causes minor churn:

  1. there was a pattern across all aggregators having an extra/unused Storage generic parameter. It's removed in 4 places.
  2. The SDK's use of JSON to parse instrument hints could not be preserved in the move upstream; I created a new config type and adapted the code to match style w/ the upstream repository; new JSON-specific types are used and converted into functional-option-generated configs.

Testing: Minor new tests added to preserve test coverage in both packages.

@jmacd jmacd requested a review from paivagustavo July 15, 2022 06:47
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

Given this is mostly renaming and little code change it looks good to me. The structure package makes the import a bit longer which I don't love, but I do like the separation you introduce as it leads to the histogram to be much simpler. Are there any concerns with removing the storage parameter (I know it was unused, but was curious)

@jmacd
Copy link
Contributor Author

jmacd commented Jul 18, 2022

I intend to do a bit more work here. @paivagustavo in a previous code review asked for an optimization in Move(). It requires a Swap() operation to do this right, and I'd like to take the opportunity now.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 18, 2022

(I know it was unused, but was curious)

(The Storage parameter was accidental redundancy, completely unnecessary and an artifact of me learning generics.)

@jmacd
Copy link
Contributor Author

jmacd commented Jul 18, 2022

It's a long import name!

The plan is for github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/histogram/structure to become go.opentelemetry.io/otel/sdk/metric/aggregator/exponential/structure, where it will be a sibling of the go.opentelemetry.io/otel/sdk/metric/aggregator/exponential/mapping library which was merged previously.

Joshua MacDonald added 2 commits July 18, 2022 09:59
@codecov-commenter
Copy link

Codecov Report

Merging #222 (0a34d83) into main (b60529d) will decrease coverage by 0.00%.
The diff coverage is 97.82%.

@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
- Coverage   94.26%   94.25%   -0.01%     
==========================================
  Files          62       64       +2     
  Lines        3312     3360      +48     
==========================================
+ Hits         3122     3167      +45     
- Misses        145      148       +3     
  Partials       45       45              
Impacted Files Coverage Δ
lightstep/sdk/metric/aggregator/aggregator.go 87.50% <77.77%> (-12.50%) ⬇️
lightstep/sdk/metric/aggregator/gauge/gauge.go 90.56% <90.00%> (ø)
...metric/aggregator/minmaxsumcount/minmaxsumcount.go 94.28% <90.00%> (ø)
...tstep/sdk/metric/aggregator/histogram/histogram.go 96.72% <98.21%> (-1.45%) ⬇️
...tric/aggregator/histogram/structure/exponential.go 96.55% <99.20%> (ø)
...dk/metric/aggregator/histogram/structure/config.go 100.00% <100.00%> (ø)
.../sdk/metric/aggregator/histogram/structure/test.go 100.00% <100.00%> (ø)
lightstep/sdk/metric/aggregator/sum/sum.go 100.00% <100.00%> (ø)
...ghtstep/sdk/metric/internal/viewstate/viewstate.go 98.63% <100.00%> (+<0.01%) ⬆️
lightstep/sdk/metric/view/views.go 100.00% <100.00%> (ø)
... and 1 more

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 b60529d...0a34d83. Read the comment docs.

@jmacd jmacd merged commit aa82048 into main Jul 18, 2022
@jmacd jmacd deleted the jmacd/factorhisto branch July 18, 2022 17:35
jmacd pushed a commit to jmacd/opentelemetry-go that referenced this pull request Jul 18, 2022
jmacd pushed a commit to lightstep/go-expohisto that referenced this pull request Oct 5, 2022
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