diff --git a/CHANGELOG.md b/CHANGELOG.md index fb91daf7486..89df8a8c5a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Change the `otlpmetric.Client` interface's `UploadMetrics` method to accept a single `ResourceMetrics` instead of a slice of them. (#2491) - Specify explicit buckets in Prometheus example. (#2493) - W3C baggage will now decode urlescaped values. (#2529) +- Baggage members are now only validated once, when calling `NewMember` and not also when adding it to the baggage itself. (#2522) ### Removed diff --git a/baggage/baggage.go b/baggage/baggage.go index d52a298ed1b..824c67b27a3 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -61,45 +61,57 @@ type Property struct { // hasValue indicates if a zero-value value means the property does not // have a value or if it was the zero-value. hasValue bool + + // hasData indicates whether the created property contains data or not. + // Properties that do not contain data are invalid with no other check + // required. + hasData bool } func NewKeyProperty(key string) (Property, error) { - p := Property{} if !keyRe.MatchString(key) { - return p, fmt.Errorf("%w: %q", errInvalidKey, key) + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } - p.key = key + + p := Property{key: key, hasData: true} return p, nil } func NewKeyValueProperty(key, value string) (Property, error) { - p := Property{} if !keyRe.MatchString(key) { - return p, fmt.Errorf("%w: %q", errInvalidKey, key) + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } if !valueRe.MatchString(value) { - return p, fmt.Errorf("%w: %q", errInvalidValue, value) + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) + } + + p := Property{ + key: key, + value: value, + hasValue: true, + hasData: true, } - p.key = key - p.value = value - p.hasValue = true return p, nil } +func newInvalidProperty() Property { + return Property{} +} + // parseProperty attempts to decode a Property from the passed string. It // returns an error if the input is invalid according to the W3C Baggage // specification. func parseProperty(property string) (Property, error) { - p := Property{} if property == "" { - return p, nil + return newInvalidProperty(), nil } match := propertyRe.FindStringSubmatch(property) if len(match) != 4 { - return p, fmt.Errorf("%w: %q", errInvalidProperty, property) + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidProperty, property) } + p := Property{hasData: true} if match[1] != "" { p.key = match[1] } else { @@ -107,6 +119,7 @@ func parseProperty(property string) (Property, error) { p.value = match[3] p.hasValue = true } + return p, nil } @@ -117,6 +130,10 @@ func (p Property) validate() error { return fmt.Errorf("invalid property: %w", err) } + if !p.hasData { + return errFunc(fmt.Errorf("%w: %q", errInvalidProperty, p)) + } + if !keyRe.MatchString(p.key) { return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key)) } @@ -220,26 +237,40 @@ func (p properties) String() string { type Member struct { key, value string properties properties + + // hasData indicates whether the created property contains data or not. + // Properties that do not contain data are invalid with no other check + // required. + hasData bool } // NewMember returns a new Member from the passed arguments. An error is // returned if the created Member would be invalid according to the W3C // Baggage specification. func NewMember(key, value string, props ...Property) (Member, error) { - m := Member{key: key, value: value, properties: properties(props).Copy()} + m := Member{ + key: key, + value: value, + properties: properties(props).Copy(), + hasData: true, + } if err := m.validate(); err != nil { - return Member{}, err + return newInvalidMember(), err } return m, nil } +func newInvalidMember() Member { + return Member{} +} + // parseMember attempts to decode a Member from the passed string. It returns // an error if the input is invalid according to the W3C Baggage // specification. func parseMember(member string) (Member, error) { if n := len(member); n > maxBytesPerMembers { - return Member{}, fmt.Errorf("%w: %d", errMemberBytes, n) + return newInvalidMember(), fmt.Errorf("%w: %d", errMemberBytes, n) } var ( @@ -254,7 +285,7 @@ func parseMember(member string) (Member, error) { for _, pStr := range strings.Split(parts[1], propertyDelimiter) { p, err := parseProperty(pStr) if err != nil { - return Member{}, err + return newInvalidMember(), err } props = append(props, p) } @@ -265,7 +296,7 @@ func parseMember(member string) (Member, error) { // Take into account a value can contain equal signs (=). kv := strings.SplitN(parts[0], keyValueDelimiter, 2) if len(kv) != 2 { - return Member{}, fmt.Errorf("%w: %q", errInvalidMember, member) + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidMember, member) } // "Leading and trailing whitespaces are allowed but MUST be trimmed // when converting the header into a data structure." @@ -273,13 +304,13 @@ func parseMember(member string) (Member, error) { var err error value, err = url.QueryUnescape(strings.TrimSpace(kv[1])) if err != nil { - return Member{}, fmt.Errorf("%w: %q", err, value) + return newInvalidMember(), fmt.Errorf("%w: %q", err, value) } if !keyRe.MatchString(key) { - return Member{}, fmt.Errorf("%w: %q", errInvalidKey, key) + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key) } if !valueRe.MatchString(value) { - return Member{}, fmt.Errorf("%w: %q", errInvalidValue, value) + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) } default: // This should never happen unless a developer has changed the string @@ -288,12 +319,16 @@ func parseMember(member string) (Member, error) { panic("failed to parse baggage member") } - return Member{key: key, value: value, properties: props}, nil + return Member{key: key, value: value, properties: props, hasData: true}, nil } // validate ensures m conforms to the W3C Baggage specification, returning an // error otherwise. func (m Member) validate() error { + if !m.hasData { + return fmt.Errorf("%w: %q", errInvalidMember, m) + } + if !keyRe.MatchString(m.key) { return fmt.Errorf("%w: %q", errInvalidKey, m.key) } @@ -329,9 +364,10 @@ type Baggage struct { //nolint:golint list baggage.List } -// New returns a new valid Baggage. It returns an error if the passed members -// are invalid according to the W3C Baggage specification or if it results in -// a Baggage exceeding limits set in that specification. +// New returns a new valid Baggage. It returns an error if it results in a +// Baggage exceeding limits set in that specification. +// +// It expects all the provided members to have already been validated. func New(members ...Member) (Baggage, error) { if len(members) == 0 { return Baggage{}, nil @@ -339,9 +375,10 @@ func New(members ...Member) (Baggage, error) { b := make(baggage.List) for _, m := range members { - if err := m.validate(); err != nil { - return Baggage{}, err + if !m.hasData { + return Baggage{}, errInvalidMember } + // OpenTelemetry resolves duplicates by last-one-wins. b[m.key] = baggage.Item{ Value: m.value, @@ -406,6 +443,8 @@ func Parse(bStr string) (Baggage, error) { // // If there is no list-member matching the passed key the returned Member will // be a zero-value Member. +// The returned member is not validated, as we assume the validation happened +// when it was added to the Baggage. func (b Baggage) Member(key string) Member { v, ok := b.list[key] if !ok { @@ -413,7 +452,7 @@ func (b Baggage) Member(key string) Member { // where a zero-valued Member is included in the Baggage because a // zero-valued Member is invalid according to the W3C Baggage // specification (it has an empty key). - return Member{} + return newInvalidMember() } return Member{ @@ -425,6 +464,9 @@ func (b Baggage) Member(key string) Member { // Members returns all the baggage list-members. // The order of the returned list-members does not have significance. +// +// The returned members are not validated, as we assume the validation happened +// when they were added to the Baggage. func (b Baggage) Members() []Member { if len(b.list) == 0 { return nil @@ -448,8 +490,8 @@ func (b Baggage) Members() []Member { // If member is invalid according to the W3C Baggage specification, an error // is returned with the original Baggage. func (b Baggage) SetMember(member Member) (Baggage, error) { - if err := member.validate(); err != nil { - return b, fmt.Errorf("%w: %s", errInvalidMember, err) + if !member.hasData { + return b, errInvalidMember } n := len(b.list) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 88c820319b5..8cc4832ad00 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -138,7 +138,7 @@ func TestNewKeyProperty(t *testing.T) { p, err = NewKeyProperty("key") assert.NoError(t, err) - assert.Equal(t, Property{key: "key"}, p) + assert.Equal(t, Property{key: "key", hasData: true}, p) } func TestNewKeyValueProperty(t *testing.T) { @@ -152,11 +152,14 @@ func TestNewKeyValueProperty(t *testing.T) { p, err = NewKeyValueProperty("key", "value") assert.NoError(t, err) - assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p) + assert.Equal(t, Property{key: "key", value: "value", hasValue: true, hasData: true}, p) } func TestPropertyValidate(t *testing.T) { p := Property{} + assert.ErrorIs(t, p.validate(), errInvalidProperty) + + p.hasData = true assert.ErrorIs(t, p.validate(), errInvalidKey) p.key = "k" @@ -179,7 +182,7 @@ func TestNewEmptyBaggage(t *testing.T) { } func TestNewBaggage(t *testing.T) { - b, err := New(Member{key: "k"}) + b, err := New(Member{key: "k", hasData: true}) assert.NoError(t, err) assert.Equal(t, Baggage{list: baggage.List{"k": {}}}, b) } @@ -192,8 +195,9 @@ func TestNewBaggageWithDuplicates(t *testing.T) { for i := range m { // Duplicates are collapsed. m[i] = Member{ - key: "a", - value: fmt.Sprintf("%d", i), + key: "a", + value: fmt.Sprintf("%d", i), + hasData: true, } } b, err := New(m...) @@ -205,9 +209,9 @@ func TestNewBaggageWithDuplicates(t *testing.T) { assert.Equal(t, want, b) } -func TestNewBaggageErrorInvalidMember(t *testing.T) { - _, err := New(Member{key: ""}) - assert.ErrorIs(t, err, errInvalidKey) +func TestNewBaggageErrorEmptyMember(t *testing.T) { + _, err := New(Member{}) + assert.ErrorIs(t, err, errInvalidMember) } func key(n int) string { @@ -223,7 +227,7 @@ func key(n int) string { func TestNewBaggageErrorTooManyBytes(t *testing.T) { m := make([]Member, (maxBytesPerBaggageString/maxBytesPerMembers)+1) for i := range m { - m[i] = Member{key: key(maxBytesPerMembers)} + m[i] = Member{key: key(maxBytesPerMembers), hasData: true} } _, err := New(m...) assert.ErrorIs(t, err, errBaggageBytes) @@ -232,7 +236,7 @@ func TestNewBaggageErrorTooManyBytes(t *testing.T) { func TestNewBaggageErrorTooManyMembers(t *testing.T) { m := make([]Member, maxMembers+1) for i := range m { - m[i] = Member{key: fmt.Sprintf("%d", i)} + m[i] = Member{key: fmt.Sprintf("%d", i), hasData: true} } _, err := New(m...) assert.ErrorIs(t, err, errMemberNumber) @@ -533,7 +537,7 @@ func TestBaggageDeleteMember(t *testing.T) { assert.NotContains(t, b1.list, key) } -func TestBaggageSetMemberError(t *testing.T) { +func TestBaggageSetMemberEmpty(t *testing.T) { _, err := Baggage{}.SetMember(Member{}) assert.ErrorIs(t, err, errInvalidMember) } @@ -542,7 +546,7 @@ func TestBaggageSetMember(t *testing.T) { b0 := Baggage{} key := "k" - m := Member{key: key} + m := Member{key: key, hasData: true} b1, err := b0.SetMember(m) assert.NoError(t, err) assert.NotContains(t, b0.list, key) @@ -558,7 +562,7 @@ func TestBaggageSetMember(t *testing.T) { assert.Equal(t, 1, len(b1.list)) assert.Equal(t, 1, len(b2.list)) - p := properties{{key: "p"}} + p := properties{{key: "p", hasData: true}} m.properties = p b3, err := b2.SetMember(m) assert.NoError(t, err) @@ -569,12 +573,12 @@ func TestBaggageSetMember(t *testing.T) { // The returned baggage needs to be immutable and should use a copy of the // properties slice. - p[0] = Property{key: "different"} + p[0] = Property{key: "different", hasData: true} assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b3.list[key]) // Reset for below. - p[0] = Property{key: "p"} + p[0] = Property{key: "p", hasData: true} - m = Member{key: "another"} + m = Member{key: "another", hasData: true} b4, err := b3.SetMember(m) assert.NoError(t, err) assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b3.list[key]) @@ -664,7 +668,10 @@ func TestMemberProperties(t *testing.T) { } func TestMemberValidation(t *testing.T) { - m := Member{} + m := Member{hasData: false} + assert.ErrorIs(t, m.validate(), errInvalidMember) + + m.hasData = true assert.ErrorIs(t, m.validate(), errInvalidKey) m.key, m.value = "k", "\\" @@ -677,13 +684,18 @@ func TestMemberValidation(t *testing.T) { func TestNewMember(t *testing.T) { m, err := NewMember("", "") assert.ErrorIs(t, err, errInvalidKey) - assert.Equal(t, Member{}, m) + assert.Equal(t, Member{hasData: false}, m) key, val := "k", "v" - p := Property{key: "foo"} + p := Property{key: "foo", hasData: true} m, err = NewMember(key, val, p) assert.NoError(t, err) - expected := Member{key: key, value: val, properties: properties{{key: "foo"}}} + expected := Member{ + key: key, + value: val, + properties: properties{{key: "foo", hasData: true}}, + hasData: true, + } assert.Equal(t, expected, m) // Ensure new member is immutable. @@ -692,12 +704,46 @@ func TestNewMember(t *testing.T) { } func TestPropertiesValidate(t *testing.T) { - p := properties{{}} + p := properties{{hasData: true}} assert.ErrorIs(t, p.validate(), errInvalidKey) p[0].key = "foo" assert.NoError(t, p.validate()) - p = append(p, Property{key: "bar"}) + p = append(p, Property{key: "bar", hasData: true}) assert.NoError(t, p.validate()) } + +var benchBaggage Baggage + +func BenchmarkNew(b *testing.B) { + mem1, _ := NewMember("key1", "val1") + mem2, _ := NewMember("key2", "val2") + mem3, _ := NewMember("key3", "val3") + mem4, _ := NewMember("key4", "val4") + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + benchBaggage, _ = New(mem1, mem2, mem3, mem4) + } +} + +var benchMember Member + +func BenchmarkNewMember(b *testing.B) { + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + benchMember, _ = NewMember("key", "value") + } +} + +func BenchmarkParse(b *testing.B) { + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + benchBaggage, _ = Parse(`userId=alice,serverNode = DF28 , isProduction = false,hasProp=stuff;propKey;propWValue=value`) + } +}