baggage: Accept non-ASCII keys #5132
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
|
XSAM
requested review from
MrAlias,
Aneurysm9,
evantorrie,
dashpole,
MadVikingGod,
pellared,
hanyuancheung and
dmathieu
as code owners
| @@ -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.
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.
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.
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.
I created open-telemetry/opentelemetry-specification#3972 for tracking.
There was a problem hiding this comment.
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.
I missed this:
MetadataOptional 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.
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.
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.
Could we provide a method to expose baggage.List, so we could avoid memory allocation? (it is under "go.opentelemetry.io/otel/internal/baggage")
pellared
added
blocked:specification
resolves #4946
I also add additional test cases to cover more lines.