-
Notifications
You must be signed in to change notification settings - Fork 891
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
Specify allowed characters for Baggage keys and values #3801
Specify allowed characters for Baggage keys and values #3801
Conversation
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Note: this could be a breaking change e.g. for Go SDK (open-telemetry/opentelemetry-go#3685), whose current implementation is wrong imo. |
Signed-off-by: Yuri Shkuro <github@ysh.us>
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.
Note: this could be a breaking change e.g. for Go SDK (open-telemetry/opentelemetry-go#3685), whose current implementation is wrong imo.
I would say that we chose the wrong decision in OTel Go. Still we might try to fix it in some backwards-compatible way (by extending the API).
TL;DR; For me this is not a blocker for this PR.
I am working towards making a fix in OTel Go: open-telemetry/opentelemetry-go#4804 |
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Java is also not compliant with this. Currently, we only propagate keys/values that meet this definition, but we allow arbitrary keys for in-memory usage. |
Signed-off-by: Yuri Shkuro <github@ysh.us>
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 support @pellared comment on different concerns (the in-process API and the inter-process propagation). Maybe we can adjust this PR to only mention that invalid keys MUST NOT be propagated? Then we can spin off a separate issue/PR discussing on (Also this would be easy to do as it would align with what some SIGs already do) |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Updated to allow arbitrary strings in the API, but caution that Propagators may not allow some keys. |
Co-authored-by: Robert Pająk <pellared@hotmail.com>
cc @open-telemetry/go-maintainers - the new wording makes Go implementation non-compliant, but it's not a breaking change for its API, you will just need to relax the checks (i.e. never return errors from |
Let's remind maintainers in the call next Monday about this, but otherwise we are fine to go. |
@carlosalberto are we good to merge? |
…y#3801) ## Changes * Add missing spec language on what keys and values are allowed for the baggage. * Emphasize case sensitivity * Definitions are aligned with W3C Baggage Spec, but provide a distinction between API representation and the wire encoding (done by W3C Propagator)
Changes