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?
Changes from 3 commits
c81a51e
bc9c5b8
dc96def
7d9d476
6501fba
a037804
9434b2f
171b0db
ce67af3
e864429
097d8a2
d716cec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |
"fmt" | ||
"net/url" | ||
"strings" | ||
"unicode/utf8" | ||
|
||
"go.opentelemetry.io/otel/internal/baggage" | ||
) | ||
|
@@ -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. | ||
// However, the specific Propagators that are used to transmit baggage entries across | ||
// component boundaries may impose their own restrictions on baggage key. | ||
Comment on lines
+272
to
+273
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel we should allow However, the W3C baggage propagator uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we provide a method to expose |
||
// For example, the W3C Baggage specification restricts the baggage keys to strings that | ||
// satisfy the token definition from RFC7230, Section 3.2.6. | ||
// For maximum compatibility, alpha-numeric value are strongly recommended to be used as baggage key. | ||
func NewMemberRaw(key, value string, props ...Property) (Member, error) { | ||
m := Member{ | ||
key: key, | ||
|
@@ -313,9 +319,12 @@ func (m Member) validate() error { | |
return fmt.Errorf("%w: %q", errInvalidMember, m) | ||
} | ||
|
||
if !validateKey(m.key) { | ||
if !validateBaggageName(m.key) { | ||
return fmt.Errorf("%w: %q", errInvalidKey, m.key) | ||
} | ||
if !validateBaggageValue(m.value) { | ||
return fmt.Errorf("%w: %q", errInvalidValue, m.value) | ||
} | ||
return m.properties.validate() | ||
} | ||
|
||
|
@@ -630,6 +639,23 @@ func skipSpace(s string, offset int) int { | |
return i | ||
} | ||
|
||
// validateBaggageName checks if the string is a valid OpenTelemetry Baggage name. | ||
// Baggage name is a valid UTF-8 string. | ||
func validateBaggageName(s string) bool { | ||
if len(s) == 0 { | ||
return false | ||
} | ||
|
||
return utf8.ValidString(s) | ||
} | ||
|
||
// validateBaggageValue checks if the string is a valid OpenTelemetry Baggage value. | ||
// Baggage value is a valid UTF-8 strings. | ||
func validateBaggageValue(s string) bool { | ||
return utf8.ValidString(s) | ||
} | ||
|
||
// validateKey checks if the string is a valid W3C Baggage key. | ||
func validateKey(s string) bool { | ||
if len(s) == 0 { | ||
return false | ||
|
@@ -658,6 +684,7 @@ func validateKeyChar(c int32) bool { | |
c == 0x7e | ||
} | ||
|
||
// validateValue checks if the string is a valid W3C Baggage value. | ||
func validateValue(s string) bool { | ||
for _, c := range s { | ||
if !validateValueChar(c) { | ||
|
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
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.
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:
So actually, I think it should have similar behavior and it is not actually blocked by specification.