-
Notifications
You must be signed in to change notification settings - Fork 821
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
feat(exporters): update proto version and use otlp-transformer #2929
Conversation
dd69e8e
to
4d01b43
Compare
Codecov Report
@@ 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
|
…d spanId in browser exporter.
37a4073
to
78720de
Compare
There was a problem hiding this 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 ?
experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/src/OTLPMetricExporter.ts
Outdated
Show resolved
Hide resolved
...packages/opentelemetry-exporter-metrics-otlp-http/src/platform/browser/OTLPMetricExporter.ts
Show resolved
Hide resolved
I did test it with |
There was a problem hiding this 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
experimental/packages/exporter-trace-otlp-proto/test/traceHelper.ts
Outdated
Show resolved
Hide resolved
experimental/packages/exporter-trace-otlp-proto/test/traceHelper.ts
Outdated
Show resolved
Hide resolved
…ySpans -> scopeSpans
experimental/packages/exporter-trace-otlp-http/src/platform/browser/OTLPTraceExporter.ts
Outdated
Show resolved
Hide resolved
@vmarchaud I updated the READMEs to include minimum and maximum tested versions. 🙂 I found that with these updates
for both traces and metrics. |
There was a problem hiding this 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.
experimental/packages/exporter-trace-otlp-http/test/traceHelper.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
experimental/packages/exporter-trace-otlp-grpc/test/OTLPTraceExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/exporter-trace-otlp-grpc/test/OTLPTraceExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
There was a problem hiding this 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!
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 theotlp-transformer
package introduced in #2746.Fixes #2886, #2675
Type of change
non-breakingchange which fixes an issue)non-breakingchange which adds functionality)How Has This Been Tested?
Checklist: