Skip to content

Commit

Permalink
baggage: Add NewMemberRaw and NewKeyValuePropertyRaw (#4804)
Browse files Browse the repository at this point in the history
  • Loading branch information
pellared authored Jan 10, 2024
1 parent 6ead8d8 commit 259143a
Show file tree
Hide file tree
Showing 8 changed files with 150 additions and 59 deletions.
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 raw string instead of a percent-encoded value. (#4804)

### 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 @@ func NewKeyProperty(key string) (Property, error) {

// 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)
}
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 @@ func (p Property) validate() error {
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 @@ func (p Property) Value() (string, bool) {
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))
}
return p.key
}
Expand Down Expand Up @@ -198,7 +210,7 @@ func (p properties) validate() error {
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 @@ type Member struct {
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)
}
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 @@ func NewMember(key, value string, props ...Property) (Member, error) {
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 @@ func parseMember(member string) (Member, error) {
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 @@ func parseMember(member string) (Member, error) {
}

// 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 @@ func (m Member) validate() error {
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 @@ func (m Member) Value() string { return m.value }
// 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 @@ func (b Baggage) Len() int {
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 @@ func parsePropertyInternal(s string) (p Property, ok bool) {
return
}

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

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

0 comments on commit 259143a

Please sign in to comment.