Validate members once, in NewMember (#2522)

* use NewMember, or specify if the member is not validated when creating new ones

* expect members to already be validated when creating a new package

* add changelog entry

* add an isEmpty field to member and property for quick validation

* rename isEmpty to hasData

So by default, an empty struct really is marked as having no data

* Update baggage/baggage_test.go

Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com>

* don't validate the member in parseMember, we alredy ran that validation

We also don't want to use NewMember, as that runs the property
validation again, making the benchmark quite slower

* move changelog entry to the fixed section

* provide the member/property data when returning an invalid error

Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com>

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
 

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)

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`)
+	}
+}