-
Notifications
You must be signed in to change notification settings - Fork 13
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: use OTLP gRPC exporter by default #319
Conversation
@theletterf, could you please proofread the changed docs? |
fea747f
to
1716ea5
Compare
Codecov Report
@@ Coverage Diff @@
## main #319 +/- ##
==========================================
+ Coverage 93.82% 93.89% +0.07%
==========================================
Files 10 10
Lines 340 344 +4
Branches 88 89 +1
==========================================
+ Hits 319 323 +4
Misses 21 21
Continue to review full report at Codecov.
|
Nice! Thanks for all the copy suggestions, @theletterf! |
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
c91745e
to
70b8e50
Compare
Actually already a dep of the grpc exporter, but required to satisfy the lint.
package.json
Outdated
@@ -90,10 +90,11 @@ | |||
"winston": "3.3.3" | |||
}, | |||
"dependencies": { | |||
"@grpc/grpc-js": "^1.3.7", |
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.
Can we be sure future versions of exporter-collector-grpc
will have a compatible version of @grpc/grpc-js
? Is the dependency explicitly needed if the exporter depends on one?
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.
The version of this should be updated with the exporter - this range is what was specified on the exporter.
The dep isn't explicitly needed, added it when lint pointed that out. Although, the only real reason to actually do that in our case is to avoid a situation where the exporter removes the dep which breaks us. Even if we do add the dep, it causes undefined behaviour because the exporter will not be compatible anymore.
Removing the dep and adding the lint exception makes more sense to me now - WDYT?
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.
See bfb7d93.
❗ Integration tests cannot work on this PR - those are using the latest published version of the package, which does not understand gRPC configuration which we are using for both e2e test variants. ❗
Description
Replacing OTLP/HTTP exporters with OTLP/gRPC(for the main default) and Jaeger(for collectorless) tandem.
Type of change
npm run change:new
)