baggage: Accept non-ASCII keys (#5132)

resolves #4946 I also add additional test cases to cover more lines. benchmark results: ``` goos: darwin goarch: arm64 pkg: go.opentelemetry.io/otel/baggage │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ New-10 402.3n ± 1% 422.4n ± 6% +4.98% (p=0.000 n=10) NewMemberRaw-10 10.82n ± 0% 13.90n ± 1% +28.51% (p=0.000 n=10) Parse-10 803.8n ± 1% 795.0n ± 1% -1.09% (p=0.011 n=10) String-10 682.6n ± 0% 610.0n ± 2% -10.63% (p=0.000 n=10) ValueEscape/nothing_to_escape-10 4.856n ± 0% 4.849n ± 0% ~ (p=0.279 n=10) ValueEscape/requires_escaping-10 22.47n ± 1% 22.36n ± 1% ~ (p=0.342 n=10) ValueEscape/long_value-10 513.3n ± 1% 510.1n ± 0% -0.62% (p=0.006 n=10) MemberString-10 430.8n ± 2% 471.3n ± 2% +9.41% (p=0.000 n=10) geomean 124.5n 128.5n +3.22% │ old.txt │ new.txt │ │ B/op │ B/op vs base │ New-10 704.0 ± 0% 704.0 ± 0% ~ (p=1.000 n=10) ¹ NewMemberRaw-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ Parse-10 888.0 ± 0% 888.0 ± 0% ~ (p=1.000 n=10) ¹ String-10 936.0 ± 0% 840.0 ± 0% -10.26% (p=0.000 n=10) ValueEscape/nothing_to_escape-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ValueEscape/requires_escaping-10 16.00 ± 0% 16.00 ± 0% ~ (p=1.000 n=10) ¹ ValueEscape/long_value-10 576.0 ± 0% 576.0 ± 0% ~ (p=1.000 n=10) ¹ MemberString-10 656.0 ± 0% 656.0 ± 0% ~ (p=1.000 n=10) ¹ geomean ² -1.34% ² ¹ all samples are equal ² summaries must be >0 to compute geomean │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ New-10 8.000 ± 0% 8.000 ± 0% ~ (p=1.000 n=10) ¹ NewMemberRaw-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ Parse-10 9.000 ± 0% 9.000 ± 0% ~ (p=1.000 n=10) ¹ String-10 10.000 ± 0% 8.000 ± 0% -20.00% (p=0.000 n=10) ValueEscape/nothing_to_escape-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ValueEscape/requires_escaping-10 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹ ValueEscape/long_value-10 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹ MemberString-10 4.000 ± 0% 4.000 ± 0% ~ (p=1.000 n=10) ¹ geomean ² -2.75% ² ¹ all samples are equal ² summaries must be >0 to compute geomean ``` FYI, the old implementation of `NewMemberRaw` didn't verify the value, so the benchmark result of `NewMemberRaw` is not an apple-to-apple comparison of `utf8.ValidString` and `validateKey`. --------- Co-authored-by: Chester Cheung <[email protected]> Co-authored-by: Robert Pająk <[email protected]>
open-telemetry · Aug 15, 2024 · d61bbf1 · d61bbf1
1 parent 1dc9522
commit d61bbf1
Show file tree

Hide file tree

Showing 3 changed files with 235 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - `SimpleProcessor` in `go.opentelemetry.io/otel/sdk/log` synchronizes `OnEmit` calls. (#5666)
 - The `SimpleProcessor` type in `go.opentelemetry.io/otel/sdk/log` is no longer comparable. (#5693)
 - The `BatchProcessor` type in `go.opentelemetry.io/otel/sdk/log` is no longer comparable. (#5693)
+- `NewMemberRaw`, `NewKeyProperty` and `NewKeyValuePropertyRaw` in `go.opentelemetry.io/otel/baggage` allow UTF-8 string in key. (#5132)
 
 ### Fixed
 

diff --git a/baggage/baggage.go b/baggage/baggage.go
@@ -44,9 +44,15 @@ type Property struct {
 
 // NewKeyProperty returns a new Property for key.
 //
+// The passed key must be valid, non-empty UTF-8 string.
 // If key is invalid, an error will be returned.
+// However, the specific Propagators that are used to transmit baggage entries across
+// component boundaries may impose their own restrictions on Property key.
+// For example, the W3C Baggage specification restricts the Property 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 Property key.
 func NewKeyProperty(key string) (Property, error) {
-	if !validateKey(key) {
+	if !validateBaggageName(key) {
 		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key)
 	}
 
@@ -62,6 +68,10 @@ func NewKeyProperty(key string) (Property, error) {
 // Notice: Consider using [NewKeyValuePropertyRaw] instead
 // that does not require percent-encoding of the value.
 func NewKeyValueProperty(key, value string) (Property, error) {
+	if !validateKey(key) {
+		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key)
+	}
+
 	if !validateValue(value) {
 		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value)
 	}
@@ -74,11 +84,20 @@ func NewKeyValueProperty(key, value string) (Property, error) {
 
 // NewKeyValuePropertyRaw returns a new Property for key with value.
 //
-// The passed key must be compliant with W3C Baggage specification.
+// The passed key must be valid, non-empty UTF-8 string.
+// The passed 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 Property key.
+// For example, the W3C Baggage specification restricts the Property 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 Property key.
 func NewKeyValuePropertyRaw(key, value string) (Property, error) {
-	if !validateKey(key) {
+	if !validateBaggageName(key) {
 		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key)
 	}
+	if !validateBaggageValue(value) {
+		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value)
+	}
 
 	p := Property{
 		key:      key,
@@ -115,12 +134,15 @@ func (p Property) validate() error {
 		return fmt.Errorf("invalid property: %w", err)
 	}
 
-	if !validateKey(p.key) {
+	if !validateBaggageName(p.key) {
 		return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key))
 	}
 	if !p.hasValue && p.value != "" {
 		return errFunc(errors.New("inconsistent value"))
 	}
+	if p.hasValue && !validateBaggageValue(p.value) {
+		return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value))
+	}
 	return nil
 }
 
@@ -138,7 +160,15 @@ func (p Property) Value() (string, bool) {
 
 // String encodes Property into a header string compliant with the W3C Baggage
 // specification.
+// It would return empty string if the key is invalid with the W3C Baggage
+// specification. This could happen for a UTF-8 key, as it may contain
+// invalid characters.
 func (p Property) String() string {
+	//  W3C Baggage specification does not allow percent-encoded keys.
+	if !validateKey(p.key) {
+		return ""
+	}
+
 	if p.hasValue {
 		return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, valueEscape(p.value))
 	}
@@ -203,9 +233,14 @@ func (p properties) validate() error {
 // String encodes properties into a header string compliant with the W3C Baggage
 // specification.
 func (p properties) String() string {
-	props := make([]string, len(p))
-	for i, prop := range p {
-		props[i] = prop.String()
+	props := make([]string, 0, len(p))
+	for _, prop := range p {
+		s := prop.String()
+
+		// Ignored empty properties.
+		if s != "" {
+			props = append(props, s)
+		}
 	}
 	return strings.Join(props, propertyDelimiter)
 }
@@ -230,6 +265,10 @@ type Member struct {
 // Notice: Consider using [NewMemberRaw] instead
 // that does not require percent-encoding of the value.
 func NewMember(key, value string, props ...Property) (Member, error) {
+	if !validateKey(key) {
+		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key)
+	}
+
 	if !validateValue(value) {
 		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
 	}
@@ -242,7 +281,13 @@ 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 must be valid, non-empty UTF-8 string.
+// The passed 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.
+// 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,
@@ -340,9 +385,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()
 }
 
@@ -357,10 +405,15 @@ func (m Member) Properties() []Property { return m.properties.Copy() }
 
 // String encodes Member into a header string compliant with the W3C Baggage
 // specification.
+// It would return empty string if the key is invalid with the W3C Baggage
+// specification. This could happen for a UTF-8 key, as it may contain
+// invalid characters.
 func (m Member) String() string {
-	// A key is just an ASCII string. A value is restricted to be
-	// US-ASCII characters excluding CTLs, whitespace,
-	// DQUOTE, comma, semicolon, and backslash.
+	//  W3C Baggage specification does not allow percent-encoded keys.
+	if !validateKey(m.key) {
+		return ""
+	}
+
 	s := m.key + keyValueDelimiter + valueEscape(m.value)
 	if len(m.properties) > 0 {
 		s += propertyDelimiter + m.properties.String()
@@ -554,14 +607,22 @@ func (b Baggage) Len() int {
 
 // String encodes Baggage into a header string compliant with the W3C Baggage
 // specification.
+// It would ignore members where the member key is invalid with the W3C Baggage
+// specification. This could happen for a UTF-8 key, as it may contain
+// invalid characters.
 func (b Baggage) String() string {
 	members := make([]string, 0, len(b.list))
 	for k, v := range b.list {
-		members = append(members, Member{
+		s := Member{
 			key:        k,
 			value:      v.Value,
 			properties: fromInternalProperties(v.Properties),
-		}.String())
+		}.String()
+
+		// Ignored empty members.
+		if s != "" {
+			members = append(members, s)
+		}
 	}
 	return strings.Join(members, listDelimiter)
 }
@@ -748,6 +809,24 @@ var safeKeyCharset = [utf8.RuneSelf]bool{
 	'~': true,
 }
 
+// validateBaggageName checks if the string is a valid OpenTelemetry Baggage name.
+// Baggage name is a valid, non-empty 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.
+// Empty string is also a valid UTF-8 string.
+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
@@ -766,6 +845,7 @@ func validateKeyChar(c int32) bool {
 	return c >= 0 && c < int32(utf8.RuneSelf) && safeKeyCharset[c]
 }
 
+// validateValue checks if the string is a valid W3C Baggage value.
 func validateValue(s string) bool {
 	for _, c := range s {
 		if !validateValueChar(c) {