From cd720fb77ce1047e61f57c6731289cc7dedcc5eb Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 25 May 2021 08:33:37 -0700 Subject: [PATCH 01/20] Rename baggage context file --- baggage/{baggage.go => context.go} | 0 baggage/{baggage_test.go => context_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename baggage/{baggage.go => context.go} (100%) rename baggage/{baggage_test.go => context_test.go} (100%) diff --git a/baggage/baggage.go b/baggage/context.go similarity index 100% rename from baggage/baggage.go rename to baggage/context.go diff --git a/baggage/baggage_test.go b/baggage/context_test.go similarity index 100% rename from baggage/baggage_test.go rename to baggage/context_test.go From ef34b20b3172390c96eac11a970bfebaca102f9d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 27 May 2021 09:55:25 -0700 Subject: [PATCH 02/20] Initial baggage implementation --- baggage/baggage.go | 405 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 405 insertions(+) create mode 100644 baggage/baggage.go diff --git a/baggage/baggage.go b/baggage/baggage.go new file mode 100644 index 00000000000..250f2964f80 --- /dev/null +++ b/baggage/baggage.go @@ -0,0 +1,405 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package baggage + +import ( + "errors" + "fmt" + "regexp" + "strings" +) + +const ( + maxMembers = 180 + maxBytesPerMembers = 4096 + maxBytesPerBaggageString = 8192 + + listDelimiter = "," + propertyDelimiter = ";" + + keyDef = `([\x21\x23-\x27\x2A\x2B\x2D\x2E\x30-\x39\x41-\x5a\x5e-\x7a\x7c\x7e]+)` + valueDef = `([\x21\x23-\x2b\x2d-\x3a\x3c-\x5B\x5D-\x7e]*)` + keyValueDef = `\s*` + keyDef + `\s*=\s*` + valueDef + `\s*` +) + +var ( + keyRe = regexp.MustCompile(`^` + keyDef + `$`) + valueRe = regexp.MustCompile(`^` + valueDef + `$`) + keyValueRe = regexp.MustCompile(`^` + keyValueDef + `$`) + propertyRe = regexp.MustCompile(`^(?:\s*` + keyDef + `\s*|` + keyValueDef + `)$`) +) + +var ( + errInvalidProperty = errors.New("invalid baggage property") + errInvalidMember = errors.New("invalid baggage list-member") + errMemberNumber = errors.New("too many list-members in baggage-string") + errMemberBytes = errors.New("list-member too large") + errBaggageBytes = errors.New("baggage-string too large") +) + +// Property is an additional metadata entry for a baggage list-member. +type Property struct { + key, value string + + // hasValue indicates if a zero-value value means the property does not + // have a value or if it was the zero-value. + hasValue bool +} + +func NewKeyProperty(key string) (Property, error) { + p := Property{} + if !keyRe.MatchString(key) { + return p, fmt.Errorf("invalid key: %q", key) + } + p.key = key + return p, nil +} + +func NewKeyValueProperty(key, value string) (Property, error) { + p := Property{} + if !keyRe.MatchString(key) { + return p, fmt.Errorf("invalid key: %q", key) + } + if !valueRe.MatchString(value) { + return p, fmt.Errorf("invalid value: %q", value) + } + p.key = key + p.value = value + p.hasValue = true + return p, nil +} + +// 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 + } + + match := propertyRe.FindStringSubmatch(property) + if len(match) != 4 { + return p, fmt.Errorf("%w: %q", errInvalidProperty, property) + } + + if match[1] != "" { + p.key = match[1] + } else { + p.key = match[2] + p.value = match[3] + p.hasValue = true + } + return p, nil +} + +// validate ensures p conforms to the W3C Baggage specification, returning an +// error otherwise. +func (p Property) validate() error { + if !keyRe.MatchString(p.key) { + return fmt.Errorf("%w: invalid key: %q", errInvalidProperty, p.key) + } + if p.hasValue && !valueRe.MatchString(p.value) { + return fmt.Errorf("%w: invalid value: %q", errInvalidProperty, p.value) + } + if !p.hasValue && p.value != "" { + return fmt.Errorf("%w: inconsistent value", errInvalidProperty) + } + return nil +} + +// Key returns the Property key. +func (p Property) Key() string { + return p.key +} + +// Value returns the Property value. Additionally a boolean value is returned +// indicating if the returned value is the empty if the Property has a value +// that is empty or if the value is not set. +func (p Property) Value() (string, bool) { + return p.value, p.hasValue +} + +// String encodes Property into a string compliant with the W3C Baggage +// specification. +func (p Property) String() string { + if p.hasValue { + return fmt.Sprintf("%s=%v", p.key, p.value) + } + return p.key +} + +type properties []Property + +func (p properties) Copy() properties { + props := make(properties, len(p)) + copy(props, p) + return props +} + +// validate ensures each Property in p conforms to the W3C Baggage +// specification, returning an error otherwise. +func (p properties) validate() error { + for _, prop := range p { + if err := prop.validate(); err != nil { + return err + } + } + return nil +} + +// String encodes properties into a 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() + } + return strings.Join(props, propertyDelimiter) +} + +// Member is a list-member of a baggage-string as defined by the W3C Baggage +// specification. +type Member struct { + key, value string + properties properties +} + +func NewMember(key, value string, props ...Property) (Member, error) { + m := Member{} + if !keyRe.MatchString(key) { + return m, fmt.Errorf("invalid key: %q", key) + } + if !valueRe.MatchString(value) { + return m, fmt.Errorf("invalid value: %q", value) + } + p := properties(props) + if err := p.validate(); err != nil { + return m, err + } + + return Member{key: key, value: value, properties: p.Copy()}, nil +} + +// 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", errBaggageBytes, n) + } + + var ( + key, value string + props properties + ) + + parts := strings.SplitN(member, propertyDelimiter, 2) + switch len(parts) { + case 2: + for _, pStr := range strings.Split(parts[1], propertyDelimiter) { + p, err := parseProperty(pStr) + if err != nil { + return Member{}, err + } + props = append(props, p) + } + fallthrough + case 1: + match := keyValueRe.FindStringSubmatch(parts[0]) + if len(match) != 3 { + return Member{}, fmt.Errorf("%w: %q", errInvalidMember, member) + } + key, value = match[1], match[2] + default: + return Member{}, fmt.Errorf("%w: %q", errInvalidMember, member) + } + + return Member{key: key, value: value, properties: props}, nil +} + +// validate ensures m conforms to the W3C Baggage specification, returning an +// error otherwise. +func (m Member) validate() error { + if !keyRe.MatchString(m.key) { + return fmt.Errorf("%w: invalid key: %q", errInvalidMember, m.key) + } + if !valueRe.MatchString(m.value) { + return fmt.Errorf("%w: invalid value: %q", errInvalidMember, m.value) + } + return m.properties.validate() +} + +// Key returns the Member key. +func (m Member) Key() string { return m.key } + +// Value returns the Member value. +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 +// specification. +func (m Member) String() string { + s := fmt.Sprintf("%s=%s", m.key, m.value) + if len(m.properties) > 0 { + s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String()) + } + return s +} + +type value struct { + value string + properties properties +} + +// Baggage is a list of baggage members representing the baggage-string as +// defined by the W3C Baggage specification. +type Baggage struct { //nolint:golint + list map[string]value +} + +// Parse attempts to decode a baggage-string from the passed string. It +// returns an error if the input is invalid according to the W3C Baggage +// specification. +// +// If there are duplicate list-members contained in baggage, the last one +// defined (reading left-to-right) will be the only one kept. This diverges +// from the W3C Baggage specification which allows duplicate list-members, but +// conforms to the OpenTelemetry Baggage specification. +func Parse(baggage string) (Baggage, error) { + var b Baggage + if baggage == "" { + return b, nil + } + + if n := len(baggage); n > maxBytesPerBaggageString { + return b, fmt.Errorf("%w: %d", errBaggageBytes, n) + } + + for _, memberStr := range strings.Split(baggage, listDelimiter) { + // Trim empty baggage members. + if len(memberStr) == 0 { + continue + } + + m, err := parseMember(memberStr) + if err != nil { + return Baggage{}, err + } + + b.list[m.key] = value{m.value, m.properties} + if n := len(b.list); n > maxMembers { + return Baggage{}, errMemberNumber + } + } + + return b, nil +} + +// Member returns the baggage list-member identified by key and a boolean +// value indicating if the list-member existed or not. +func (b Baggage) Member(key string) (Member, bool) { + v, ok := b.list[key] + return Member{ + key: key, + value: v.value, + properties: v.properties.Copy(), + }, ok +} + +// Members returns all the baggage list-member. +// The order of the returned list-members does not have significance. +func (b Baggage) Members() []Member { + members := make([]Member, len(b.list)) + for key := range b.list { + // No need to verify the key exists. + m, _ := b.Member(key) + members = append(members, m) + } + return members +} + +// SetMember returns a copy the Baggage with the member included. If the +// baggage contains a Member with the same key the existing Member is +// replaced. +// +// 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, err + } + + n := len(b.list) + if _, ok := b.list[member.key]; !ok { + n++ + } + list := make(map[string]value, n) + + for k, v := range b.list { + // Update instead of just copy and overwrite. + if k == member.key { + list[member.key] = value{ + value: member.value, + properties: member.properties.Copy(), + } + continue + } + list[k] = v + } + + return Baggage{list: list}, nil +} + +// DeleteMember returns a copy of the Baggage with the list-member identified +// by key removed. +func (b Baggage) DeleteMember(key string) Baggage { + n := len(b.list) + if _, ok := b.list[key]; ok { + n-- + } + list := make(map[string]value, n) + + for k, v := range b.list { + if k == key { + continue + } + list[k] = v + } + + return Baggage{list: list} +} + +// Len returns the number of list-members in the Baggage. +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. +func (b Baggage) String() string { + members := make([]string, 0, len(b.list)) + for k, v := range b.list { + members = append(members, Member{ + key: k, + value: v.value, + properties: v.properties, + }.String()) + } + return strings.Join(members, listDelimiter) +} From 82ef78718bbefb89aa14eead26763382aa1cc813 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 27 May 2021 11:41:59 -0700 Subject: [PATCH 03/20] Initial tests --- baggage/baggage.go | 63 +++---- baggage/baggage_test.go | 392 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 424 insertions(+), 31 deletions(-) create mode 100644 baggage/baggage_test.go diff --git a/baggage/baggage.go b/baggage/baggage.go index 250f2964f80..e046b9f99e9 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -42,7 +42,9 @@ var ( ) var ( - errInvalidProperty = errors.New("invalid baggage property") + errInvalidKey = errors.New("invalid key") + errInvalidValue = errors.New("invalid value") + errInvalidProperty = errors.New("invalid baggage list-member property") errInvalidMember = errors.New("invalid baggage list-member") errMemberNumber = errors.New("too many list-members in baggage-string") errMemberBytes = errors.New("list-member too large") @@ -61,7 +63,7 @@ type Property struct { func NewKeyProperty(key string) (Property, error) { p := Property{} if !keyRe.MatchString(key) { - return p, fmt.Errorf("invalid key: %q", key) + return p, fmt.Errorf("%w: %q", errInvalidKey, key) } p.key = key return p, nil @@ -70,10 +72,10 @@ func NewKeyProperty(key string) (Property, error) { func NewKeyValueProperty(key, value string) (Property, error) { p := Property{} if !keyRe.MatchString(key) { - return p, fmt.Errorf("invalid key: %q", key) + return p, fmt.Errorf("%w: %q", errInvalidKey, key) } if !valueRe.MatchString(value) { - return p, fmt.Errorf("invalid value: %q", value) + return p, fmt.Errorf("%w: %q", errInvalidValue, value) } p.key = key p.value = value @@ -108,14 +110,18 @@ func parseProperty(property string) (Property, error) { // validate ensures p conforms to the W3C Baggage specification, returning an // error otherwise. func (p Property) validate() error { + errFunc := func(err error) error { + return fmt.Errorf("invalid property: %w", err) + } + if !keyRe.MatchString(p.key) { - return fmt.Errorf("%w: invalid key: %q", errInvalidProperty, p.key) + return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key)) } if p.hasValue && !valueRe.MatchString(p.value) { - return fmt.Errorf("%w: invalid value: %q", errInvalidProperty, p.value) + return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value)) } if !p.hasValue && p.value != "" { - return fmt.Errorf("%w: inconsistent value", errInvalidProperty) + return errFunc(errors.New("inconsistent value")) } return nil } @@ -198,7 +204,7 @@ func NewMember(key, value string, props ...Property) (Member, error) { // specification. func parseMember(member string) (Member, error) { if n := len(member); n > maxBytesPerMembers { - return Member{}, fmt.Errorf("%w: %d", errBaggageBytes, n) + return Member{}, fmt.Errorf("%w: %d", errMemberBytes, n) } var ( @@ -262,8 +268,8 @@ func (m Member) String() string { } type value struct { - value string - properties properties + v string + p properties } // Baggage is a list of baggage members representing the baggage-string as @@ -281,33 +287,28 @@ type Baggage struct { //nolint:golint // from the W3C Baggage specification which allows duplicate list-members, but // conforms to the OpenTelemetry Baggage specification. func Parse(baggage string) (Baggage, error) { - var b Baggage if baggage == "" { - return b, nil + return Baggage{}, nil } if n := len(baggage); n > maxBytesPerBaggageString { - return b, fmt.Errorf("%w: %d", errBaggageBytes, n) + return Baggage{}, fmt.Errorf("%w: %d", errBaggageBytes, n) } - for _, memberStr := range strings.Split(baggage, listDelimiter) { - // Trim empty baggage members. - if len(memberStr) == 0 { - continue - } + members := strings.Split(baggage, listDelimiter) + if len(members) > maxMembers { + return Baggage{}, errMemberNumber + } + b := make(map[string]value, len(members)) + for _, memberStr := range members { m, err := parseMember(memberStr) if err != nil { return Baggage{}, err } - - b.list[m.key] = value{m.value, m.properties} - if n := len(b.list); n > maxMembers { - return Baggage{}, errMemberNumber - } + b[m.key] = value{m.value, m.properties} } - - return b, nil + return Baggage{b}, nil } // Member returns the baggage list-member identified by key and a boolean @@ -316,8 +317,8 @@ func (b Baggage) Member(key string) (Member, bool) { v, ok := b.list[key] return Member{ key: key, - value: v.value, - properties: v.properties.Copy(), + value: v.v, + properties: v.p.Copy(), }, ok } @@ -354,8 +355,8 @@ func (b Baggage) SetMember(member Member) (Baggage, error) { // Update instead of just copy and overwrite. if k == member.key { list[member.key] = value{ - value: member.value, - properties: member.properties.Copy(), + v: member.value, + p: member.properties.Copy(), } continue } @@ -397,8 +398,8 @@ func (b Baggage) String() string { for k, v := range b.list { members = append(members, Member{ key: k, - value: v.value, - properties: v.properties, + value: v.v, + properties: v.p, }.String()) } return strings.Join(members, listDelimiter) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go new file mode 100644 index 00000000000..e2e496c501d --- /dev/null +++ b/baggage/baggage_test.go @@ -0,0 +1,392 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package baggage + +import ( + "fmt" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestKeyRegExp(t *testing.T) { + // ASCII only + invalidKeyRune := []rune{ + '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', + '\x08', '\x09', '\x0A', '\x0B', '\x0C', '\x0D', '\x0E', '\x0F', + '\x10', '\x11', '\x12', '\x13', '\x14', '\x15', '\x16', '\x17', + '\x18', '\x19', '\x1A', '\x1B', '\x1C', '\x1D', '\x1E', '\x1F', ' ', + '(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']', '?', + '=', '{', '}', '\x7F', + } + + for _, ch := range invalidKeyRune { + assert.NotRegexp(t, keyDef, fmt.Sprintf("%c", ch)) + } +} + +func TestValueRegExp(t *testing.T) { + // ASCII only + invalidValueRune := []rune{ + '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', + '\x08', '\x09', '\x0A', '\x0B', '\x0C', '\x0D', '\x0E', '\x0F', + '\x10', '\x11', '\x12', '\x13', '\x14', '\x15', '\x16', '\x17', + '\x18', '\x19', '\x1A', '\x1B', '\x1C', '\x1D', '\x1E', '\x1F', ' ', + '"', ',', ';', '\\', '\x7F', + } + + for _, ch := range invalidValueRune { + assert.NotRegexp(t, `^`+valueDef+`$`, fmt.Sprintf("invalid-%c-value", ch)) + } +} + +func TestParseProperty(t *testing.T) { + p := Property{key: "key", value: "value", hasValue: true} + + testcases := []struct { + in string + expected Property + }{ + { + in: "", + expected: Property{}, + }, + { + in: "key", + expected: Property{ + key: "key", + }, + }, + { + in: "key=", + expected: Property{ + key: "key", + hasValue: true, + }, + }, + { + in: "key=value", + expected: p, + }, + { + in: " key=value ", + expected: p, + }, + { + in: "key = value", + expected: p, + }, + { + in: " key = value ", + expected: p, + }, + { + in: "\tkey=value", + expected: p, + }, + } + + for _, tc := range testcases { + actual, err := parseProperty(tc.in) + + if !assert.NoError(t, err) { + continue + } + + assert.Equal(t, tc.expected.Key(), actual.Key(), tc.in) + + actualV, actualOk := actual.Value() + expectedV, expectedOk := tc.expected.Value() + assert.Equal(t, expectedOk, actualOk, tc.in) + assert.Equal(t, expectedV, actualV, tc.in) + } +} + +func TestParsePropertyError(t *testing.T) { + _, err := parseProperty(",;,") + assert.ErrorIs(t, err, errInvalidProperty) +} + +func TestNewKeyProperty(t *testing.T) { + p, err := NewKeyProperty(" ") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Property{}, p) + + p, err = NewKeyProperty("key") + assert.NoError(t, err) + assert.Equal(t, Property{key: "key"}, p) +} + +func TestNewKeyValueProperty(t *testing.T) { + p, err := NewKeyValueProperty(" ", "") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Property{}, p) + + p, err = NewKeyValueProperty("key", ";") + assert.ErrorIs(t, err, errInvalidValue) + assert.Equal(t, Property{}, p) + + p, err = NewKeyValueProperty("key", "value") + assert.NoError(t, err) + assert.Equal(t, Property{key: "key", value: "value", 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 = ";" + 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()) +} + +func TestBaggageParse(t *testing.T) { + b := make([]rune, maxBytesPerBaggageString+1) + for i := range b { + b[i] = 'a' + } + tooLarge := string(b) + + b = make([]rune, maxBytesPerMembers+1) + for i := range b { + b[i] = 'a' + } + tooLargeMember := string(b) + + m := make([]string, maxMembers+1) + for i := range m { + m[i] = "a=" + } + tooManyMembers := strings.Join(m, listDelimiter) + + testcases := []struct { + name string + in string + baggage map[string]value + err error + }{ + { + name: "empty value", + in: "", + baggage: map[string]value(nil), + }, + { + name: "single member empty value no properties", + in: "foo=", + baggage: map[string]value{ + "foo": {v: ""}, + }, + }, + { + name: "single member no properties", + in: "foo=1", + baggage: map[string]value{ + "foo": {v: "1"}, + }, + }, + { + name: "single member empty value with properties", + in: "foo=;state=on;red", + baggage: map[string]value{ + "foo": { + v: "", + p: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + }, + }, + { + name: "single member with properties", + in: "foo=1;state=on;red", + baggage: map[string]value{ + "foo": { + v: "1", + p: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + }, + }, + { + name: "two members with properties", + in: "foo=1;state=on;red,bar=2;yellow", + baggage: map[string]value{ + "foo": { + v: "1", + p: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + "bar": { + v: "2", + p: properties{{key: "yellow"}}, + }, + }, + }, + { + // According to the OTel spec, last value wins. + name: "duplicate key", + in: "foo=1;state=on;red,foo=2", + baggage: map[string]value{ + "foo": {v: "2"}, + }, + }, + { + name: "invalid member: empty key", + in: "foo=,,bar=", + err: errInvalidMember, + }, + { + name: "invalid member: invalid value", + in: "foo=\\", + err: errInvalidMember, + }, + { + name: "invalid property: invalid key", + in: "foo=1;=v", + err: errInvalidProperty, + }, + { + name: "invalid property: invalid value", + in: "foo=1;key=\\", + err: errInvalidProperty, + }, + { + name: "invalid baggage string: too large", + in: tooLarge, + err: errBaggageBytes, + }, + { + name: "invalid baggage string: member too large", + in: tooLargeMember, + err: errMemberBytes, + }, + { + name: "invalid baggage string: too many members", + in: tooManyMembers, + err: errMemberNumber, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + actual, err := Parse(tc.in) + assert.ErrorIs(t, err, tc.err) + assert.Equal(t, Baggage{list: tc.baggage}, actual) + }) + } +} + +func TestBaggageString(t *testing.T) { + testcases := []struct { + name string + out string + baggage map[string]value + }{ + { + name: "empty value", + out: "", + baggage: map[string]value(nil), + }, + { + name: "single member empty value no properties", + out: "foo=", + baggage: map[string]value{ + "foo": {v: ""}, + }, + }, + { + name: "single member no properties", + out: "foo=1", + baggage: map[string]value{ + "foo": {v: "1"}, + }, + }, + { + name: "single member empty value with properties", + out: "foo=;red;state=on", + baggage: map[string]value{ + "foo": { + v: "", + p: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + }, + }, + { + name: "single member with properties", + out: "foo=1;red;state=on", + baggage: map[string]value{ + "foo": { + v: "1", + p: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + }, + }, + { + name: "two members with properties", + out: "foo=1;red;state=on,bar=2;yellow", + baggage: map[string]value{ + "foo": { + v: "1", + p: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + "bar": { + v: "2", + p: properties{{key: "yellow"}}, + }, + }, + }, + } + + orderer := func(s string) string { + members := strings.Split(s, listDelimiter) + for i, m := range members { + parts := strings.Split(m, propertyDelimiter) + if len(parts) > 1 { + sort.Strings(parts[1:]) + members[i] = strings.Join(parts, propertyDelimiter) + } + } + return strings.Join(members, listDelimiter) + } + + for _, tc := range testcases { + b := Baggage{tc.baggage} + assert.Equal(t, tc.out, orderer(b.String())) + } +} From 9087850a6b2a46eb771dde688e8e57379505e369 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 27 May 2021 15:28:36 -0700 Subject: [PATCH 04/20] More tests --- baggage/baggage.go | 84 ++++++++++------- baggage/baggage_test.go | 199 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 248 insertions(+), 35 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index e046b9f99e9..d659c7d2a4e 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -150,6 +150,10 @@ func (p Property) String() string { type properties []Property func (p properties) Copy() properties { + if len(p) == 0 { + return nil + } + props := make(properties, len(p)) copy(props, p) return props @@ -183,20 +187,16 @@ type Member struct { properties properties } +// 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{} - if !keyRe.MatchString(key) { - return m, fmt.Errorf("invalid key: %q", key) - } - if !valueRe.MatchString(value) { - return m, fmt.Errorf("invalid value: %q", value) - } - p := properties(props) - if err := p.validate(); err != nil { - return m, err + m := Member{key: key, value: value, properties: properties(props).Copy()} + if err := m.validate(); err != nil { + return Member{}, err } - return Member{key: key, value: value, properties: p.Copy()}, nil + return m, nil } // parseMember attempts to decode a Member from the passed string. It returns @@ -230,7 +230,10 @@ func parseMember(member string) (Member, error) { } key, value = match[1], match[2] default: - return Member{}, fmt.Errorf("%w: %q", errInvalidMember, member) + // This should never happen unless a developer has changed the string + // splitting somehow. Panic instead of failing silently and allowing + // the bug to slip past the CI checks. + panic("failed to parse baggage member") } return Member{key: key, value: value, properties: props}, nil @@ -240,10 +243,10 @@ func parseMember(member string) (Member, error) { // error otherwise. func (m Member) validate() error { if !keyRe.MatchString(m.key) { - return fmt.Errorf("%w: invalid key: %q", errInvalidMember, m.key) + return fmt.Errorf("%w: %q", errInvalidKey, m.key) } if !valueRe.MatchString(m.value) { - return fmt.Errorf("%w: invalid value: %q", errInvalidMember, m.value) + return fmt.Errorf("%w: %q", errInvalidValue, m.value) } return m.properties.validate() } @@ -311,25 +314,37 @@ func Parse(baggage string) (Baggage, error) { return Baggage{b}, nil } -// Member returns the baggage list-member identified by key and a boolean -// value indicating if the list-member existed or not. -func (b Baggage) Member(key string) (Member, bool) { +// Member returns the baggage list-member identified by key. +// +// If there is no list-member matching the passed key the returned Member will +// be a zero-value Member. +func (b Baggage) Member(key string) Member { v, ok := b.list[key] - return Member{ - key: key, - value: v.v, - properties: v.p.Copy(), - }, ok + if !ok { + // We do not need to worry about distiguising between the situation + // 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 Member{key: key, value: v.v, properties: v.p.Copy()} } -// Members returns all the baggage list-member. +// Members returns all the baggage list-members. // The order of the returned list-members does not have significance. func (b Baggage) Members() []Member { - members := make([]Member, len(b.list)) - for key := range b.list { - // No need to verify the key exists. - m, _ := b.Member(key) - members = append(members, m) + if len(b.list) == 0 { + return nil + } + + members := make([]Member, 0, len(b.list)) + for k, v := range b.list { + members = append(members, Member{ + key: k, + value: v.v, + properties: v.p.Copy(), + }) } return members } @@ -342,7 +357,7 @@ func (b Baggage) Members() []Member { // is returned with the original Baggage. func (b Baggage) SetMember(member Member) (Baggage, error) { if err := member.validate(); err != nil { - return b, err + return b, fmt.Errorf("%w: %s", errInvalidMember, err) } n := len(b.list) @@ -352,17 +367,18 @@ func (b Baggage) SetMember(member Member) (Baggage, error) { list := make(map[string]value, n) for k, v := range b.list { - // Update instead of just copy and overwrite. + // Do not copy if we are just going to overwrite. if k == member.key { - list[member.key] = value{ - v: member.value, - p: member.properties.Copy(), - } continue } list[k] = v } + list[member.key] = value{ + v: member.value, + p: member.properties.Copy(), + } + return Baggage{list: list}, nil } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index e2e496c501d..ecd4cbaaa65 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -356,7 +356,7 @@ func TestBaggageString(t *testing.T) { }, { name: "two members with properties", - out: "foo=1;red;state=on,bar=2;yellow", + out: "bar=2;yellow,foo=1;red;state=on", baggage: map[string]value{ "foo": { v: "1", @@ -382,6 +382,7 @@ func TestBaggageString(t *testing.T) { members[i] = strings.Join(parts, propertyDelimiter) } } + sort.Strings(members) return strings.Join(members, listDelimiter) } @@ -390,3 +391,199 @@ func TestBaggageString(t *testing.T) { assert.Equal(t, tc.out, orderer(b.String())) } } + +func TestBaggageLen(t *testing.T) { + b := Baggage{} + assert.Equal(t, 0, b.Len()) + + b.list = make(map[string]value, 1) + assert.Equal(t, 0, b.Len()) + + b.list["k"] = value{} + assert.Equal(t, 1, b.Len()) +} + +func TestBaggageDeleteMember(t *testing.T) { + key := "k" + + b0 := Baggage{} + b1 := b0.DeleteMember(key) + assert.NotContains(t, b1.list, key) + + b0 = Baggage{list: map[string]value{ + key: {}, + "other": {}, + }} + b1 = b0.DeleteMember(key) + assert.Contains(t, b0.list, key) + assert.NotContains(t, b1.list, key) +} + +func TestBaggageSetMemberError(t *testing.T) { + _, err := Baggage{}.SetMember(Member{}) + assert.ErrorIs(t, err, errInvalidMember) +} + +func TestBaggageSetMember(t *testing.T) { + b0 := Baggage{} + + key := "k" + m := Member{key: key} + b1, err := b0.SetMember(m) + assert.NoError(t, err) + assert.NotContains(t, b0.list, key) + assert.Equal(t, value{}, b1.list[key]) + assert.Equal(t, 0, len(b0.list)) + assert.Equal(t, 1, len(b1.list)) + + m.value = "v" + b2, err := b1.SetMember(m) + assert.NoError(t, err) + assert.Equal(t, value{}, b1.list[key]) + assert.Equal(t, value{v: "v"}, b2.list[key]) + assert.Equal(t, 1, len(b1.list)) + assert.Equal(t, 1, len(b2.list)) + + p := properties{{key: "p"}} + m.properties = p + b3, err := b2.SetMember(m) + assert.NoError(t, err) + assert.Equal(t, value{v: "v"}, b2.list[key]) + assert.Equal(t, value{v: "v", p: p}, b3.list[key]) + assert.Equal(t, 1, len(b2.list)) + assert.Equal(t, 1, len(b3.list)) + + // The returned baggage needs to be immutable and should use a copy of the + // properties slice. + p[0] = Property{key: "different"} + assert.Equal(t, value{v: "v", p: properties{{key: "p"}}}, b3.list[key]) + // Reset for below. + p[0] = Property{key: "p"} + + m = Member{key: "another"} + b4, err := b3.SetMember(m) + assert.NoError(t, err) + assert.Equal(t, value{v: "v", p: p}, b3.list[key]) + assert.NotContains(t, b3.list, m.key) + assert.Equal(t, value{v: "v", p: p}, b4.list[key]) + assert.Equal(t, value{}, b4.list[m.key]) + assert.Equal(t, 1, len(b3.list)) + assert.Equal(t, 2, len(b4.list)) +} + +func TestNilBaggageMembers(t *testing.T) { + assert.Nil(t, Baggage{}.Members()) +} + +func TestBaggageMembers(t *testing.T) { + members := []Member{ + { + key: "foo", + value: "1", + properties: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + { + key: "bar", + value: "2", + properties: properties{ + {key: "yellow"}, + }, + }, + } + + baggage := Baggage{list: map[string]value{ + "foo": { + v: "1", + p: properties{ + {key: "state", value: "on", hasValue: true}, + {key: "red"}, + }, + }, + "bar": { + v: "2", + p: properties{{key: "yellow"}}, + }, + }} + + assert.ElementsMatch(t, members, baggage.Members()) +} + +func TestBaggageMember(t *testing.T) { + baggage := Baggage{list: map[string]value{"foo": {v: "1"}}} + assert.Equal(t, Member{key: "foo", value: "1"}, baggage.Member("foo")) + assert.Equal(t, Member{}, baggage.Member("bar")) +} + +func TestMemberKey(t *testing.T) { + m := Member{} + assert.Equal(t, "", m.Key(), "even invalid values should be returned") + + key := "k" + m.key = key + assert.Equal(t, key, m.Key()) +} + +func TestMemberValue(t *testing.T) { + m := Member{key: "k", value: "\\"} + assert.Equal(t, "\\", m.Value(), "even invalid values should be returned") + + value := "v" + m.value = value + assert.Equal(t, value, m.Value()) +} + +func TestMemberProperties(t *testing.T) { + m := Member{key: "k", value: "v"} + assert.Nil(t, m.Properties()) + + p := []Property{{key: "foo"}} + m.properties = properties(p) + got := m.Properties() + assert.Equal(t, p, got) + + // Returned slice needs to be a copy so the orginal is immutable. + got[0] = Property{key: "bar"} + assert.NotEqual(t, m.properties, got) +} + +func TestMemberValidation(t *testing.T) { + m := Member{} + 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()) +} + +func TestNewMember(t *testing.T) { + m, err := NewMember("", "") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Member{}, m) + + key, val := "k", "v" + p := Property{key: "foo"} + m, err = NewMember(key, val, p) + assert.NoError(t, err) + expected := Member{key: key, value: val, properties: properties{{key: "foo"}}} + 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) + + p[0].key = "foo" + assert.NoError(t, p.validate()) + + p = append(p, Property{key: "bar"}) + assert.NoError(t, p.validate()) +} From b6bc2ec80d3c23e7e64fbb9c8bb696011bf48dcd Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 27 May 2021 15:51:34 -0700 Subject: [PATCH 05/20] Update baggage context functionality --- baggage/context.go | 56 +++++++++--------------------- baggage/context_test.go | 77 +++++++++++------------------------------ 2 files changed, 37 insertions(+), 96 deletions(-) diff --git a/baggage/context.go b/baggage/context.go index 365388c654e..75f0558c3ec 100644 --- a/baggage/context.go +++ b/baggage/context.go @@ -16,52 +16,28 @@ package baggage // import "go.opentelemetry.io/otel/baggage" import ( "context" - - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/internal/baggage" ) -// Set returns a copy of the set of baggage key-values in ctx. -func Set(ctx context.Context) attribute.Set { - // TODO (MrAlias, #1222): The underlying storage, the Map, shares many of - // the functional elements of the attribute.Set. These should be unified so - // this conversion is unnecessary and there is no performance hit calling - // this. - m := baggage.MapFromContext(ctx) - values := make([]attribute.KeyValue, 0, m.Len()) - m.Foreach(func(kv attribute.KeyValue) bool { - values = append(values, kv) - return true - }) - return attribute.NewSet(values...) -} +type baggageContextKeyType int -// Value returns the value related to key in the baggage of ctx. If no -// value is set, the returned attribute.Value will be an uninitialized zero-value -// with type INVALID. -func Value(ctx context.Context, key attribute.Key) attribute.Value { - v, _ := baggage.MapFromContext(ctx).Value(key) - return v -} +const baggageKey baggageContextKeyType = iota -// ContextWithValues returns a copy of parent with pairs updated in the baggage. -func ContextWithValues(parent context.Context, pairs ...attribute.KeyValue) context.Context { - m := baggage.MapFromContext(parent).Apply(baggage.MapUpdate{ - MultiKV: pairs, - }) - return baggage.ContextWithMap(parent, m) +// ContextWithBaggage returns a copy of parent with baggage. +func ContextWithBaggage(parent context.Context, baggage Baggage) context.Context { + return context.WithValue(parent, baggageKey, baggage) } -// ContextWithoutValues returns a copy of parent in which the values related -// to keys have been removed from the baggage. -func ContextWithoutValues(parent context.Context, keys ...attribute.Key) context.Context { - m := baggage.MapFromContext(parent).Apply(baggage.MapUpdate{ - DropMultiK: keys, - }) - return baggage.ContextWithMap(parent, m) +// ContextWithBaggage returns a copy of parent with no baggage. +func ContextWithoutBaggage(parent context.Context) context.Context { + return context.WithValue(parent, baggageKey, nil) } -// ContextWithEmpty returns a copy of parent without baggage. -func ContextWithEmpty(parent context.Context) context.Context { - return baggage.ContextWithNoCorrelationData(parent) +// FromContext returns the baggage contained in ctx. +func FromContext(ctx context.Context) Baggage { + switch v := ctx.Value(baggageKey).(type) { + case Baggage: + return v + default: + return Baggage{} + } } diff --git a/baggage/context_test.go b/baggage/context_test.go index 53f57b1bcd4..770654de85a 100644 --- a/baggage/context_test.go +++ b/baggage/context_test.go @@ -18,69 +18,34 @@ import ( "context" "testing" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/internal/baggage" + "github.com/stretchr/testify/assert" ) -func TestBaggage(t *testing.T) { +func TestContextWithBaggage(t *testing.T) { ctx := context.Background() - ctx = baggage.ContextWithMap(ctx, baggage.NewEmptyMap()) + b := Baggage{list: map[string]value{"foo": {v: "1"}}} - b := Set(ctx) - if b.Len() != 0 { - t.Fatalf("empty baggage returned a set with %d elements", b.Len()) - } + nCtx := ContextWithBaggage(ctx, b) + assert.Equal(t, b, nCtx.Value(baggageKey)) + assert.Nil(t, ctx.Value(baggageKey)) +} - first, second, third := attribute.Key("first"), attribute.Key("second"), attribute.Key("third") - ctx = ContextWithValues(ctx, first.Bool(true), second.String("2")) - m := baggage.MapFromContext(ctx) - v, ok := m.Value(first) - if !ok { - t.Fatal("WithValues failed to set first value") - } - if !v.AsBool() { - t.Fatal("WithValues failed to set first correct value") - } - v, ok = m.Value(second) - if !ok { - t.Fatal("WithValues failed to set second value") - } - if v.AsString() != "2" { - t.Fatal("WithValues failed to set second correct value") - } - _, ok = m.Value(third) - if ok { - t.Fatal("WithValues set an unexpected third value") - } +func TestContextWithoutBaggage(t *testing.T) { + b := Baggage{list: map[string]value{"foo": {v: "1"}}} - b = Set(ctx) - if b.Len() != 2 { - t.Fatalf("Baggage returned a set with %d elements, want 2", b.Len()) - } + ctx := context.Background() + ctx = context.WithValue(ctx, baggageKey, b) - v = Value(ctx, first) - if v.Type() != attribute.BOOL || !v.AsBool() { - t.Fatal("Value failed to get correct first value") - } - v = Value(ctx, second) - if v.Type() != attribute.STRING || v.AsString() != "2" { - t.Fatal("Value failed to get correct second value") - } + nCtx := ContextWithoutBaggage(ctx) + assert.Nil(t, nCtx.Value(baggageKey)) + assert.Equal(t, b, ctx.Value(baggageKey)) +} - ctx = ContextWithoutValues(ctx, first) - m = baggage.MapFromContext(ctx) - _, ok = m.Value(first) - if ok { - t.Fatal("WithoutValues failed to remove a baggage value") - } - _, ok = m.Value(second) - if !ok { - t.Fatal("WithoutValues removed incorrect value") - } +func TestFromContext(t *testing.T) { + ctx := context.Background() + assert.Equal(t, Baggage{}, FromContext(ctx)) - ctx = ContextWithEmpty(ctx) - m = baggage.MapFromContext(ctx) - if m.Len() != 0 { - t.Fatal("WithoutBaggage failed to clear baggage") - } + b := Baggage{list: map[string]value{"foo": {v: "1"}}} + ctx = context.WithValue(ctx, baggageKey, b) + assert.Equal(t, b, FromContext(ctx)) } From 587ab77ca7279ed59ff41fe74b6481e069fe77bb Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 27 May 2021 17:33:16 -0700 Subject: [PATCH 06/20] Add New method to baggage pkg --- baggage/baggage.go | 48 +++++++++++++++++++++++----- baggage/baggage_test.go | 69 +++++++++++++++++++++++++++++++++++------ 2 files changed, 100 insertions(+), 17 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index d659c7d2a4e..5865f275101 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -281,6 +281,36 @@ type Baggage struct { //nolint:golint list map[string]value } +// 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. +func New(members ...Member) (Baggage, error) { + if len(members) == 0 { + return Baggage{}, nil + } + + b := make(map[string]value) + for _, m := range members { + if err := m.validate(); err != nil { + return Baggage{}, err + } + // OpenTelemetry resolves duplicates by last-one-wins. + b[m.key] = value{m.value, m.properties.Copy()} + } + + // Check member numbers after deduplicating. + if len(b) > maxMembers { + return Baggage{}, errMemberNumber + } + + bag := Baggage{b} + if n := len(bag.String()); n > maxBytesPerBaggageString { + return Baggage{}, fmt.Errorf("%w: %d", errBaggageBytes, n) + } + + return bag, nil +} + // Parse attempts to decode a baggage-string from the passed string. It // returns an error if the input is invalid according to the W3C Baggage // specification. @@ -298,19 +328,23 @@ func Parse(baggage string) (Baggage, error) { return Baggage{}, fmt.Errorf("%w: %d", errBaggageBytes, n) } - members := strings.Split(baggage, listDelimiter) - if len(members) > maxMembers { - return Baggage{}, errMemberNumber - } - - b := make(map[string]value, len(members)) - for _, memberStr := range members { + b := make(map[string]value) + for _, memberStr := range strings.Split(baggage, listDelimiter) { m, err := parseMember(memberStr) if err != nil { return Baggage{}, err } + // OpenTelemetry resolves duplicates by last-one-wins. b[m.key] = value{m.value, m.properties} } + + // OpenTelemetry does not allow for duplicate list-members, but the W3C + // specification does. Now that we have deduplicated, ensure the baggage + // does not exceed list-member limits. + if len(b) > maxMembers { + return Baggage{}, errMemberNumber + } + return Baggage{b}, nil } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index ecd4cbaaa65..a556141571b 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -16,6 +16,7 @@ package baggage import ( "fmt" + "math/rand" "sort" "strings" "testing" @@ -162,22 +163,70 @@ func TestPropertyValidate(t *testing.T) { assert.NoError(t, p.validate()) } -func TestBaggageParse(t *testing.T) { - b := make([]rune, maxBytesPerBaggageString+1) - for i := range b { - b[i] = 'a' +func TestNewEmptyBaggage(t *testing.T) { + b, err := New() + assert.NoError(t, err) + assert.Equal(t, Baggage{}, b) +} + +func TestNewBaggage(t *testing.T) { + b, err := New(Member{key: "k"}) + assert.NoError(t, err) + assert.Equal(t, Baggage{list: map[string]value{"k": {}}}, b) +} + +func TestNewBaggageWithDuplicates(t *testing.T) { + m := make([]Member, maxMembers+1) + for i := range m { + // Duplicates are collapsed. + m[i] = Member{key: "a"} } - tooLarge := string(b) + b, err := New(m...) + assert.NoError(t, err) + assert.Equal(t, Baggage{list: map[string]value{"a": {}}}, b) +} + +func TestNewBaggageErrorInvalidMember(t *testing.T) { + _, err := New(Member{key: ""}) + assert.ErrorIs(t, err, errInvalidKey) +} - b = make([]rune, maxBytesPerMembers+1) +func key(n int) string { + r := []rune("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") + + b := make([]rune, n) for i := range b { - b[i] = 'a' + b[i] = r[rand.Intn(len(r))] } - tooLargeMember := string(b) + return string(b) +} + +func TestNewBaggageErrorTooManyBytes(t *testing.T) { + m := make([]Member, (maxBytesPerBaggageString/maxBytesPerMembers)+1) + for i := range m { + m[i] = Member{key: key(maxBytesPerMembers)} + } + _, err := New(m...) + assert.ErrorIs(t, err, errBaggageBytes) +} + +func TestNewBaggageErrorTooManyMembers(t *testing.T) { + m := make([]Member, maxMembers+1) + for i := range m { + m[i] = Member{key: fmt.Sprintf("%d", i)} + } + _, err := New(m...) + assert.ErrorIs(t, err, errMemberNumber) +} + +func TestBaggageParse(t *testing.T) { + tooLarge := key(maxBytesPerBaggageString + 1) + + tooLargeMember := key(maxBytesPerMembers + 1) m := make([]string, maxMembers+1) for i := range m { - m[i] = "a=" + m[i] = fmt.Sprintf("a%d=", i) } tooManyMembers := strings.Join(m, listDelimiter) @@ -544,7 +593,7 @@ func TestMemberProperties(t *testing.T) { got := m.Properties() assert.Equal(t, p, got) - // Returned slice needs to be a copy so the orginal is immutable. + // Returned slice needs to be a copy so the original is immutable. got[0] = Property{key: "bar"} assert.NotEqual(t, m.properties, got) } From 000b7bcf81732fc1f7b1e10ca0ea93d2b0837411 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 27 May 2021 17:39:16 -0700 Subject: [PATCH 07/20] Update namedtracer example --- example/namedtracer/main.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/example/namedtracer/main.go b/example/namedtracer/main.go index 0ea4669a846..37063a4e550 100644 --- a/example/namedtracer/main.go +++ b/example/namedtracer/main.go @@ -59,7 +59,11 @@ func main() { tracer := tp.Tracer("example/namedtracer/main") ctx := context.Background() defer func() { _ = tp.Shutdown(ctx) }() - ctx = baggage.ContextWithValues(ctx, fooKey.String("foo1"), barKey.String("bar1")) + + m0, _ := baggage.NewMember(string(fooKey), "foo1") + m1, _ := baggage.NewMember(string(barKey), "bar1") + b, _ := baggage.New(m0, m1) + ctx = baggage.ContextWithBaggage(ctx, b) var span trace.Span ctx, span = tracer.Start(ctx, "operation") From 6792ed321c7e2b57edbc67b54bd52ecdda45d34a Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Jun 2021 11:16:21 -0700 Subject: [PATCH 08/20] URL encode baggage values --- baggage/baggage.go | 4 +++- baggage/baggage_test.go | 12 +++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 5865f275101..b800d491de6 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -17,6 +17,7 @@ package baggage import ( "errors" "fmt" + "net/url" "regexp" "strings" ) @@ -263,7 +264,8 @@ func (m Member) Properties() []Property { return m.properties.Copy() } // String encodes Member into a string compliant with the W3C Baggage // specification. func (m Member) String() string { - s := fmt.Sprintf("%s=%s", m.key, m.value) + // A key is just an ASCII string, but a value is URL encoded UTF-8. + s := fmt.Sprintf("%s=%s", m.key, url.QueryEscape(m.value)) if len(m.properties) > 0 { s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String()) } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index a556141571b..c2ff5f7a39e 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -377,6 +377,13 @@ func TestBaggageString(t *testing.T) { "foo": {v: "1"}, }, }, + { + name: "URL encoded value", + out: "foo=1%3D1", + baggage: map[string]value{ + "foo": {v: "1=1"}, + }, + }, { name: "single member empty value with properties", out: "foo=;red;state=on", @@ -392,13 +399,16 @@ func TestBaggageString(t *testing.T) { }, { name: "single member with properties", - out: "foo=1;red;state=on", + // Properties are "opaque values" meaning they are sent as they + // are set and no encoding is performed. + out: "foo=1;red;state=on;z=z=z", baggage: map[string]value{ "foo": { v: "1", p: properties{ {key: "state", value: "on", hasValue: true}, {key: "red"}, + {key: "z", value: "z=z", hasValue: true}, }, }, }, From fc7ebdca627b3e48b4a65f8ca3cd52b9d9957a37 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Jun 2021 10:19:16 -0700 Subject: [PATCH 09/20] Refactor and use internal baggage pkg --- baggage/baggage.go | 81 +++++--- baggage/baggage_test.go | 147 +++++++------- baggage/context.go | 22 +- baggage/context_test.go | 30 +-- internal/baggage/baggage.go | 339 ++----------------------------- internal/baggage/baggage_test.go | 285 -------------------------- internal/baggage/context.go | 95 +++++++++ internal/baggage/context_test.go | 96 +++++++++ 8 files changed, 360 insertions(+), 735 deletions(-) delete mode 100644 internal/baggage/baggage_test.go create mode 100644 internal/baggage/context.go create mode 100644 internal/baggage/context_test.go diff --git a/baggage/baggage.go b/baggage/baggage.go index b800d491de6..cfaa53a3bb3 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -20,6 +20,8 @@ import ( "net/url" "regexp" "strings" + + "go.opentelemetry.io/otel/internal/baggage" ) const ( @@ -150,6 +152,38 @@ func (p Property) String() string { type properties []Property +func fromInternalProperties(iProps []baggage.Property) properties { + if len(iProps) == 0 { + return nil + } + + props := make(properties, len(iProps)) + for i, p := range iProps { + props[i] = Property{ + key: p.Key, + value: p.Value, + hasValue: p.HasValue, + } + } + return props +} + +func (p properties) asInternal() []baggage.Property { + if len(p) == 0 { + return nil + } + + iProps := make([]baggage.Property, len(p)) + for i, prop := range p { + iProps[i] = baggage.Property{ + Key: prop.key, + Value: prop.value, + HasValue: prop.hasValue, + } + } + return iProps +} + func (p properties) Copy() properties { if len(p) == 0 { return nil @@ -272,15 +306,10 @@ func (m Member) String() string { return s } -type value struct { - v string - p properties -} - // Baggage is a list of baggage members representing the baggage-string as // defined by the W3C Baggage specification. type Baggage struct { //nolint:golint - list map[string]value + list baggage.List } // New returns a new valid Baggage. It returns an error if the passed members @@ -291,13 +320,13 @@ func New(members ...Member) (Baggage, error) { return Baggage{}, nil } - b := make(map[string]value) + b := make(baggage.List) for _, m := range members { if err := m.validate(); err != nil { return Baggage{}, err } // OpenTelemetry resolves duplicates by last-one-wins. - b[m.key] = value{m.value, m.properties.Copy()} + b[m.key] = baggage.Item{m.value, m.properties.asInternal()} } // Check member numbers after deduplicating. @@ -321,23 +350,23 @@ func New(members ...Member) (Baggage, error) { // defined (reading left-to-right) will be the only one kept. This diverges // from the W3C Baggage specification which allows duplicate list-members, but // conforms to the OpenTelemetry Baggage specification. -func Parse(baggage string) (Baggage, error) { - if baggage == "" { +func Parse(bStr string) (Baggage, error) { + if bStr == "" { return Baggage{}, nil } - if n := len(baggage); n > maxBytesPerBaggageString { + if n := len(bStr); n > maxBytesPerBaggageString { return Baggage{}, fmt.Errorf("%w: %d", errBaggageBytes, n) } - b := make(map[string]value) - for _, memberStr := range strings.Split(baggage, listDelimiter) { + b := make(baggage.List) + for _, memberStr := range strings.Split(bStr, listDelimiter) { m, err := parseMember(memberStr) if err != nil { return Baggage{}, err } // OpenTelemetry resolves duplicates by last-one-wins. - b[m.key] = value{m.value, m.properties} + b[m.key] = baggage.Item{m.value, m.properties.asInternal()} } // OpenTelemetry does not allow for duplicate list-members, but the W3C @@ -364,7 +393,11 @@ func (b Baggage) Member(key string) Member { return Member{} } - return Member{key: key, value: v.v, properties: v.p.Copy()} + return Member{ + key: key, + value: v.Value, + properties: fromInternalProperties(v.Properties), + } } // Members returns all the baggage list-members. @@ -378,8 +411,8 @@ func (b Baggage) Members() []Member { for k, v := range b.list { members = append(members, Member{ key: k, - value: v.v, - properties: v.p.Copy(), + value: v.Value, + properties: fromInternalProperties(v.Properties), }) } return members @@ -400,7 +433,7 @@ func (b Baggage) SetMember(member Member) (Baggage, error) { if _, ok := b.list[member.key]; !ok { n++ } - list := make(map[string]value, n) + list := make(baggage.List, n) for k, v := range b.list { // Do not copy if we are just going to overwrite. @@ -410,9 +443,9 @@ func (b Baggage) SetMember(member Member) (Baggage, error) { list[k] = v } - list[member.key] = value{ - v: member.value, - p: member.properties.Copy(), + list[member.key] = baggage.Item{ + Value: member.value, + Properties: member.properties.asInternal(), } return Baggage{list: list}, nil @@ -425,7 +458,7 @@ func (b Baggage) DeleteMember(key string) Baggage { if _, ok := b.list[key]; ok { n-- } - list := make(map[string]value, n) + list := make(baggage.List, n) for k, v := range b.list { if k == key { @@ -450,8 +483,8 @@ func (b Baggage) String() string { for k, v := range b.list { members = append(members, Member{ key: k, - value: v.v, - properties: v.p, + value: v.Value, + properties: fromInternalProperties(v.Properties), }.String()) } return strings.Join(members, listDelimiter) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index c2ff5f7a39e..751ee5e2ebf 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/internal/baggage" ) func TestKeyRegExp(t *testing.T) { @@ -172,7 +173,7 @@ func TestNewEmptyBaggage(t *testing.T) { func TestNewBaggage(t *testing.T) { b, err := New(Member{key: "k"}) assert.NoError(t, err) - assert.Equal(t, Baggage{list: map[string]value{"k": {}}}, b) + assert.Equal(t, Baggage{list: baggage.List{"k": {}}}, b) } func TestNewBaggageWithDuplicates(t *testing.T) { @@ -183,7 +184,7 @@ func TestNewBaggageWithDuplicates(t *testing.T) { } b, err := New(m...) assert.NoError(t, err) - assert.Equal(t, Baggage{list: map[string]value{"a": {}}}, b) + assert.Equal(t, Baggage{list: baggage.List{"a": {}}}, b) } func TestNewBaggageErrorInvalidMember(t *testing.T) { @@ -233,37 +234,37 @@ func TestBaggageParse(t *testing.T) { testcases := []struct { name string in string - baggage map[string]value + baggage baggage.List err error }{ { name: "empty value", in: "", - baggage: map[string]value(nil), + baggage: baggage.List(nil), }, { name: "single member empty value no properties", in: "foo=", - baggage: map[string]value{ - "foo": {v: ""}, + baggage: baggage.List{ + "foo": {Value: ""}, }, }, { name: "single member no properties", in: "foo=1", - baggage: map[string]value{ - "foo": {v: "1"}, + baggage: baggage.List{ + "foo": {Value: "1"}, }, }, { name: "single member empty value with properties", in: "foo=;state=on;red", - baggage: map[string]value{ + baggage: baggage.List{ "foo": { - v: "", - p: properties{ - {key: "state", value: "on", hasValue: true}, - {key: "red"}, + Value: "", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, }, }, }, @@ -271,12 +272,12 @@ func TestBaggageParse(t *testing.T) { { name: "single member with properties", in: "foo=1;state=on;red", - baggage: map[string]value{ + baggage: baggage.List{ "foo": { - v: "1", - p: properties{ - {key: "state", value: "on", hasValue: true}, - {key: "red"}, + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, }, }, }, @@ -284,17 +285,17 @@ func TestBaggageParse(t *testing.T) { { name: "two members with properties", in: "foo=1;state=on;red,bar=2;yellow", - baggage: map[string]value{ + baggage: baggage.List{ "foo": { - v: "1", - p: properties{ - {key: "state", value: "on", hasValue: true}, - {key: "red"}, + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, }, }, "bar": { - v: "2", - p: properties{{key: "yellow"}}, + Value: "2", + Properties: []baggage.Property{{Key: "yellow"}}, }, }, }, @@ -302,8 +303,8 @@ func TestBaggageParse(t *testing.T) { // According to the OTel spec, last value wins. name: "duplicate key", in: "foo=1;state=on;red,foo=2", - baggage: map[string]value{ - "foo": {v: "2"}, + baggage: baggage.List{ + "foo": {Value: "2"}, }, }, { @@ -356,43 +357,43 @@ func TestBaggageString(t *testing.T) { testcases := []struct { name string out string - baggage map[string]value + baggage baggage.List }{ { name: "empty value", out: "", - baggage: map[string]value(nil), + baggage: baggage.List(nil), }, { name: "single member empty value no properties", out: "foo=", - baggage: map[string]value{ - "foo": {v: ""}, + baggage: baggage.List{ + "foo": {Value: ""}, }, }, { name: "single member no properties", out: "foo=1", - baggage: map[string]value{ - "foo": {v: "1"}, + baggage: baggage.List{ + "foo": {Value: "1"}, }, }, { name: "URL encoded value", out: "foo=1%3D1", - baggage: map[string]value{ - "foo": {v: "1=1"}, + baggage: baggage.List{ + "foo": {Value: "1=1"}, }, }, { name: "single member empty value with properties", out: "foo=;red;state=on", - baggage: map[string]value{ + baggage: baggage.List{ "foo": { - v: "", - p: properties{ - {key: "state", value: "on", hasValue: true}, - {key: "red"}, + Value: "", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, }, }, }, @@ -402,13 +403,13 @@ func TestBaggageString(t *testing.T) { // Properties are "opaque values" meaning they are sent as they // are set and no encoding is performed. out: "foo=1;red;state=on;z=z=z", - baggage: map[string]value{ + baggage: baggage.List{ "foo": { - v: "1", - p: properties{ - {key: "state", value: "on", hasValue: true}, - {key: "red"}, - {key: "z", value: "z=z", hasValue: true}, + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, + {Key: "z", Value: "z=z", HasValue: true}, }, }, }, @@ -416,17 +417,17 @@ func TestBaggageString(t *testing.T) { { name: "two members with properties", out: "bar=2;yellow,foo=1;red;state=on", - baggage: map[string]value{ + baggage: baggage.List{ "foo": { - v: "1", - p: properties{ - {key: "state", value: "on", hasValue: true}, - {key: "red"}, + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, }, }, "bar": { - v: "2", - p: properties{{key: "yellow"}}, + Value: "2", + Properties: []baggage.Property{{Key: "yellow"}}, }, }, }, @@ -455,10 +456,10 @@ func TestBaggageLen(t *testing.T) { b := Baggage{} assert.Equal(t, 0, b.Len()) - b.list = make(map[string]value, 1) + b.list = make(baggage.List, 1) assert.Equal(t, 0, b.Len()) - b.list["k"] = value{} + b.list["k"] = baggage.Item{} assert.Equal(t, 1, b.Len()) } @@ -469,7 +470,7 @@ func TestBaggageDeleteMember(t *testing.T) { b1 := b0.DeleteMember(key) assert.NotContains(t, b1.list, key) - b0 = Baggage{list: map[string]value{ + b0 = Baggage{list: baggage.List{ key: {}, "other": {}, }} @@ -491,15 +492,15 @@ func TestBaggageSetMember(t *testing.T) { b1, err := b0.SetMember(m) assert.NoError(t, err) assert.NotContains(t, b0.list, key) - assert.Equal(t, value{}, b1.list[key]) + assert.Equal(t, baggage.Item{}, b1.list[key]) assert.Equal(t, 0, len(b0.list)) assert.Equal(t, 1, len(b1.list)) m.value = "v" b2, err := b1.SetMember(m) assert.NoError(t, err) - assert.Equal(t, value{}, b1.list[key]) - assert.Equal(t, value{v: "v"}, b2.list[key]) + assert.Equal(t, baggage.Item{}, b1.list[key]) + assert.Equal(t, baggage.Item{Value: "v"}, b2.list[key]) assert.Equal(t, 1, len(b1.list)) assert.Equal(t, 1, len(b2.list)) @@ -507,25 +508,25 @@ func TestBaggageSetMember(t *testing.T) { m.properties = p b3, err := b2.SetMember(m) assert.NoError(t, err) - assert.Equal(t, value{v: "v"}, b2.list[key]) - assert.Equal(t, value{v: "v", p: p}, b3.list[key]) + assert.Equal(t, baggage.Item{Value: "v"}, b2.list[key]) + assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b3.list[key]) assert.Equal(t, 1, len(b2.list)) assert.Equal(t, 1, len(b3.list)) // The returned baggage needs to be immutable and should use a copy of the // properties slice. p[0] = Property{key: "different"} - assert.Equal(t, value{v: "v", p: properties{{key: "p"}}}, b3.list[key]) + assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b3.list[key]) // Reset for below. p[0] = Property{key: "p"} m = Member{key: "another"} b4, err := b3.SetMember(m) assert.NoError(t, err) - assert.Equal(t, value{v: "v", p: p}, b3.list[key]) + assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b3.list[key]) assert.NotContains(t, b3.list, m.key) - assert.Equal(t, value{v: "v", p: p}, b4.list[key]) - assert.Equal(t, value{}, b4.list[m.key]) + assert.Equal(t, baggage.Item{Value: "v", Properties: []baggage.Property{{Key: "p"}}}, b4.list[key]) + assert.Equal(t, baggage.Item{}, b4.list[m.key]) assert.Equal(t, 1, len(b3.list)) assert.Equal(t, 2, len(b4.list)) } @@ -553,17 +554,17 @@ func TestBaggageMembers(t *testing.T) { }, } - baggage := Baggage{list: map[string]value{ + baggage := Baggage{list: baggage.List{ "foo": { - v: "1", - p: properties{ - {key: "state", value: "on", hasValue: true}, - {key: "red"}, + Value: "1", + Properties: []baggage.Property{ + {Key: "state", Value: "on", HasValue: true}, + {Key: "red"}, }, }, "bar": { - v: "2", - p: properties{{key: "yellow"}}, + Value: "2", + Properties: []baggage.Property{{Key: "yellow"}}, }, }} @@ -571,7 +572,7 @@ func TestBaggageMembers(t *testing.T) { } func TestBaggageMember(t *testing.T) { - baggage := Baggage{list: map[string]value{"foo": {v: "1"}}} + baggage := Baggage{list: baggage.List{"foo": {Value: "1"}}} assert.Equal(t, Member{key: "foo", value: "1"}, baggage.Member("foo")) assert.Equal(t, Member{}, baggage.Member("bar")) } diff --git a/baggage/context.go b/baggage/context.go index 75f0558c3ec..0ec4e39736a 100644 --- a/baggage/context.go +++ b/baggage/context.go @@ -16,28 +16,24 @@ package baggage // import "go.opentelemetry.io/otel/baggage" import ( "context" -) - -type baggageContextKeyType int -const baggageKey baggageContextKeyType = iota + "go.opentelemetry.io/otel/internal/baggage" +) // ContextWithBaggage returns a copy of parent with baggage. -func ContextWithBaggage(parent context.Context, baggage Baggage) context.Context { - return context.WithValue(parent, baggageKey, baggage) +func ContextWithBaggage(parent context.Context, b Baggage) context.Context { + // Delegate so any hooks for the OpenTracing bridge are handled. + return baggage.ContextWithList(parent, b.list) } // ContextWithBaggage returns a copy of parent with no baggage. func ContextWithoutBaggage(parent context.Context) context.Context { - return context.WithValue(parent, baggageKey, nil) + // Delegate so any hooks for the OpenTracing bridge are handled. + return baggage.ContextWithList(parent, nil) } // FromContext returns the baggage contained in ctx. func FromContext(ctx context.Context) Baggage { - switch v := ctx.Value(baggageKey).(type) { - case Baggage: - return v - default: - return Baggage{} - } + // Delegate so any hooks for the OpenTracing bridge are handled. + return Baggage{list: baggage.ListFromContext(ctx)} } diff --git a/baggage/context_test.go b/baggage/context_test.go index 770654de85a..31ad836488a 100644 --- a/baggage/context_test.go +++ b/baggage/context_test.go @@ -19,33 +19,17 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/internal/baggage" ) -func TestContextWithBaggage(t *testing.T) { - ctx := context.Background() - b := Baggage{list: map[string]value{"foo": {v: "1"}}} - - nCtx := ContextWithBaggage(ctx, b) - assert.Equal(t, b, nCtx.Value(baggageKey)) - assert.Nil(t, ctx.Value(baggageKey)) -} - -func TestContextWithoutBaggage(t *testing.T) { - b := Baggage{list: map[string]value{"foo": {v: "1"}}} - - ctx := context.Background() - ctx = context.WithValue(ctx, baggageKey, b) - - nCtx := ContextWithoutBaggage(ctx) - assert.Nil(t, nCtx.Value(baggageKey)) - assert.Equal(t, b, ctx.Value(baggageKey)) -} - -func TestFromContext(t *testing.T) { +func TestContext(t *testing.T) { ctx := context.Background() assert.Equal(t, Baggage{}, FromContext(ctx)) - b := Baggage{list: map[string]value{"foo": {v: "1"}}} - ctx = context.WithValue(ctx, baggageKey, b) + b := Baggage{list: baggage.List{"key": baggage.Item{Value: "val"}}} + ctx = ContextWithBaggage(ctx, b) assert.Equal(t, b, FromContext(ctx)) + + ctx = ContextWithoutBaggage(ctx) + assert.Equal(t, Baggage{}, FromContext(ctx)) } diff --git a/internal/baggage/baggage.go b/internal/baggage/baggage.go index b1f7daf8d86..87d0af4054c 100644 --- a/internal/baggage/baggage.go +++ b/internal/baggage/baggage.go @@ -12,327 +12,32 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package baggage provides types and functions to manage W3C Baggage. +/* +Package baggage provides base types and functionality to store and retrieve +baggage in Go context. This package exists because the OpenTracing bridge to +OpenTelemetry needs to synchronize state whenever baggage for a context is +modified and that context contains an OpenTracing span. If it were not for +this need this package would not need to exist and the +`go.opentelemetry.io/otel/baggage` package would be the singular place where +W3C baggage is handled. +*/ package baggage -import ( - "context" +// List is the collection of baggage members. The W3C allows for duplicates, +// but OpenTelemetry does not, therefore, this is represented as a map. +type List map[string]Item - "go.opentelemetry.io/otel/attribute" -) - -type rawMap map[attribute.Key]attribute.Value -type keySet map[attribute.Key]struct{} - -// Map is an immutable storage for correlations. -type Map struct { - m rawMap -} - -// MapUpdate contains information about correlation changes to be -// made. -type MapUpdate struct { - // DropSingleK contains a single key to be dropped from - // correlations. Use this to avoid an overhead of a slice - // allocation if there is only one key to drop. - DropSingleK attribute.Key - // DropMultiK contains all the keys to be dropped from - // correlations. - DropMultiK []attribute.Key - - // SingleKV contains a single key-value pair to be added to - // correlations. Use this to avoid an overhead of a slice - // allocation if there is only one key-value pair to add. - SingleKV attribute.KeyValue - // MultiKV contains all the key-value pairs to be added to - // correlations. - MultiKV []attribute.KeyValue -} - -func newMap(raw rawMap) Map { - return Map{ - m: raw, - } -} - -// NewEmptyMap creates an empty correlations map. -func NewEmptyMap() Map { - return newMap(nil) -} - -// NewMap creates a map with the contents of the update applied. In -// this function, having an update with DropSingleK or DropMultiK -// makes no sense - those fields are effectively ignored. -func NewMap(update MapUpdate) Map { - return NewEmptyMap().Apply(update) -} - -// Apply creates a copy of the map with the contents of the update -// applied. Apply will first drop the keys from DropSingleK and -// DropMultiK, then add key-value pairs from SingleKV and MultiKV. -func (m Map) Apply(update MapUpdate) Map { - delSet, addSet := getModificationSets(update) - mapSize := getNewMapSize(m.m, delSet, addSet) - - r := make(rawMap, mapSize) - for k, v := range m.m { - // do not copy items we want to drop - if _, ok := delSet[k]; ok { - continue - } - // do not copy items we would overwrite - if _, ok := addSet[k]; ok { - continue - } - r[k] = v - } - if update.SingleKV.Key.Defined() { - r[update.SingleKV.Key] = update.SingleKV.Value - } - for _, kv := range update.MultiKV { - r[kv.Key] = kv.Value - } - if len(r) == 0 { - r = nil - } - return newMap(r) -} - -func getModificationSets(update MapUpdate) (delSet, addSet keySet) { - deletionsCount := len(update.DropMultiK) - if update.DropSingleK.Defined() { - deletionsCount++ - } - if deletionsCount > 0 { - delSet = make(map[attribute.Key]struct{}, deletionsCount) - for _, k := range update.DropMultiK { - delSet[k] = struct{}{} - } - if update.DropSingleK.Defined() { - delSet[update.DropSingleK] = struct{}{} - } - } - - additionsCount := len(update.MultiKV) - if update.SingleKV.Key.Defined() { - additionsCount++ - } - if additionsCount > 0 { - addSet = make(map[attribute.Key]struct{}, additionsCount) - for _, k := range update.MultiKV { - addSet[k.Key] = struct{}{} - } - if update.SingleKV.Key.Defined() { - addSet[update.SingleKV.Key] = struct{}{} - } - } - - return -} - -func getNewMapSize(m rawMap, delSet, addSet keySet) int { - mapSizeDiff := 0 - for k := range addSet { - if _, ok := m[k]; !ok { - mapSizeDiff++ - } - } - for k := range delSet { - if _, ok := m[k]; ok { - if _, inAddSet := addSet[k]; !inAddSet { - mapSizeDiff-- - } - } - } - return len(m) + mapSizeDiff -} - -// Value gets a value from correlations map and returns a boolean -// value indicating whether the key exist in the map. -func (m Map) Value(k attribute.Key) (attribute.Value, bool) { - value, ok := m.m[k] - return value, ok -} - -// HasValue returns a boolean value indicating whether the key exist -// in the map. -func (m Map) HasValue(k attribute.Key) bool { - _, has := m.Value(k) - return has -} - -// Len returns a length of the map. -func (m Map) Len() int { - return len(m.m) -} - -// Foreach calls a passed callback once on each key-value pair until -// all the key-value pairs of the map were iterated or the callback -// returns false, whichever happens first. -func (m Map) Foreach(f func(attribute.KeyValue) bool) { - for k, v := range m.m { - if !f(attribute.KeyValue{ - Key: k, - Value: v, - }) { - return - } - } -} - -type correlationsType struct{} - -// SetHookFunc describes a type of a callback that is called when -// storing baggage in the context. -type SetHookFunc func(context.Context) context.Context - -// GetHookFunc describes a type of a callback that is called when -// getting baggage from the context. -type GetHookFunc func(context.Context, Map) Map - -// value under this key is either of type Map or correlationsData -var correlationsKey = &correlationsType{} - -type correlationsData struct { - m Map - setHook SetHookFunc - getHook GetHookFunc -} - -func (d correlationsData) isHookless() bool { - return d.setHook == nil && d.getHook == nil -} - -type hookKind int - -const ( - hookKindSet hookKind = iota - hookKindGet -) - -func (d *correlationsData) overrideHook(kind hookKind, setHook SetHookFunc, getHook GetHookFunc) { - switch kind { - case hookKindSet: - d.setHook = setHook - case hookKindGet: - d.getHook = getHook - } -} - -// ContextWithSetHook installs a hook function that will be invoked -// every time ContextWithMap is called. To avoid unnecessary callback -// invocations (recursive or not), the callback can temporarily clear -// the hooks from the context with the ContextWithNoHooks function. -// -// Note that NewContext also calls ContextWithMap, so the hook will be -// invoked. -// -// Passing nil SetHookFunc creates a context with no set hook to call. -// -// This function should not be used by applications or libraries. It -// is mostly for interoperation with other observability APIs. -func ContextWithSetHook(ctx context.Context, hook SetHookFunc) context.Context { - return contextWithHook(ctx, hookKindSet, hook, nil) +// Item is the value and metadata properties part of a list-member. +type Item struct { + Value string + Properties []Property } -// ContextWithGetHook installs a hook function that will be invoked -// every time MapFromContext is called. To avoid unnecessary callback -// invocations (recursive or not), the callback can temporarily clear -// the hooks from the context with the ContextWithNoHooks function. -// -// Note that NewContext also calls MapFromContext, so the hook will be -// invoked. -// -// Passing nil GetHookFunc creates a context with no get hook to call. -// -// This function should not be used by applications or libraries. It -// is mostly for interoperation with other observability APIs. -func ContextWithGetHook(ctx context.Context, hook GetHookFunc) context.Context { - return contextWithHook(ctx, hookKindGet, nil, hook) -} - -func contextWithHook(ctx context.Context, kind hookKind, setHook SetHookFunc, getHook GetHookFunc) context.Context { - switch v := ctx.Value(correlationsKey).(type) { - case correlationsData: - v.overrideHook(kind, setHook, getHook) - if v.isHookless() { - return context.WithValue(ctx, correlationsKey, v.m) - } - return context.WithValue(ctx, correlationsKey, v) - case Map: - return contextWithOneHookAndMap(ctx, kind, setHook, getHook, v) - default: - m := NewEmptyMap() - return contextWithOneHookAndMap(ctx, kind, setHook, getHook, m) - } -} - -func contextWithOneHookAndMap(ctx context.Context, kind hookKind, setHook SetHookFunc, getHook GetHookFunc, m Map) context.Context { - d := correlationsData{m: m} - d.overrideHook(kind, setHook, getHook) - if d.isHookless() { - return ctx - } - return context.WithValue(ctx, correlationsKey, d) -} - -// ContextWithNoHooks creates a context with all the hooks -// disabled. Also returns old set and get hooks. This function can be -// used to temporarily clear the context from hooks and then reinstate -// them by calling ContextWithSetHook and ContextWithGetHook functions -// passing the hooks returned by this function. -// -// This function should not be used by applications or libraries. It -// is mostly for interoperation with other observability APIs. -func ContextWithNoHooks(ctx context.Context) (context.Context, SetHookFunc, GetHookFunc) { - switch v := ctx.Value(correlationsKey).(type) { - case correlationsData: - return context.WithValue(ctx, correlationsKey, v.m), v.setHook, v.getHook - default: - return ctx, nil, nil - } -} - -// ContextWithMap returns a context with the Map entered into it. -func ContextWithMap(ctx context.Context, m Map) context.Context { - switch v := ctx.Value(correlationsKey).(type) { - case correlationsData: - v.m = m - ctx = context.WithValue(ctx, correlationsKey, v) - if v.setHook != nil { - ctx = v.setHook(ctx) - } - return ctx - default: - return context.WithValue(ctx, correlationsKey, m) - } -} - -// ContextWithNoCorrelationData returns a context stripped of correlation -// data. -func ContextWithNoCorrelationData(ctx context.Context) context.Context { - return context.WithValue(ctx, correlationsKey, nil) -} - -// NewContext returns a context with the map from passed context -// updated with the passed key-value pairs. -func NewContext(ctx context.Context, keyvalues ...attribute.KeyValue) context.Context { - return ContextWithMap(ctx, MapFromContext(ctx).Apply(MapUpdate{ - MultiKV: keyvalues, - })) -} +// Property is a metadata entry for a list-member. +type Property struct { + Key, Value string -// MapFromContext gets the current Map from a Context. -func MapFromContext(ctx context.Context) Map { - switch v := ctx.Value(correlationsKey).(type) { - case correlationsData: - if v.getHook != nil { - return v.getHook(ctx, v.m) - } - return v.m - case Map: - return v - default: - return NewEmptyMap() - } + // HasValue indicates if a zero-value value means the property does not + // have a value or if it was the zero-value. + HasValue bool } diff --git a/internal/baggage/baggage_test.go b/internal/baggage/baggage_test.go deleted file mode 100644 index 8c164a5c5cf..00000000000 --- a/internal/baggage/baggage_test.go +++ /dev/null @@ -1,285 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package baggage - -import ( - "fmt" - "testing" - - "go.opentelemetry.io/otel/attribute" -) - -type testCase struct { - name string - value MapUpdate - init []int - wantKVs []attribute.KeyValue -} - -func TestMap(t *testing.T) { - for _, testcase := range getTestCases() { - t.Logf("Running test case %s", testcase.name) - var got Map - if len(testcase.init) > 0 { - got = makeTestMap(testcase.init).Apply(testcase.value) - } else { - got = NewMap(testcase.value) - } - for _, s := range testcase.wantKVs { - if ok := got.HasValue(s.Key); !ok { - t.Errorf("Expected Key %s to have Value", s.Key) - } - if g, ok := got.Value(s.Key); !ok || g != s.Value { - t.Errorf("+got: %v, -want: %v", g, s.Value) - } - } - // test Foreach() - got.Foreach(func(kv attribute.KeyValue) bool { - for _, want := range testcase.wantKVs { - if kv == want { - return false - } - } - t.Errorf("Expected label %v, but not found", kv) - return true - }) - if l, exp := got.Len(), len(testcase.wantKVs); l != exp { - t.Errorf("+got: %d, -want: %d", l, exp) - } - } -} - -func TestSizeComputation(t *testing.T) { - for _, testcase := range getTestCases() { - t.Logf("Running test case %s", testcase.name) - var initMap Map - if len(testcase.init) > 0 { - initMap = makeTestMap(testcase.init) - } else { - initMap = NewEmptyMap() - } - gotMap := initMap.Apply(testcase.value) - - delSet, addSet := getModificationSets(testcase.value) - mapSize := getNewMapSize(initMap.m, delSet, addSet) - - if gotMap.Len() != mapSize { - t.Errorf("Expected computed size to be %d, got %d", gotMap.Len(), mapSize) - } - } -} - -func getTestCases() []testCase { - return []testCase{ - { - name: "map with MultiKV", - value: MapUpdate{MultiKV: []attribute.KeyValue{ - attribute.Int64("key1", 1), - attribute.String("key2", "val2")}, - }, - init: []int{}, - wantKVs: []attribute.KeyValue{ - attribute.Int64("key1", 1), - attribute.String("key2", "val2"), - }, - }, - { - name: "map with SingleKV", - value: MapUpdate{SingleKV: attribute.String("key1", "val1")}, - init: []int{}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - }, - }, - { - name: "map with both add fields", - value: MapUpdate{SingleKV: attribute.Int64("key1", 3), - MultiKV: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2")}, - }, - init: []int{}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - }, - }, - { - name: "map with empty MapUpdate", - value: MapUpdate{}, - init: []int{}, - wantKVs: []attribute.KeyValue{}, - }, - { - name: "map with DropSingleK", - value: MapUpdate{DropSingleK: attribute.Key("key1")}, - init: []int{}, - wantKVs: []attribute.KeyValue{}, - }, - { - name: "map with DropMultiK", - value: MapUpdate{DropMultiK: []attribute.Key{ - attribute.Key("key1"), attribute.Key("key2"), - }}, - init: []int{}, - wantKVs: []attribute.KeyValue{}, - }, - { - name: "map with both drop fields", - value: MapUpdate{ - DropSingleK: attribute.Key("key1"), - DropMultiK: []attribute.Key{ - attribute.Key("key1"), - attribute.Key("key2"), - }, - }, - init: []int{}, - wantKVs: []attribute.KeyValue{}, - }, - { - name: "map with all fields", - value: MapUpdate{ - DropSingleK: attribute.Key("key1"), - DropMultiK: []attribute.Key{ - attribute.Key("key1"), - attribute.Key("key2"), - }, - SingleKV: attribute.String("key4", "val4"), - MultiKV: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - init: []int{}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - attribute.String("key4", "val4"), - }, - }, - { - name: "Existing map with MultiKV", - value: MapUpdate{MultiKV: []attribute.KeyValue{ - attribute.Int64("key1", 1), - attribute.String("key2", "val2")}, - }, - init: []int{5}, - wantKVs: []attribute.KeyValue{ - attribute.Int64("key1", 1), - attribute.String("key2", "val2"), - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with SingleKV", - value: MapUpdate{SingleKV: attribute.String("key1", "val1")}, - init: []int{5}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with both add fields", - value: MapUpdate{SingleKV: attribute.Int64("key1", 3), - MultiKV: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2")}, - }, - init: []int{5}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with empty MapUpdate", - value: MapUpdate{}, - init: []int{5}, - wantKVs: []attribute.KeyValue{ - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with DropSingleK", - value: MapUpdate{DropSingleK: attribute.Key("key1")}, - init: []int{1, 5}, - wantKVs: []attribute.KeyValue{ - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with DropMultiK", - value: MapUpdate{DropMultiK: []attribute.Key{ - attribute.Key("key1"), attribute.Key("key2"), - }}, - init: []int{1, 5}, - wantKVs: []attribute.KeyValue{ - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with both drop fields", - value: MapUpdate{ - DropSingleK: attribute.Key("key1"), - DropMultiK: []attribute.Key{ - attribute.Key("key1"), - attribute.Key("key2"), - }, - }, - init: []int{1, 2, 5}, - wantKVs: []attribute.KeyValue{ - attribute.Int("key5", 5), - }, - }, - { - name: "Existing map with all the fields", - value: MapUpdate{ - DropSingleK: attribute.Key("key1"), - DropMultiK: []attribute.Key{ - attribute.Key("key1"), - attribute.Key("key2"), - attribute.Key("key5"), - attribute.Key("key6"), - }, - SingleKV: attribute.String("key4", "val4"), - MultiKV: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - init: []int{5, 6, 7}, - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - attribute.String("key4", "val4"), - attribute.Int("key7", 7), - }, - }, - } -} - -func makeTestMap(ints []int) Map { - r := make(rawMap, len(ints)) - for _, v := range ints { - r[attribute.Key(fmt.Sprintf("key%d", v))] = attribute.IntValue(v) - } - return newMap(r) -} diff --git a/internal/baggage/context.go b/internal/baggage/context.go new file mode 100644 index 00000000000..7d8694e8c68 --- /dev/null +++ b/internal/baggage/context.go @@ -0,0 +1,95 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package baggage + +import "context" + +type baggageContextKeyType int + +const baggageKey baggageContextKeyType = iota + +// SetHookFunc is a callback called when storing baggage in the context. +type SetHookFunc func(context.Context, List) context.Context + +// GetHookFunc is a callback called when getting baggage from the context. +type GetHookFunc func(context.Context, List) List + +type baggageState struct { + list List + + setHook SetHookFunc + getHook GetHookFunc +} + +// ContextWithSetHook returns a copy of parent with hook configured to be +// invoked every time ContextWithBaggage is called. +// +// Passing nil SetHookFunc creates a context with no set hook to call. +func ContextWithSetHook(parent context.Context, hook SetHookFunc) context.Context { + var s baggageState + switch v := parent.Value(baggageKey).(type) { + case baggageState: + s = v + } + + s.setHook = hook + return context.WithValue(parent, baggageKey, s) +} + +// ContextWithGetHook returns a copy of parent with hook configured to be +// invoked every time FromContext is called. +// +// Passing nil GetHookFunc creates a context with no get hook to call. +func ContextWithGetHook(parent context.Context, hook GetHookFunc) context.Context { + var s baggageState + switch v := parent.Value(baggageKey).(type) { + case baggageState: + s = v + } + + s.getHook = hook + return context.WithValue(parent, baggageKey, s) +} + +// ContextWithList returns a copy of parent with baggage. Passing nil list +// returns a context without any baggage. +func ContextWithList(parent context.Context, list List) context.Context { + var s baggageState + switch v := parent.Value(baggageKey).(type) { + case baggageState: + s = v + } + + s.list = list + ctx := context.WithValue(parent, baggageKey, s) + if s.setHook != nil { + ctx = s.setHook(ctx, list) + } + + return ctx +} + +// ListFromContext returns the baggage contained in ctx. +func ListFromContext(ctx context.Context) List { + switch v := ctx.Value(baggageKey).(type) { + case baggageState: + if v.getHook != nil { + return v.getHook(ctx, v.list) + } + return v.list + default: + return nil + } +} diff --git a/internal/baggage/context_test.go b/internal/baggage/context_test.go new file mode 100644 index 00000000000..ba98211232c --- /dev/null +++ b/internal/baggage/context_test.go @@ -0,0 +1,96 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package baggage + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestContextWithList(t *testing.T) { + ctx := context.Background() + l := List{"foo": {Value: "1"}} + + nCtx := ContextWithList(ctx, l) + assert.Equal(t, baggageState{list: l}, nCtx.Value(baggageKey)) + assert.Nil(t, ctx.Value(baggageKey)) +} + +func TestClearContextOfList(t *testing.T) { + l := List{"foo": {Value: "1"}} + + ctx := context.Background() + ctx = context.WithValue(ctx, baggageKey, l) + + nCtx := ContextWithList(ctx, nil) + nL, ok := nCtx.Value(baggageKey).(baggageState) + require.True(t, ok, "wrong type stored in context") + assert.Nil(t, nL.list) + assert.Equal(t, l, ctx.Value(baggageKey)) +} + +func TestListFromContext(t *testing.T) { + ctx := context.Background() + assert.Nil(t, ListFromContext(ctx)) + + l := List{"foo": {Value: "1"}} + ctx = context.WithValue(ctx, baggageKey, baggageState{list: l}) + assert.Equal(t, l, ListFromContext(ctx)) +} + +func TestContextWithSetHook(t *testing.T) { + var called bool + f := func(ctx context.Context, list List) context.Context { + called = true + return ctx + } + + ctx := context.Background() + ctx = ContextWithSetHook(ctx, f) + assert.False(t, called, "SetHookFunc called when setting hook") + ctx = ContextWithList(ctx, nil) + assert.True(t, called, "SetHookFunc not called when setting List") + + // Ensure resetting the hook works. + called = false + ctx = ContextWithSetHook(ctx, f) + assert.False(t, called, "SetHookFunc called when re-setting hook") + ContextWithList(ctx, nil) + assert.True(t, called, "SetHookFunc not called when re-setting List") +} + +func TestContextWithGetHook(t *testing.T) { + var called bool + f := func(ctx context.Context, list List) List { + called = true + return list + } + + ctx := context.Background() + ctx = ContextWithGetHook(ctx, f) + assert.False(t, called, "GetHookFunc called when setting hook") + _ = ListFromContext(ctx) + assert.True(t, called, "GetHookFunc not called when getting List") + + // Ensure resetting the hook works. + called = false + ctx = ContextWithGetHook(ctx, f) + assert.False(t, called, "GetHookFunc called when re-setting hook") + _ = ListFromContext(ctx) + assert.True(t, called, "GetHookFunc not called when re-getting List") +} From 1eac60a2dabb52287e37b49d5ab99f9b90838a00 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Jun 2021 10:20:37 -0700 Subject: [PATCH 10/20] Update OpenTracing bridge --- bridge/opentracing/bridge.go | 77 ++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 33 deletions(-) diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index a6af1fdf1aa..1656f67164b 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -27,16 +27,17 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/bridge/opentracing/migration" "go.opentelemetry.io/otel/codes" - "go.opentelemetry.io/otel/internal/baggage" + iBaggage "go.opentelemetry.io/otel/internal/baggage" "go.opentelemetry.io/otel/internal/trace/noop" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) type bridgeSpanContext struct { - baggageItems baggage.Map + bag baggage.Baggage otelSpanContext trace.SpanContext } @@ -44,7 +45,7 @@ var _ ot.SpanContext = &bridgeSpanContext{} func newBridgeSpanContext(otelSpanContext trace.SpanContext, parentOtSpanContext ot.SpanContext) *bridgeSpanContext { bCtx := &bridgeSpanContext{ - baggageItems: baggage.NewEmptyMap(), + bag: baggage.Baggage{}, otelSpanContext: otelSpanContext, } if parentOtSpanContext != nil { @@ -57,20 +58,25 @@ func newBridgeSpanContext(otelSpanContext trace.SpanContext, parentOtSpanContext } func (c *bridgeSpanContext) ForeachBaggageItem(handler func(k, v string) bool) { - c.baggageItems.Foreach(func(kv attribute.KeyValue) bool { - return handler(string(kv.Key), kv.Value.Emit()) - }) + for _, m := range c.bag.Members() { + if !handler(m.Key(), m.Value()) { + return + } + } } func (c *bridgeSpanContext) setBaggageItem(restrictedKey, value string) { crk := http.CanonicalHeaderKey(restrictedKey) - c.baggageItems = c.baggageItems.Apply(baggage.MapUpdate{SingleKV: attribute.String(crk, value)}) + m, err := baggage.NewMember(crk, value) + if err != nil { + return + } + c.bag, _ = c.bag.SetMember(m) } -func (c *bridgeSpanContext) baggageItem(restrictedKey string) string { +func (c *bridgeSpanContext) baggageItem(restrictedKey string) baggage.Member { crk := http.CanonicalHeaderKey(restrictedKey) - val, _ := c.baggageItems.Value(attribute.Key(crk)) - return val.Emit() + return c.bag.Member(crk) } type bridgeSpan struct { @@ -247,7 +253,7 @@ func (s *bridgeSpan) updateOTelContext(restrictedKey, value string) { } func (s *bridgeSpan) BaggageItem(restrictedKey string) string { - return s.ctx.baggageItem(restrictedKey) + return s.ctx.baggageItem(restrictedKey).Value() } func (s *bridgeSpan) Tracer() ot.Tracer { @@ -341,12 +347,12 @@ func (t *BridgeTracer) SetTextMapPropagator(propagator propagation.TextMapPropag } func (t *BridgeTracer) NewHookedContext(ctx context.Context) context.Context { - ctx = baggage.ContextWithSetHook(ctx, t.baggageSetHook) - ctx = baggage.ContextWithGetHook(ctx, t.baggageGetHook) + ctx = iBaggage.ContextWithSetHook(ctx, t.baggageSetHook) + ctx = iBaggage.ContextWithGetHook(ctx, t.baggageGetHook) return ctx } -func (t *BridgeTracer) baggageSetHook(ctx context.Context) context.Context { +func (t *BridgeTracer) baggageSetHook(ctx context.Context, list iBaggage.List) context.Context { span := ot.SpanFromContext(ctx) if span == nil { t.warningHandler("No active OpenTracing span, can not propagate the baggage items from OpenTelemetry context\n") @@ -357,38 +363,43 @@ func (t *BridgeTracer) baggageSetHook(ctx context.Context) context.Context { t.warningHandler("Encountered a foreign OpenTracing span, will not propagate the baggage items from OpenTelemetry context\n") return ctx } - // we clear the context only to avoid calling a get hook - // during MapFromContext, but otherwise we don't change the - // context, so we don't care about the old hooks. - clearCtx, _, _ := baggage.ContextWithNoHooks(ctx) - m := baggage.MapFromContext(clearCtx) - m.Foreach(func(kv attribute.KeyValue) bool { - bSpan.setBaggageItemOnly(string(kv.Key), kv.Value.Emit()) - return true - }) + for k, v := range list { + bSpan.setBaggageItemOnly(k, v.Value) + } return ctx } -func (t *BridgeTracer) baggageGetHook(ctx context.Context, m baggage.Map) baggage.Map { +func (t *BridgeTracer) baggageGetHook(ctx context.Context, list iBaggage.List) iBaggage.List { span := ot.SpanFromContext(ctx) if span == nil { t.warningHandler("No active OpenTracing span, can not propagate the baggage items from OpenTracing span context\n") - return m + return list } bSpan, ok := span.(*bridgeSpan) if !ok { t.warningHandler("Encountered a foreign OpenTracing span, will not propagate the baggage items from OpenTracing span context\n") - return m + return list } items := bSpan.extraBaggageItems if len(items) == 0 { - return m + return list } - kv := make([]attribute.KeyValue, 0, len(items)) + + // Privilege of using the internal representation of Baggage here comes + // with the responsibility to make sure we maintain its immutability. We + // need to return a copy to ensure this. + + merged := make(iBaggage.List, len(list)) + for k, v := range list { + merged[k] = v + } + for k, v := range items { - kv = append(kv, attribute.String(k, v)) + // Overwrite according to OpenTelemetry specification. + merged[k] = iBaggage.Item{Value: v} } - return m.Apply(baggage.MapUpdate{MultiKV: kv}) + + return merged } // StartSpan is a part of the implementation of the OpenTracing Tracer @@ -634,7 +645,7 @@ func (t *BridgeTracer) Inject(sm ot.SpanContext, format interface{}, carrier int sc: bridgeSC.otelSpanContext, } ctx := trace.ContextWithSpan(context.Background(), fs) - ctx = baggage.ContextWithMap(ctx, bridgeSC.baggageItems) + ctx = baggage.ContextWithBaggage(ctx, bridgeSC.bag) t.getPropagator().Inject(ctx, propagation.HeaderCarrier(header)) return nil } @@ -653,9 +664,9 @@ func (t *BridgeTracer) Extract(format interface{}, carrier interface{}) (ot.Span } header := http.Header(hhcarrier) ctx := t.getPropagator().Extract(context.Background(), propagation.HeaderCarrier(header)) - baggage := baggage.MapFromContext(ctx) + baggage := baggage.FromContext(ctx) bridgeSC := &bridgeSpanContext{ - baggageItems: baggage, + bag: baggage, otelSpanContext: trace.SpanContextFromContext(ctx), } if !bridgeSC.otelSpanContext.IsValid() { From 7fcf73c9cddaac69da203c3dc280f56510830135 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Jun 2021 10:20:47 -0700 Subject: [PATCH 11/20] Update baggage propagator --- propagation/baggage.go | 73 ++---------- propagation/baggage_test.go | 225 ++++++++++++++++++------------------ 2 files changed, 124 insertions(+), 174 deletions(-) diff --git a/propagation/baggage.go b/propagation/baggage.go index bc76191892e..16afed5319b 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -16,11 +16,8 @@ package propagation // import "go.opentelemetry.io/otel/propagation" import ( "context" - "net/url" - "strings" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/internal/baggage" + "go.opentelemetry.io/otel/baggage" ) const baggageHeader = "baggage" @@ -35,74 +32,24 @@ var _ TextMapPropagator = Baggage{} // Inject sets baggage key-values from ctx into the carrier. func (b Baggage) Inject(ctx context.Context, carrier TextMapCarrier) { - baggageMap := baggage.MapFromContext(ctx) - firstIter := true - var headerValueBuilder strings.Builder - baggageMap.Foreach(func(kv attribute.KeyValue) bool { - if !firstIter { - headerValueBuilder.WriteRune(',') - } - firstIter = false - headerValueBuilder.WriteString(url.QueryEscape(strings.TrimSpace((string)(kv.Key)))) - headerValueBuilder.WriteRune('=') - headerValueBuilder.WriteString(url.QueryEscape(strings.TrimSpace(kv.Value.Emit()))) - return true - }) - if headerValueBuilder.Len() > 0 { - headerString := headerValueBuilder.String() - carrier.Set(baggageHeader, headerString) + bStr := baggage.FromContext(ctx).String() + if bStr != "" { + carrier.Set(baggageHeader, bStr) } } // Extract returns a copy of parent with the baggage from the carrier added. func (b Baggage) Extract(parent context.Context, carrier TextMapCarrier) context.Context { - bVal := carrier.Get(baggageHeader) - if bVal == "" { + bStr := carrier.Get(baggageHeader) + if bStr == "" { return parent } - baggageValues := strings.Split(bVal, ",") - keyValues := make([]attribute.KeyValue, 0, len(baggageValues)) - for _, baggageValue := range baggageValues { - valueAndProps := strings.Split(baggageValue, ";") - if len(valueAndProps) < 1 { - continue - } - nameValue := strings.Split(valueAndProps[0], "=") - if len(nameValue) < 2 { - continue - } - name, err := url.QueryUnescape(nameValue[0]) - if err != nil { - continue - } - trimmedName := strings.TrimSpace(name) - value, err := url.QueryUnescape(nameValue[1]) - if err != nil { - continue - } - trimmedValue := strings.TrimSpace(value) - - // TODO (skaris): properties defiend https://w3c.github.io/correlation-context/, are currently - // just put as part of the value. - var trimmedValueWithProps strings.Builder - trimmedValueWithProps.WriteString(trimmedValue) - for _, prop := range valueAndProps[1:] { - trimmedValueWithProps.WriteRune(';') - trimmedValueWithProps.WriteString(prop) - } - - keyValues = append(keyValues, attribute.String(trimmedName, trimmedValueWithProps.String())) - } - - if len(keyValues) > 0 { - // Only update the context if valid values were found - return baggage.ContextWithMap(parent, baggage.NewMap(baggage.MapUpdate{ - MultiKV: keyValues, - })) + bag, err := baggage.Parse(bStr) + if err != nil { + return parent } - - return parent + return baggage.ContextWithBaggage(parent, bag) } // Fields returns the keys who's values are set with Inject. diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index f9749d620df..299e009e3cf 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -21,65 +21,101 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/internal/baggage" + "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/propagation" ) +type property struct { + Key, Value string +} + +type member struct { + Key, Value string + + Properties []property +} + +func (m member) Member(t *testing.T) baggage.Member { + props := make([]baggage.Property, 0, len(m.Properties)) + for _, p := range m.Properties { + p, err := baggage.NewKeyValueProperty(p.Key, p.Value) + if err != nil { + t.Fatal(err) + } + props = append(props, p) + } + + bMember, err := baggage.NewMember(m.Key, m.Value, props...) + if err != nil { + t.Fatal(err) + } + return bMember +} + +type members []member + +func (m members) Baggage(t *testing.T) baggage.Baggage { + bMembers := make([]baggage.Member, 0, len(m)) + for _, mem := range m { + bMembers = append(bMembers, mem.Member(t)) + } + bag, err := baggage.New(bMembers...) + if err != nil { + t.Fatal(err) + } + return bag +} + func TestExtractValidBaggageFromHTTPReq(t *testing.T) { prop := propagation.TextMapPropagator(propagation.Baggage{}) tests := []struct { - name string - header string - wantKVs []attribute.KeyValue + name string + header string + want members }{ { name: "valid w3cHeader", header: "key1=val1,key2=val2", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + want: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, }, }, { name: "valid w3cHeader with spaces", header: "key1 = val1, key2 =val2 ", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + want: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, }, }, { name: "valid w3cHeader with properties", header: "key1=val1,key2=val2;prop=1", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2;prop=1"), - }, - }, - { - name: "valid header with url-escaped comma", - header: "key1=val1,key2=val2%2Cval3", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2,val3"), + want: members{ + {Key: "key1", Value: "val1"}, + { + Key: "key2", + Value: "val2", + Properties: []property{ + {Key: "prop", Value: "1"}, + }, + }, }, }, { name: "valid header with an invalid header", header: "key1=val1,key2=val2,a,val3", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - }, + want: members{}, }, { name: "valid header with no value", header: "key1=,key2=val2", - wantKVs: []attribute.KeyValue{ - attribute.String("key1", ""), - attribute.String("key2", "val2"), + want: members{ + {Key: "key1", Value: ""}, + {Key: "key2", Value: "val2"}, }, }, } @@ -91,27 +127,8 @@ func TestExtractValidBaggageFromHTTPReq(t *testing.T) { ctx := context.Background() ctx = prop.Extract(ctx, propagation.HeaderCarrier(req.Header)) - gotBaggage := baggage.MapFromContext(ctx) - wantBaggage := baggage.NewMap(baggage.MapUpdate{MultiKV: tt.wantKVs}) - if gotBaggage.Len() != wantBaggage.Len() { - t.Errorf( - "Got and Want Baggage are not the same size %d != %d", - gotBaggage.Len(), - wantBaggage.Len(), - ) - } - totalDiff := "" - wantBaggage.Foreach(func(keyValue attribute.KeyValue) bool { - val, _ := gotBaggage.Value(keyValue.Key) - diff := cmp.Diff(keyValue, attribute.KeyValue{Key: keyValue.Key, Value: val}, cmp.AllowUnexported(attribute.Value{})) - if diff != "" { - totalDiff += diff + "\n" - } - return true - }) - if totalDiff != "" { - t.Errorf("Extract Tracecontext: %s: -got +want %s", tt.name, totalDiff) - } + expected := tt.want.Baggage(t) + assert.Equal(t, expected, baggage.FromContext(ctx)) }) } } @@ -121,7 +138,7 @@ func TestExtractInvalidDistributedContextFromHTTPReq(t *testing.T) { tests := []struct { name string header string - hasKVs []attribute.KeyValue + has members }{ { name: "no key values", @@ -130,17 +147,31 @@ func TestExtractInvalidDistributedContextFromHTTPReq(t *testing.T) { { name: "invalid header with existing context", header: "header2", - hasKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + has: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, }, }, { name: "empty header value", header: "", - hasKVs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + has: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, + }, + }, + { + name: "with properties", + header: "key1=val1,key2=val2;prop=1", + has: members{ + {Key: "key1", Value: "val1"}, + { + Key: "key2", + Value: "val2", + Properties: []property{ + {Key: "prop", Value: "1"}, + }, + }, }, }, } @@ -150,26 +181,10 @@ func TestExtractInvalidDistributedContextFromHTTPReq(t *testing.T) { req, _ := http.NewRequest("GET", "http://example.com", nil) req.Header.Set("baggage", tt.header) - ctx := baggage.NewContext(context.Background(), tt.hasKVs...) - wantBaggage := baggage.MapFromContext(ctx) + expected := tt.has.Baggage(t) + ctx := baggage.ContextWithBaggage(context.Background(), expected) ctx = prop.Extract(ctx, propagation.HeaderCarrier(req.Header)) - gotBaggage := baggage.MapFromContext(ctx) - if gotBaggage.Len() != wantBaggage.Len() { - t.Errorf( - "Got and Want Baggage are not the same size %d != %d", - gotBaggage.Len(), - wantBaggage.Len(), - ) - } - totalDiff := "" - wantBaggage.Foreach(func(keyValue attribute.KeyValue) bool { - val, _ := gotBaggage.Value(keyValue.Key) - diff := cmp.Diff(keyValue, attribute.KeyValue{Key: keyValue.Key, Value: val}, cmp.AllowUnexported(attribute.Value{})) - if diff != "" { - totalDiff += diff + "\n" - } - return true - }) + assert.Equal(t, expected, baggage.FromContext(ctx)) }) } } @@ -178,62 +193,50 @@ func TestInjectBaggageToHTTPReq(t *testing.T) { propagator := propagation.Baggage{} tests := []struct { name string - kvs []attribute.KeyValue + mems members wantInHeader []string - wantedLen int }{ { name: "two simple values", - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), + mems: members{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, }, wantInHeader: []string{"key1=val1", "key2=val2"}, }, { - name: "two values with escaped chars", - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1,val2"), - attribute.String("key2", "val3=4"), + name: "values with escaped chars", + mems: members{ + {Key: "key2", Value: "val3=4"}, }, - wantInHeader: []string{"key1=val1%2Cval2", "key2=val3%3D4"}, + wantInHeader: []string{"key2=val3%3D4"}, }, { - name: "values of non-string types", - kvs: []attribute.KeyValue{ - attribute.Bool("key1", true), - attribute.Int("key2", 123), - attribute.Int64("key3", 123), - attribute.Float64("key4", 123.567), + name: "with properties", + mems: members{ + {Key: "key1", Value: "val1"}, + { + Key: "key2", + Value: "val2", + Properties: []property{ + {Key: "prop", Value: "1"}, + }, + }, }, wantInHeader: []string{ - "key1=true", - "key2=123", - "key3=123", - "key4=123.567", + "key1=val1", + "key2=val2;prop=1", }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { req, _ := http.NewRequest("GET", "http://example.com", nil) - ctx := baggage.ContextWithMap(context.Background(), baggage.NewMap(baggage.MapUpdate{MultiKV: tt.kvs})) + ctx := baggage.ContextWithBaggage(context.Background(), tt.mems.Baggage(t)) propagator.Inject(ctx, propagation.HeaderCarrier(req.Header)) - gotHeader := req.Header.Get("baggage") - wantedLen := len(strings.Join(tt.wantInHeader, ",")) - if wantedLen != len(gotHeader) { - t.Errorf( - "%s: Inject baggage incorrect length %d != %d.", tt.name, tt.wantedLen, len(gotHeader), - ) - } - for _, inHeader := range tt.wantInHeader { - if !strings.Contains(gotHeader, inHeader) { - t.Errorf( - "%s: Inject baggage missing part of header: %s in %s", tt.name, inHeader, gotHeader, - ) - } - } + got := strings.Split(req.Header.Get("baggage"), ",") + assert.ElementsMatch(t, tt.wantInHeader, got) }) } } From 00ed8b7716d8304b0103536d1ed67f75bca08396 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Jun 2021 12:02:33 -0700 Subject: [PATCH 12/20] Fix lint and test errors --- baggage/baggage.go | 10 ++++-- baggage/baggage_test.go | 1 + baggage/context_test.go | 1 + bridge/opentracing/internal/mock.go | 55 +++++++++++++++++------------ bridge/opentracing/mix_test.go | 22 ++++++++---- 5 files changed, 58 insertions(+), 31 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index cfaa53a3bb3..f59f977a9d2 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -326,7 +326,10 @@ func New(members ...Member) (Baggage, error) { return Baggage{}, err } // OpenTelemetry resolves duplicates by last-one-wins. - b[m.key] = baggage.Item{m.value, m.properties.asInternal()} + b[m.key] = baggage.Item{ + Value: m.value, + Properties: m.properties.asInternal(), + } } // Check member numbers after deduplicating. @@ -366,7 +369,10 @@ func Parse(bStr string) (Baggage, error) { return Baggage{}, err } // OpenTelemetry resolves duplicates by last-one-wins. - b[m.key] = baggage.Item{m.value, m.properties.asInternal()} + b[m.key] = baggage.Item{ + Value: m.value, + Properties: m.properties.asInternal(), + } } // OpenTelemetry does not allow for duplicate list-members, but the W3C diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 751ee5e2ebf..681d014c873 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/internal/baggage" ) diff --git a/baggage/context_test.go b/baggage/context_test.go index 31ad836488a..1b32acdb54d 100644 --- a/baggage/context_test.go +++ b/baggage/context_test.go @@ -19,6 +19,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/internal/baggage" ) diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index e57d9129b85..6bbbb1c71bd 100644 --- a/bridge/opentracing/internal/mock.go +++ b/bridge/opentracing/internal/mock.go @@ -23,7 +23,6 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" - "go.opentelemetry.io/otel/internal/baggage" "go.opentelemetry.io/otel/semconv" "go.opentelemetry.io/otel/trace" @@ -45,7 +44,6 @@ type MockContextKeyValue struct { } type MockTracer struct { - Resources baggage.Map FinishedSpans []*MockSpan SpareTraceIDs []trace.TraceID SpareSpanIDs []trace.SpanID @@ -60,7 +58,6 @@ var _ migration.DeferredContextSetupTracerExtension = &MockTracer{} func NewMockTracer() *MockTracer { return &MockTracer{ - Resources: baggage.NewEmptyMap(), FinishedSpans: nil, SpareTraceIDs: nil, SpareSpanIDs: nil, @@ -85,14 +82,12 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...trace.SpanO mockTracer: t, officialTracer: t, spanContext: spanContext, - Attributes: baggage.NewMap(baggage.MapUpdate{ - MultiKV: config.Attributes, - }), - StartTime: startTime, - EndTime: time.Time{}, - ParentSpanID: t.getParentSpanID(ctx, config), - Events: nil, - SpanKind: trace.ValidateSpanKind(config.SpanKind), + Attributes: config.Attributes, + StartTime: startTime, + EndTime: time.Time{}, + ParentSpanID: t.getParentSpanID(ctx, config), + Events: nil, + SpanKind: trace.ValidateSpanKind(config.SpanKind), } if !migration.SkipContextSetup(ctx) { ctx = trace.ContextWithSpan(ctx, span) @@ -182,7 +177,7 @@ func (t *MockTracer) DeferredContextSetupHook(ctx context.Context, span trace.Sp type MockEvent struct { Timestamp time.Time Name string - Attributes baggage.Map + Attributes []attribute.KeyValue } type MockSpan struct { @@ -192,7 +187,7 @@ type MockSpan struct { SpanKind trace.SpanKind recording bool - Attributes baggage.Map + Attributes []attribute.KeyValue StartTime time.Time EndTime time.Time ParentSpanID trace.SpanID @@ -223,13 +218,29 @@ func (s *MockSpan) SetError(v bool) { } func (s *MockSpan) SetAttributes(attributes ...attribute.KeyValue) { - s.applyUpdate(baggage.MapUpdate{ - MultiKV: attributes, - }) + s.applyUpdate(attributes) } -func (s *MockSpan) applyUpdate(update baggage.MapUpdate) { - s.Attributes = s.Attributes.Apply(update) +func (s *MockSpan) applyUpdate(update []attribute.KeyValue) { + updateM := make(map[attribute.Key]attribute.Value, len(update)) + for _, kv := range update { + updateM[kv.Key] = kv.Value + } + + seen := make(map[attribute.Key]struct{}) + for i, kv := range s.Attributes { + if v, ok := updateM[kv.Key]; ok { + s.Attributes[i].Value = v + seen[kv.Key] = struct{}{} + } + } + + for k, v := range updateM { + if _, ok := seen[k]; ok { + continue + } + s.Attributes = append(s.Attributes, attribute.KeyValue{Key: k, Value: v}) + } } func (s *MockSpan) End(options ...trace.SpanOption) { @@ -269,11 +280,9 @@ func (s *MockSpan) Tracer() trace.Tracer { func (s *MockSpan) AddEvent(name string, o ...trace.EventOption) { c := trace.NewEventConfig(o...) s.Events = append(s.Events, MockEvent{ - Timestamp: c.Timestamp, - Name: name, - Attributes: baggage.NewMap(baggage.MapUpdate{ - MultiKV: c.Attributes, - }), + Timestamp: c.Timestamp, + Name: name, + Attributes: c.Attributes, }) } diff --git a/bridge/opentracing/mix_test.go b/bridge/opentracing/mix_test.go index 8dc9445b688..5324274bb5a 100644 --- a/bridge/opentracing/mix_test.go +++ b/bridge/opentracing/mix_test.go @@ -23,7 +23,7 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" - otelbaggage "go.opentelemetry.io/otel/internal/baggage" + "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/trace" "go.opentelemetry.io/otel/bridge/opentracing/internal" @@ -515,7 +515,18 @@ func (bio *baggageInteroperationTest) addAndRecordBaggage(t *testing.T, ctx cont value := bio.baggageItems[idx].value otSpan.SetBaggageItem(otKey, value) - ctx = otelbaggage.NewContext(ctx, attribute.String(otelKey, value)) + + m, err := baggage.NewMember(otelKey, value) + if err != nil { + t.Error(err) + return ctx + } + b, err := baggage.FromContext(ctx).SetMember(m) + if err != nil { + t.Error(err) + return ctx + } + ctx = baggage.ContextWithBaggage(ctx, b) otRecording := make(map[string]string) otSpan.Context().ForeachBaggageItem(func(key, value string) bool { @@ -523,10 +534,9 @@ func (bio *baggageInteroperationTest) addAndRecordBaggage(t *testing.T, ctx cont return true }) otelRecording := make(map[string]string) - otelbaggage.MapFromContext(ctx).Foreach(func(kv attribute.KeyValue) bool { - otelRecording[string(kv.Key)] = kv.Value.Emit() - return true - }) + for _, m := range baggage.FromContext(ctx).Members() { + otelRecording[m.Key()] = m.Value() + } bio.recordedOTBaggage = append(bio.recordedOTBaggage, otRecording) bio.recordedOtelBaggage = append(bio.recordedOtelBaggage, otelRecording) return ctx From ecaccc545a0fd4546df8ccdda584d6380bec4525 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Jun 2021 12:19:04 -0700 Subject: [PATCH 13/20] Add changes to changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82e7ea7ff18..6f9c702ae80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm This method returns the number of list-members the `TraceState` holds. (#1937) - Creates package `go.opentelemetry.io/otel/exporters/otlp/otlptrace` that defines a trace exporter that uses a `otlptrace.Client` to send data. Creates package `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` implementing a gRPC `otlptrace.Client` and offers convenience functions, `NewExportPipeline` and `InstallNewPipeline`, to setup and install a `otlptrace.Exporter` in tracing .(#1922) +- The `Baggage`, `Member`, and `Property` types are added to the `go.opentelemetry.io/otel/baggage` package along with their related functions. (TBD) +- The new `ContextWithBaggage`, `ContextWithoutBaggage`, and `FromContext` functions were added to the `go.opentelemetry.io/otel/baggage` package. + These functions replace the `Set`, `Value`, `ContextWithValue`, `ContextWithoutValue`, and `ContextWithEmpty` functions from that package and directly work with the new `Baggage` type. (TBD) ### Changed @@ -87,6 +90,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm The `"go.opentelemetry.io/otel".Tracer` function or a `TracerProvider` should be used to acquire a library specific `Tracer` instead. (#1900) - The `http.url` attribute generated by `HTTPClientAttributesFromHTTPRequest` will no longer include username or password information. (#1919) - The `IsEmpty` method of the `TraceState` type in the `go.opentelemetry.io/otel/trace` package is removed in favor of using the added `TraceState.Len` method. (#1931) +- The `Set`, `Value`, `ContextWithValue`, `ContextWithoutValue`, and `ContextWithEmpty` functions in the `go.opentelemetry.io/otel/baggage` package are removed. + Handling of baggage is now done using the added `Baggage` type and related context functions (`ContextWithBaggage`, `ContextWithoutBaggage`, and `FromContext`) in that package. (TBD) ### Fixed From 0eedf6ce6e63a6155066481977aec7a0804ed5ea Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Jun 2021 21:38:01 +0000 Subject: [PATCH 14/20] Apply suggestions from code review --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f9c702ae80..111aba586ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,9 +33,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm This method returns the number of list-members the `TraceState` holds. (#1937) - Creates package `go.opentelemetry.io/otel/exporters/otlp/otlptrace` that defines a trace exporter that uses a `otlptrace.Client` to send data. Creates package `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` implementing a gRPC `otlptrace.Client` and offers convenience functions, `NewExportPipeline` and `InstallNewPipeline`, to setup and install a `otlptrace.Exporter` in tracing .(#1922) -- The `Baggage`, `Member`, and `Property` types are added to the `go.opentelemetry.io/otel/baggage` package along with their related functions. (TBD) +- The `Baggage`, `Member`, and `Property` types are added to the `go.opentelemetry.io/otel/baggage` package along with their related functions. (#1967) - The new `ContextWithBaggage`, `ContextWithoutBaggage`, and `FromContext` functions were added to the `go.opentelemetry.io/otel/baggage` package. - These functions replace the `Set`, `Value`, `ContextWithValue`, `ContextWithoutValue`, and `ContextWithEmpty` functions from that package and directly work with the new `Baggage` type. (TBD) + These functions replace the `Set`, `Value`, `ContextWithValue`, `ContextWithoutValue`, and `ContextWithEmpty` functions from that package and directly work with the new `Baggage` type. (#1967) ### Changed From 169bbc84cbd864d12b70dc4efe486ecdfd3b70b2 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Sun, 6 Jun 2021 10:59:11 -0700 Subject: [PATCH 15/20] Rename testcase field per suggestion --- baggage/baggage_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 681d014c873..42dc0df9fd9 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -233,34 +233,34 @@ func TestBaggageParse(t *testing.T) { tooManyMembers := strings.Join(m, listDelimiter) testcases := []struct { - name string - in string - baggage baggage.List - err error + name string + in string + want baggage.List + err error }{ { - name: "empty value", - in: "", - baggage: baggage.List(nil), + name: "empty value", + in: "", + want: baggage.List(nil), }, { name: "single member empty value no properties", in: "foo=", - baggage: baggage.List{ + want: baggage.List{ "foo": {Value: ""}, }, }, { name: "single member no properties", in: "foo=1", - baggage: baggage.List{ + want: baggage.List{ "foo": {Value: "1"}, }, }, { name: "single member empty value with properties", in: "foo=;state=on;red", - baggage: baggage.List{ + want: baggage.List{ "foo": { Value: "", Properties: []baggage.Property{ @@ -273,7 +273,7 @@ func TestBaggageParse(t *testing.T) { { name: "single member with properties", in: "foo=1;state=on;red", - baggage: baggage.List{ + want: baggage.List{ "foo": { Value: "1", Properties: []baggage.Property{ @@ -286,7 +286,7 @@ func TestBaggageParse(t *testing.T) { { name: "two members with properties", in: "foo=1;state=on;red,bar=2;yellow", - baggage: baggage.List{ + want: baggage.List{ "foo": { Value: "1", Properties: []baggage.Property{ @@ -304,7 +304,7 @@ func TestBaggageParse(t *testing.T) { // According to the OTel spec, last value wins. name: "duplicate key", in: "foo=1;state=on;red,foo=2", - baggage: baggage.List{ + want: baggage.List{ "foo": {Value: "2"}, }, }, @@ -349,7 +349,7 @@ func TestBaggageParse(t *testing.T) { t.Run(tc.name, func(t *testing.T) { actual, err := Parse(tc.in) assert.ErrorIs(t, err, tc.err) - assert.Equal(t, Baggage{list: tc.baggage}, actual) + assert.Equal(t, Baggage{list: tc.want}, actual) }) } } From 3c2e1b6a7ff6a5732584e8f3afa7b3e2079cb486 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Sun, 6 Jun 2021 11:02:27 -0700 Subject: [PATCH 16/20] Update test to verify last-one-wins semantics --- baggage/baggage_test.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 42dc0df9fd9..e89bef98d81 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -178,14 +178,24 @@ func TestNewBaggage(t *testing.T) { } func TestNewBaggageWithDuplicates(t *testing.T) { + // Having this many members would normally cause this to error, but since + // these are duplicates of the same key they will be collapsed into a + // single entry. m := make([]Member, maxMembers+1) for i := range m { // Duplicates are collapsed. - m[i] = Member{key: "a"} + m[i] = Member{ + key: "a", + value: fmt.Sprintf("%d", i), + } } b, err := New(m...) assert.NoError(t, err) - assert.Equal(t, Baggage{list: baggage.List{"a": {}}}, b) + + // Ensure that the last-one-wins by verifying the value. + v := fmt.Sprintf("%d", maxMembers) + want := Baggage{list: baggage.List{"a": {Value: v}}} + assert.Equal(t, want, b) } func TestNewBaggageErrorInvalidMember(t *testing.T) { From 99be0fdb0c52fd2d120dfdacb8934d1f65ab2b50 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Sun, 6 Jun 2021 11:15:12 -0700 Subject: [PATCH 17/20] Explicitly seed random numbers with static seed in tests --- baggage/baggage_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index e89bef98d81..3d2364b4458 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -26,6 +26,13 @@ import ( "go.opentelemetry.io/otel/internal/baggage" ) +var rng *rand.Rand + +func init() { + // Seed with a static value to ensure deterministic results. + rng = rand.New(rand.NewSource(1)) +} + func TestKeyRegExp(t *testing.T) { // ASCII only invalidKeyRune := []rune{ @@ -208,7 +215,7 @@ func key(n int) string { b := make([]rune, n) for i := range b { - b[i] = r[rand.Intn(len(r))] + b[i] = r[rng.Intn(len(r))] } return string(b) } From ecea2677cbe710857f916dc64438cb6312075a45 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 7 Jun 2021 12:38:33 -0700 Subject: [PATCH 18/20] Parse Member key/value with string split --- baggage/baggage.go | 24 +++++++++++++++++------- baggage/baggage_test.go | 19 +++++++++++++++++-- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index f59f977a9d2..a9c7d614bcc 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -30,17 +30,17 @@ const ( maxBytesPerBaggageString = 8192 listDelimiter = "," + keyValueDelimiter = "=" propertyDelimiter = ";" keyDef = `([\x21\x23-\x27\x2A\x2B\x2D\x2E\x30-\x39\x41-\x5a\x5e-\x7a\x7c\x7e]+)` valueDef = `([\x21\x23-\x2b\x2d-\x3a\x3c-\x5B\x5D-\x7e]*)` - keyValueDef = `\s*` + keyDef + `\s*=\s*` + valueDef + `\s*` + keyValueDef = `\s*` + keyDef + `\s*` + keyValueDelimiter + `\s*` + valueDef + `\s*` ) var ( keyRe = regexp.MustCompile(`^` + keyDef + `$`) valueRe = regexp.MustCompile(`^` + valueDef + `$`) - keyValueRe = regexp.MustCompile(`^` + keyValueDef + `$`) propertyRe = regexp.MustCompile(`^(?:\s*` + keyDef + `\s*|` + keyValueDef + `)$`) ) @@ -145,7 +145,7 @@ func (p Property) Value() (string, bool) { // specification. func (p Property) String() string { if p.hasValue { - return fmt.Sprintf("%s=%v", p.key, p.value) + return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, p.value) } return p.key } @@ -250,6 +250,7 @@ func parseMember(member string) (Member, error) { parts := strings.SplitN(member, propertyDelimiter, 2) switch len(parts) { case 2: + // Parse the member properties. for _, pStr := range strings.Split(parts[1], propertyDelimiter) { p, err := parseProperty(pStr) if err != nil { @@ -259,11 +260,20 @@ func parseMember(member string) (Member, error) { } fallthrough case 1: - match := keyValueRe.FindStringSubmatch(parts[0]) - if len(match) != 3 { + // Parse the member key/value pair. + + // 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) } - key, value = match[1], match[2] + key, value = kv[0], kv[1] + if !keyRe.MatchString(key) { + return Member{}, fmt.Errorf("%w: %q", errInvalidKey, key) + } + if !valueRe.MatchString(value) { + return Member{}, fmt.Errorf("%w: %q", errInvalidValue, value) + } default: // This should never happen unless a developer has changed the string // splitting somehow. Panic instead of failing silently and allowing @@ -299,7 +309,7 @@ func (m Member) Properties() []Property { return m.properties.Copy() } // specification. func (m Member) String() string { // A key is just an ASCII string, but a value is URL encoded UTF-8. - s := fmt.Sprintf("%s=%s", m.key, url.QueryEscape(m.value)) + s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, url.QueryEscape(m.value)) if len(m.properties) > 0 { s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String()) } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 3d2364b4458..ee3e1bf9f34 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -326,14 +326,29 @@ func TestBaggageParse(t *testing.T) { }, }, { - name: "invalid member: empty key", + name: "invalid member: empty", in: "foo=,,bar=", err: errInvalidMember, }, + { + name: "invalid member: no key", + in: "=foo", + err: errInvalidKey, + }, + { + name: "invalid member: no value", + in: "foo", + err: errInvalidMember, + }, + { + name: "invalid member: invalid key", + in: "\\=value", + err: errInvalidKey, + }, { name: "invalid member: invalid value", in: "foo=\\", - err: errInvalidMember, + err: errInvalidValue, }, { name: "invalid property: invalid key", From 9138a0bdf967b460c1c2956dbd8eb93bd2fc463d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 7 Jun 2021 12:48:33 -0700 Subject: [PATCH 19/20] Add test for member parse with equal signs in value --- baggage/baggage_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index ee3e1bf9f34..8bd3bdd7d90 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -300,6 +300,13 @@ func TestBaggageParse(t *testing.T) { }, }, }, + { + name: "single member with value containing equal signs", + in: "foo=0=0=0", + want: baggage.List{ + "foo": {Value: "0=0=0"}, + }, + }, { name: "two members with properties", in: "foo=1;state=on;red,bar=2;yellow", From aca26acc2d182f5e2682041a3b94d6b90fcf9862 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 7 Jun 2021 14:07:41 -0700 Subject: [PATCH 20/20] Trim whitespaces for member key/value --- baggage/baggage.go | 4 +++- baggage/baggage_test.go | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index a9c7d614bcc..8de2bc9df12 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -267,7 +267,9 @@ func parseMember(member string) (Member, error) { if len(kv) != 2 { return Member{}, fmt.Errorf("%w: %q", errInvalidMember, member) } - key, value = kv[0], kv[1] + // "Leading and trailing whitespaces are allowed but MUST be trimmed + // when converting the header into a data structure." + key, value = strings.TrimSpace(kv[0]), strings.TrimSpace(kv[1]) if !keyRe.MatchString(key) { return Member{}, fmt.Errorf("%w: %q", errInvalidKey, key) } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 8bd3bdd7d90..0635c823e9e 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -274,6 +274,13 @@ func TestBaggageParse(t *testing.T) { "foo": {Value: "1"}, }, }, + { + name: "single member with spaces", + in: " foo \t= 1\t\t ", + want: baggage.List{ + "foo": {Value: "1"}, + }, + }, { name: "single member empty value with properties", in: "foo=;state=on;red",