-
Notifications
You must be signed in to change notification settings - Fork 851
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
Make the baggage API fully functional in the API #1822
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1822 +/- ##
============================================
+ Coverage 85.47% 85.58% +0.11%
+ Complexity 1448 1434 -14
============================================
Files 177 174 -3
Lines 5589 5556 -33
Branches 580 579 -1
============================================
- Hits 4777 4755 -22
+ Misses 612 601 -11
Partials 200 200
Continue to review full report at Codecov.
|
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.
Mostly good, just one point about visibility - presume since we have an interface, we don't need to make the implementation of baggage public
* @throws IllegalStateException if a specified manager (via system properties) could not be | ||
* found. | ||
*/ | ||
public static BaggageManager getBaggageManager() { |
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.
🎉
api/src/main/java/io/opentelemetry/baggage/ImmutableBaggage.java
Outdated
Show resolved
Hide resolved
Thanks! Can you rebase to get the build fix? |
and get rid of the BaggageManager that was needed previously.
cd01df9
to
b50ce5c
Compare
Late review, thanks for this! |
and get rid of the BaggageManager that was needed previously.
This moves the Baggage implementation to the API, per open-telemetry/opentelemetry-specification#1103, which is now merged.
I could split this up, if desired, into 2 parts: