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

Enable exporting Histogram aggregation to OTLP metric #1209

Merged
merged 6 commits into from
Sep 29, 2020

Conversation

evantorrie
Copy link
Contributor

This PR converts histogram aggregations into OTLP Histogram metrics.

I haven't changed the otlp_metric_integration_test.go yet. I believe that will require switching to use Histogram as the default aggregation for ValueRecorder as well.

@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #1209 into master will decrease coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1209     +/-   ##
========================================
- Coverage    76.6%   76.5%   -0.1%     
========================================
  Files         134     134             
  Lines        5760    5806     +46     
========================================
+ Hits         4414    4447     +33     
- Misses       1103    1110      +7     
- Partials      243     249      +6     
Impacted Files Coverage Δ
exporters/otlp/internal/transform/metric.go 84.3% <ø> (-3.0%) ⬇️
sdk/export/metric/aggregation/aggregation.go 100.0% <ø> (ø)

@jmacd
Copy link
Contributor

jmacd commented Sep 29, 2020

I believe that will require switching to use Histogram as the default aggregation for ValueRecorder as well.

That works for me, although it's not in the specification, neither is the current behavior.

open-telemetry/opentelemetry-specification#982 agrees.

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.

🚀

sdk/export/metric/aggregation/aggregation.go Show resolved Hide resolved
@MrAlias MrAlias merged commit 04297f4 into open-telemetry:master Sep 29, 2020
@Aneurysm9 Aneurysm9 mentioned this pull request Oct 8, 2020
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