Skip to content
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: Add NewMemberRaw and NewKeyValuePropertyRaw #4804

Merged
merged 10 commits into from
Jan 10, 2024
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Add `WithResourceAsConstantLabels` option to apply resource attributes for every metric emitted by the Prometheus exporter. (#4733)
- Experimental cardinality limiting is added to the metric SDK.
See [metric documentation](./sdk/metric/EXPERIMENTAL.md#cardinality-limit) for more information about this feature and how to enable it. (#4457)
- Add `NewMemberRaw` and `NewKeyValuePropertyRaw` in `go.opentelemetry.io/otel/baggage`. (#4804)

### Changed

Expand All @@ -31,12 +32,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Improve `go.opentelemetry.io/otel/baggage` performance. (#4743)
- Improve performance of the `(*Set).Filter` method in `go.opentelemetry.io/otel/attribute` when the passed filter does not filter out any attributes from the set. (#4774)
- `Member.String` in `go.opentelemetry.io/otel/baggage` percent-encodes only when necessary. (#4775)
- `Property.Value` in `go.opentelemetry.io/otel/baggage` now returns a vanilla string instead of a percent-encoded value. (#4804)
pellared marked this conversation as resolved.
Show resolved Hide resolved

### Fixed

- Fix `Parse` in `go.opentelemetry.io/otel/baggage` to validate member value before percent-decoding. (#4755)
- Fix whitespace encoding of `Member.String` in `go.opentelemetry.io/otel/baggage`. (#4756)
- Fix baggage item key so that it is not canonicalized in `go.opentelemetry.io/otel/bridge/opentracing`. (#4776)
- Fix `go.opentelemetry.io/otel/bridge/opentracing` to properly handle baggage values that requires escaping during propagation. (#4804)

## [1.21.0/0.44.0] 2023-11-16

Expand Down
86 changes: 57 additions & 29 deletions baggage/baggage.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,29 @@

// NewKeyValueProperty returns a new Property for key with value.
//
// If key or value are invalid, an error will be returned.
// The passed key must be compliant with W3C Baggage specification.
// The passed value must be precent-encoded as defined in W3C Baggage specification.
//
// Notice: Consider using [NewKeyValuePropertyRaw] instead
// that does not require precent-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)
}
decodedValue, err := url.PathUnescape(value)
if err != nil {
return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value)
}

Check warning on line 81 in baggage/baggage.go

View check run for this annotation

Codecov / codecov/patch

baggage/baggage.go#L80-L81

Added lines #L80 - L81 were not covered by tests
return NewKeyValuePropertyRaw(key, decodedValue)
}

// NewKeyValuePropertyRaw returns a new Property for key with value.
//
// The passed key must be compliant with W3C Baggage specification.
func NewKeyValuePropertyRaw(key, value string) (Property, error) {
if !validateKey(key) {
return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key)
}

p := Property{
key: key,
Expand Down Expand Up @@ -113,9 +128,6 @@
if !validateKey(p.key) {
return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key))
}
if p.hasValue && !validateValue(p.value) {
return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value))
}
if !p.hasValue && p.value != "" {
return errFunc(errors.New("inconsistent value"))
}
Expand All @@ -134,11 +146,11 @@
return p.value, p.hasValue
}

// String encodes Property into a string compliant with the W3C Baggage
// String encodes Property into a header string compliant with the W3C Baggage
// specification.
func (p Property) String() string {
if p.hasValue {
return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, p.value)
return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, valueEscape(p.value))
pellared marked this conversation as resolved.
Show resolved Hide resolved
}
return p.key
}
Expand Down Expand Up @@ -198,7 +210,7 @@
return nil
}

// String encodes properties into a string compliant with the W3C Baggage
// String encodes properties into a header string compliant with the W3C Baggage
// specification.
func (p properties) String() string {
props := make([]string, len(p))
Expand All @@ -220,11 +232,28 @@
hasData bool
}

// NewMember returns a new Member from the passed arguments. The key will be
// used directly while the value will be url decoded after validation. An error
// is returned if the created Member would be invalid according to the W3C
// Baggage specification.
// NewMemberRaw returns a new Member from the passed arguments.
//
// The passed key must be compliant with W3C Baggage specification.
// The passed value must be precent-encoded as defined in W3C Baggage specification.
//
// Notice: Consider using [NewMemberRaw] instead
// that does not require precent-encoding of the value.
func NewMember(key, value string, props ...Property) (Member, error) {
if !validateValue(value) {
return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
}

Check warning on line 245 in baggage/baggage.go

View check run for this annotation

Codecov / codecov/patch

baggage/baggage.go#L244-L245

Added lines #L244 - L245 were not covered by tests
decodedValue, err := url.PathUnescape(value)
if err != nil {
return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
}
return NewMemberRaw(key, decodedValue, props...)
}

// NewMemberRaw returns a new Member from the passed arguments.
//
// The passed key must be compliant with W3C Baggage specification.
func NewMemberRaw(key, value string, props ...Property) (Member, error) {
m := Member{
key: key,
value: value,
Expand All @@ -234,11 +263,6 @@
if err := m.validate(); err != nil {
return newInvalidMember(), err
}
decodedValue, err := url.PathUnescape(value)
if err != nil {
return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
}
m.value = decodedValue
return m, nil
}

Expand Down Expand Up @@ -284,6 +308,8 @@
if !validateValue(val) {
return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, v)
}

// Decode a precent-encoded value.
value, err := url.PathUnescape(val)
if err != nil {
return newInvalidMember(), fmt.Errorf("%w: %v", errInvalidValue, err)
Expand All @@ -292,8 +318,7 @@
}

// validate ensures m conforms to the W3C Baggage specification.
// A key is just an ASCII string, but a value must be URL encoded UTF-8,
// returning an error otherwise.
// A key must be an ASCII string, returning an error otherwise.
func (m Member) validate() error {
if !m.hasData {
return fmt.Errorf("%w: %q", errInvalidMember, m)
Expand All @@ -302,9 +327,6 @@
if !validateKey(m.key) {
return fmt.Errorf("%w: %q", errInvalidKey, m.key)
}
if !validateValue(m.value) {
return fmt.Errorf("%w: %q", errInvalidValue, m.value)
}
return m.properties.validate()
}

Expand All @@ -317,7 +339,7 @@
// Properties returns a copy of the Member properties.
func (m Member) Properties() []Property { return m.properties.Copy() }

// String encodes Member into a string compliant with the W3C Baggage
// String encodes Member into a header string compliant with the W3C Baggage
// specification.
func (m Member) String() string {
// A key is just an ASCII string. A value is restricted to be
Expand Down Expand Up @@ -514,9 +536,8 @@
return len(b.list)
}

// String encodes Baggage into a string compliant with the W3C Baggage
// specification. The returned string will be invalid if the Baggage contains
// any invalid list-members.
// String encodes Baggage into a header string compliant with the W3C Baggage
// specification.
func (b Baggage) String() string {
members := make([]string, 0, len(b.list))
for k, v := range b.list {
Expand Down Expand Up @@ -595,10 +616,17 @@
return
}

// Decode a precent-encoded value.
value, err := url.PathUnescape(s[valueStart:valueEnd])
if err != nil {
return
}

Check warning on line 623 in baggage/baggage.go

View check run for this annotation

Codecov / codecov/patch

baggage/baggage.go#L622-L623

Added lines #L622 - L623 were not covered by tests

ok = true
p.key = s[keyStart:keyEnd]
p.hasValue = true
p.value = s[valueStart:valueEnd]

p.value = value
return
}

Expand Down
83 changes: 65 additions & 18 deletions baggage/baggage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func TestNewKeyProperty(t *testing.T) {
}

func TestNewKeyValueProperty(t *testing.T) {
p, err := NewKeyValueProperty(" ", "")
p, err := NewKeyValueProperty(" ", "value")
assert.ErrorIs(t, err, errInvalidKey)
assert.Equal(t, Property{}, p)

Expand All @@ -156,20 +156,27 @@ func TestNewKeyValueProperty(t *testing.T) {
assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p)
}

func TestNewKeyValuePropertyRaw(t *testing.T) {
p, err := NewKeyValuePropertyRaw(" ", "")
assert.ErrorIs(t, err, errInvalidKey)
assert.Equal(t, Property{}, p)

p, err = NewKeyValuePropertyRaw("key", "Witaj Świecie!")
assert.NoError(t, err)
assert.Equal(t, Property{key: "key", value: "Witaj Świecie!", hasValue: true}, p)
}

func TestPropertyValidate(t *testing.T) {
p := Property{}
assert.ErrorIs(t, p.validate(), errInvalidKey)

p.key = "k"
assert.NoError(t, p.validate())

p.value = ";"
p.value = "v"
assert.EqualError(t, p.validate(), "invalid property: inconsistent value")

p.hasValue = true
assert.ErrorIs(t, p.validate(), errInvalidValue)

p.value = "v"
assert.NoError(t, p.validate())
}

Expand Down Expand Up @@ -397,6 +404,15 @@ func TestBaggageParse(t *testing.T) {
"key1": {Value: "val%2,"},
},
},
{
name: "encoded property",
in: "key1=;bar=val%252%2C",
want: baggage.List{
"key1": {
Properties: []baggage.Property{{Key: "bar", HasValue: true, Value: "val%2,"}},
},
},
},
{
name: "encoded UTF-8 string",
in: "foo=%C4%85%C5%9B%C4%87",
Expand Down Expand Up @@ -528,6 +544,17 @@ func TestBaggageString(t *testing.T) {
"foo": {Value: "ąść"},
},
},
{
name: "Encoded property value",
out: "foo=;bar=%20",
baggage: baggage.List{
"foo": {
Properties: []baggage.Property{
{Key: "bar", Value: " ", HasValue: true},
},
},
},
},
{
name: "plus",
out: "foo=1+1",
Expand Down Expand Up @@ -846,9 +873,6 @@ func TestMemberValidation(t *testing.T) {
assert.ErrorIs(t, m.validate(), errInvalidKey)

m.key, m.value = "k", "\\"
assert.ErrorIs(t, m.validate(), errInvalidValue)

m.value = "v"
assert.NoError(t, m.validate())
}

Expand Down Expand Up @@ -891,6 +915,28 @@ func TestNewMember(t *testing.T) {
assert.Equal(t, expected, m)
}

func TestNewMemberRaw(t *testing.T) {
m, err := NewMemberRaw("", "")
assert.ErrorIs(t, err, errInvalidKey)
assert.Equal(t, Member{hasData: false}, m)

key, val := "k", "v"
p := Property{key: "foo"}
m, err = NewMemberRaw(key, val, p)
assert.NoError(t, err)
expected := Member{
key: key,
value: val,
properties: properties{{key: "foo"}},
hasData: true,
}
assert.Equal(t, expected, m)

// Ensure new member is immutable.
p.key = "bar"
assert.Equal(t, expected, m)
}

func TestPropertiesValidate(t *testing.T) {
p := properties{{}}
assert.ErrorIs(t, p.validate(), errInvalidKey)
Expand All @@ -904,22 +950,23 @@ func TestPropertiesValidate(t *testing.T) {

func TestMemberString(t *testing.T) {
// normal key value pair
member, _ := NewMember("key", "value")
member, _ := NewMemberRaw("key", "value")
memberStr := member.String()
assert.Equal(t, memberStr, "key=value")
// encoded key
member, _ = NewMember("key", "%3B%20")

// encoded value
member, _ = NewMemberRaw("key", "; ")
memberStr = member.String()
assert.Equal(t, memberStr, "key=%3B%20")
}

var benchBaggage Baggage

func BenchmarkNew(b *testing.B) {
mem1, _ := NewMember("key1", "val1")
mem2, _ := NewMember("key2", "val2")
mem3, _ := NewMember("key3", "val3")
mem4, _ := NewMember("key4", "val4")
mem1, _ := NewMemberRaw("key1", "val1")
mem2, _ := NewMemberRaw("key2", "val2")
mem3, _ := NewMemberRaw("key3", "val3")
mem4, _ := NewMemberRaw("key4", "val4")

b.ReportAllocs()
b.ResetTimer()
Expand All @@ -931,11 +978,11 @@ func BenchmarkNew(b *testing.B) {

var benchMember Member

func BenchmarkNewMember(b *testing.B) {
func BenchmarkNewMemberRaw(b *testing.B) {
b.ReportAllocs()

for i := 0; i < b.N; i++ {
benchMember, _ = NewMember("key", "value")
benchMember, _ = NewMemberRaw("key", "value")
}
}

Expand All @@ -950,7 +997,7 @@ func BenchmarkParse(b *testing.B) {
func BenchmarkString(b *testing.B) {
var members []Member
addMember := func(k, v string) {
m, err := NewMember(k, valueEscape(v))
m, err := NewMemberRaw(k, valueEscape(v))
require.NoError(b, err)
members = append(members, m)
}
Expand Down
2 changes: 1 addition & 1 deletion bridge/opentracing/bridge.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (c *bridgeSpanContext) ForeachBaggageItem(handler func(k, v string) bool) {
}

func (c *bridgeSpanContext) setBaggageItem(restrictedKey, value string) {
m, err := baggage.NewMember(restrictedKey, value)
m, err := baggage.NewMemberRaw(restrictedKey, value)
if err != nil {
return
}
Expand Down
Loading
Loading