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
baggage: Accept non-ASCII keys #5132
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5132 +/- ##
=======================================
- Coverage 84.5% 83.7% -0.9%
=======================================
Files 258 252 -6
Lines 17348 16466 -882
=======================================
- Hits 14674 13790 -884
- Misses 2363 2387 +24
+ Partials 311 289 -22
|
@@ -241,7 +242,12 @@ func NewMember(key, value string, props ...Property) (Member, error) { | |||
|
|||
// NewMemberRaw returns a new Member from the passed arguments. | |||
// | |||
// The passed key must be compliant with W3C Baggage specification. | |||
// The passed key and value must be valid UTF-8 string. |
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.
NewPropertyRaw
(and tests) should be updated as well.
The name/key passed to NewPropertyRaw
can be any valid UTF-8 string.
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 am unsure about this, as the property is defined in the W3C baggage, not the otel specification. And, the W3C didn't allow UTF-8 in the key.
https://www.w3.org/TR/baggage/#key
Additional metadata MAY be appended to values in the form of property set, represented as semi-colon ; delimited list of keys and/or key-value pairs, e.g. ;k1=v1;k2;k3=v3. Property keys and values are given no specific meaning by this specification. Leading and trailing OWS is allowed and is not considered to be a part of the property key or value.
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.
property is defined in the W3C baggage, not the otel specification
You are right. The OTel spec does not even have the "Members" and "Property" distinction reflecting W3C definitions. I would even say that the OTel spec says "properties" to reflect "members" in W3C which makes it very confusing.
I feel we should update the OTel spec first. Are you able to follow-up on this?
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 created open-telemetry/opentelemetry-specification#3972 for tracking.
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 thought it was designed not to define "Property" from the spec, as it was only a W3C extension of baggage value.
But yeah, the spec can mention the W3C property somewhere to prevent confusion. I can follow up on this.
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 missed this:
Metadata
Optional metadata associated with the name-value pair. This should be an opaque wrapper for a string with no semantic meaning. Left opaque to allow for future functionality.
So actually, I think it should have similar behavior and it is not actually blocked by specification.
// However, the specific Propagators that are used to transmit baggage entries across | ||
// component boundaries may impose their own restrictions on baggage key. |
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.
We should update Member.String()
and Property.String()
so that we do not encode them if its names are invalid according to W3C Baggage Propagator spec. Reference #4946 (comment)
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 feel we should allow Member.String()
to return percent-encoded UTF-8 from keys, as OTel baggage allows UTF-8 in keys. If we do not encode them, it will be strange, as the result of String
can miss some keys.
However, the W3C baggage propagator uses Member.String()
directly for encoding. Maybe we should let the W3C baggage propagator drop some members if the key is UTF-8. Though additional memory allocation could occur, I am not sure whether we should go for this path or not.
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.
Could we provide a method to expose baggage.List
, so we could avoid memory allocation? (it is under "go.opentelemetry.io/otel/internal/baggage")
resolves #4946
I also add additional test cases to cover more lines.