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

Implement Conversion Between OC and OTel Summary Metric #2048

Conversation

JasonXZLiu
Copy link
Member

@JasonXZLiu JasonXZLiu commented Nov 1, 2020

Note: This PR is based off the Summary datapoint proto (that has not been merged upstream) in this PR.

Description:

This PR has two main parts:

  • Implement the Summary datapoint proto in the OTel Collector. Changes were made to metrics_struct.go to create the generated Summary structs (using the make genproto and make genpdata commands).
  • Add in the conversion between OpenCensus summary metrics and OTel Summary datapoints. This conversion directly adds compatibility for the Prometheus Summary metric in the PrometheusReceiver.

Link to tracking Issue: opentelemetry-specification/issues/1146 and opentelemetry-collector/issues/1638

A lot of the code in this PR is auto-generated from creating the structs and protos. The following files are were auto-generated:

  • generated_metrics.go
  • generated_metrics_test.go
  • trace_config.pb.go
  • metrics.pg.go

Testing:

  • Updated unit tests in oc_to_metrics.go for conversion between OC summary metrics to OTel summary metrics
  • Created end-to-end tests for the Summary metric in PrometheusReceiver

cc - @alolita

@codecov
Copy link

codecov bot commented Nov 1, 2020

Codecov Report

Merging #2048 into master will increase coverage by 0.11%.
The diff coverage is 96.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2048      +/-   ##
==========================================
+ Coverage   92.04%   92.15%   +0.11%     
==========================================
  Files         278      278              
  Lines       16831    17120     +289     
==========================================
+ Hits        15492    15777     +285     
- Misses        921      923       +2     
- Partials      418      420       +2     
Impacted Files Coverage Δ
translator/internaldata/metrics_to_oc.go 90.56% <86.95%> (-0.62%) ⬇️
translator/internaldata/oc_to_metrics.go 91.77% <88.23%> (-0.62%) ⬇️
consumer/pdata/generated_metrics.go 100.00% <100.00%> (ø)
consumer/pdata/metric.go 99.37% <100.00%> (+0.05%) ⬆️
internal/data/testdata/metric.go 100.00% <100.00%> (ø)
receiver/otlpreceiver/otlp.go 92.30% <0.00%> (+2.56%) ⬆️
translator/internaldata/resource_to_oc.go 94.52% <0.00%> (+5.47%) ⬆️

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 15d2f5b...039b6bc. Read the comment docs.

Copy link
Contributor

@rakyll rakyll left a comment

Choose a reason for hiding this comment

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

Leaving the data conversion related review to the approvers. I left some high level questions and suggestions.

consumer/pdata/metric.go Show resolved Hide resolved
internal/data/testdata/metric.go Outdated Show resolved Hide resolved
exporter/loggingexporter/logging_exporter_test.go Outdated Show resolved Hide resolved
internal/data/testdata/metric.go Show resolved Hide resolved
@bogdandrutu
Copy link
Member

This needs to wait for the proto change to be merged.

@JasonXZLiu JasonXZLiu force-pushed the implement-summary-metric-conversion branch from 61105ad to 039b6bc Compare November 4, 2020 19:37
@bogdandrutu
Copy link
Member

@tigrannajaryan do you suggest to not update now? This blocks some important feature for AWS.

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.

4 participants