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

Use uint64 Count consistently in metric aggregation #1430

Merged
merged 3 commits into from
Jan 6, 2021

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jan 2, 2021

The OTLP Export code path uses a uint64 for histogram bucket counts. This meant an extra copy because this code, which predates the OTLP definition, chose int64 and float64 inconsistently. This PR avoids the extra copy and uses uint64 for counts consistently across aggregators and histogram buckets.

The change from int64 to uint64 is not lossy because counts should never be negative.
The change from float64 to uint64 matters only in theory for a discussion about sampling in a statsd receiver.

[In the statsd receiver case, I'm prepared to argue for the use of integer-reciprocal sampling rates to mitigate this issue.]

@jmacd jmacd added the area:metrics Part of OpenTelemetry Metrics label Jan 2, 2021
@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #1430 (244389a) into master (3a337d0) will decrease coverage by 0.0%.
The diff coverage is 87.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1430     +/-   ##
========================================
- Coverage    78.6%   78.6%   -0.1%     
========================================
  Files         126     126             
  Lines        6369    6367      -2     
========================================
- Hits         5011    5009      -2     
  Misses       1114    1114             
  Partials      244     244             
Impacted Files Coverage Δ
sdk/export/metric/aggregation/aggregation.go 100.0% <ø> (ø)
sdk/metric/aggregator/minmaxsumcount/mmsc.go 96.0% <ø> (ø)
exporters/otlp/internal/transform/metric.go 80.5% <50.0%> (-0.2%) ⬇️
sdk/metric/aggregator/aggregatortest/test.go 85.0% <100.0%> (ø)
sdk/metric/aggregator/array/array.go 93.7% <100.0%> (ø)
sdk/metric/aggregator/ddsketch/ddsketch.go 86.0% <100.0%> (ø)
sdk/metric/aggregator/histogram/histogram.go 95.7% <100.0%> (ø)

@jmacd
Copy link
Contributor Author

jmacd commented Jan 5, 2021

@bogdandrutu Please review.

@jmacd jmacd merged commit fe9d1f7 into open-telemetry:master Jan 6, 2021
@jmacd jmacd deleted the jmacd/unsigned branch January 6, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants