-
Notifications
You must be signed in to change notification settings - Fork 848
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
Migrate usage of gRPC context to OTel context. #1751
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1751 +/- ##
============================================
+ Coverage 85.21% 85.56% +0.34%
- Complexity 1351 1354 +3
============================================
Files 166 165 -1
Lines 5276 5265 -11
Branches 549 549
============================================
+ Hits 4496 4505 +9
+ Misses 577 560 -17
+ Partials 203 200 -3
Continue to review full report at Codecov.
|
In-process propagation leverages [gRPC Context](https://grpc.github.io/grpc-java/javadoc/io/grpc/Context.html), | ||
a well established context propagation library, contained in a small artifact, which is non-dependent on the | ||
entire gRPC engine. | ||
|
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.
Is it worth replacing this with a comment about our own context implementation, rather than just deleting it?
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.
I wasn't sure how to phrase it. I read this sentence as justifying the external dependency because it is "a small artifact, which is non-dependent on the entire gRPC engine." for people that might worry about that, rather than anything particularly interesting. Let me know if you have any suggestion on how to replace this, I'll put it in.
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.
it's fine for now. I was just wondering if people might be interested in how our in-process propagation was implemented.
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.
Merging! 🎉 |
Merging should be held off until next release to give this code some time to bake in snapshots but sending it out now.
Fixes #575