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

feat(exporters): update proto version and use otlp-transformer #2929

Merged
merged 32 commits into from
May 4, 2022

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Apr 26, 2022

Which problem is this PR solving?

The proto version was outdated, which causes problems with newer collector versions. This PR updates the proto version to 0.16, and makes use of the otlp-transformer package introduced in #2746.

Fixes #2886, #2675

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
    • Export will stop working for receivers that are using old OTLP versions.
  • This change requires a documentation update

How Has This Been Tested?

  • Adapted unit tests
  • Added unit tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #2929 (052eabb) into main (65aeff0) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2929      +/-   ##
==========================================
- Coverage   92.97%   92.88%   -0.09%     
==========================================
  Files         185      182       -3     
  Lines        6089     5906     -183     
  Branches     1304     1253      -51     
==========================================
- Hits         5661     5486     -175     
+ Misses        428      420       -8     
Impacted Files Coverage Δ
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 57.14% <ø> (ø)
...tal/packages/otlp-transformer/src/metrics/types.ts 100.00% <ø> (ø)
...ental/packages/otlp-transformer/src/trace/types.ts 100.00% <ø> (ø)
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...e-otlp-http/src/platform/node/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...exporter-trace-otlp-proto/src/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...-otlp-http/src/platform/node/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...tal/packages/otlp-transformer/src/metrics/index.ts 100.00% <100.00%> (ø)
... and 4 more

@pichlermarc pichlermarc marked this pull request as ready for review April 27, 2022 13:43
@pichlermarc pichlermarc requested a review from a team April 27, 2022 13:43
Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

overall lgtm, did you test with the collector ? if so, with which version of the collector ? I believe we need to update the readme for it to have a minimal compatible version, WDYT ?

@pichlermarc
Copy link
Member Author

overall lgtm, did you test with the collector ? if so, with which version of the collector ? I believe we need to update the readme for it to have a minimal compatible version, WDYT ?

I did test it with 0.49 (latest version) with oltp-http-json and otlp-http-proto. I will try it out grpc as well.
Having a minimum version in the readme is a good idea. I'm going to try out different versions, and will update the readme with a minimum once I found out which one that is. 🙂

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM.
left few minors about naming (instrumentationLibrary -> instrumentationScope), and about the fact that this PR will drop support for old collectors in OTLP-JSON.

Thanks for doing this important work

@pichlermarc
Copy link
Member Author

@vmarchaud I updated the READMEs to include minimum and maximum tested versions. 🙂

I found that with these updates

  • http/json will be compatible with >=0.48 up to 0.50 (latest)
  • http/proto will be compatible with >=0.32 up to 0.50 (latest)
  • grpc will be compatible with >=0.16 up to 0.50 (latest)

for both traces and metrics.

README.md Show resolved Hide resolved
Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM. There are few open discussions and nits but they are not blockers.

examples/otlp-exporter-node/README.md Outdated Show resolved Hide resolved
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just a few nits.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Once @legendecas's nit is resolved this can be merged IMO

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

This is a great job. Thank you!

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.

Update proto versions of OTLP exporters
7 participants