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 ExponentialHistogram data to internal package. #4217

Merged
merged 4 commits into from
Mar 4, 2022

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Feb 25, 2022

We almost assuredly will mark metrics SDK stable without exponential histograms in the first version. So for now, we should move the code to an internal package.

I'm hoping not, but based on open-telemetry/opentelemetry-specification#2304 (comment) we may do similar for Exemplar if it's not finished for metrics SDK stable release. Optimistically leaving it in place for now.

@anuraaga
Copy link
Contributor Author

/cc @jamesmoessis

@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #4217 (a61024f) into main (a6107d5) will decrease coverage by 0.01%.
The diff coverage is 73.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4217      +/-   ##
============================================
- Coverage     90.28%   90.26%   -0.02%     
+ Complexity     4749     4748       -1     
============================================
  Files           553      553              
  Lines         14600    14602       +2     
  Branches       1402     1402              
============================================
- Hits          13182    13181       -1     
- Misses          958      963       +5     
+ Partials        460      458       -2     
Impacted Files Coverage Δ
.../metrics/ExponentialHistogramBucketsMarshaler.java 100.00% <ø> (ø)
...etrics/ExponentialHistogramDataPointMarshaler.java 100.00% <ø> (ø)
...al/otlp/metrics/ExponentialHistogramMarshaler.java 100.00% <ø> (ø)
...entelemetry/exporter/prometheus/MetricAdapter.java 91.17% <0.00%> (ø)
...dk/testing/assertj/ExponentialHistogramAssert.java 100.00% <ø> (ø)
...ing/assertj/ExponentialHistogramBucketsAssert.java 100.00% <ø> (ø)
...g/assertj/ExponentialHistogramPointDataAssert.java 100.00% <ø> (ø)
...elemetry/sdk/testing/assertj/MetricAssertions.java 66.66% <ø> (ø)
.../io/opentelemetry/sdk/metrics/data/MetricData.java 92.30% <ø> (+6.10%) ⬆️
...gregator/DoubleExponentialHistogramAggregator.java 100.00% <ø> (ø)
... and 14 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 a6107d5...a61024f. Read the comment docs.

Copy link
Contributor

@jamesmoessis jamesmoessis left a comment

Choose a reason for hiding this comment

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

LGTM, just not sure what the .attach_pid10327 file is. Is that supposed to be there?

@jkwatson
Copy link
Contributor

I approve..just need to put the "internal" scare text boilerplate on the public stuff moved in there.

DoubleExponentialHistogramData() {}
public static ExponentialHistogramData empty() {
return EMPTY;
}

/**
* Create a DoubleExponentialHistogramData.
Copy link
Contributor

Choose a reason for hiding this comment

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

javadoc drift here

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

looks good. a bit of javadoc drift with the renames, but other than that, good to go.

@anuraaga anuraaga merged commit 5cbfe9d into open-telemetry:main Mar 4, 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