-
Notifications
You must be signed in to change notification settings - Fork 459
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
fix(sdk): fix incorrect doc #1499
Conversation
@@ -1,45 +1,3 @@ | |||
//! # OpenTelemetry Baggage API |
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.
Removed this as it's duplicate with the BaggatePropagator
doc
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.
Thanks.
Separately, https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry/src/baggage.rs#L393-L399 has some extra things like BaggageMetadata
that are not in spec.. Need to review them to ensure its needed.
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.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#propagators-distribution
Also, the propagators should be part of the API itself..
Not a blocker for this PR, will track separately.
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 there a ticket to track?
Also can't we reference it here without needing to duplicate?
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.
No, haven;t create an issue to track yet.
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.
Also can't we reference it here without needing to duplicate?
We don' really expose this module(we export the BaggagePropagator
directly) so users won't see it.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1499 +/- ##
=====================================
Coverage 63.6% 63.6%
=====================================
Files 144 144
Lines 20005 20005
=====================================
Hits 12741 12741
Misses 7264 7264 ☔ View full report in Codecov by Sentry. |
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.
some nits, not blockers
Fixes #1474, #1494
Design discussion issue (if applicable) #
Changes
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes