From eddc8384df369c7431c8fcbee88a27871e186616 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Tue, 5 Dec 2023 00:14:39 +0000 Subject: [PATCH 01/22] Improve NewMember performance by ~9x MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/baggage cpu: AMD EPYC 7B13 │ /tmp/old.txt │ /tmp/new.txt │ │ sec/op │ sec/op vs base │ NewMember-96 368.20n ± 21% 46.94n ± 3% -87.25% (p=0.000 n=10) --- baggage/baggage.go | 50 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 84532cb1da3..f7e556d4d5a 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -33,14 +33,15 @@ const ( keyValueDelimiter = "=" propertyDelimiter = ";" + // if you update these 2 values you must update the static implementation below: keyReValidator and valReValidator 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*` + keyValueDelimiter + `\s*` + valueDef + `\s*` ) var ( - keyRe = regexp.MustCompile(`^` + keyDef + `$`) - valueRe = regexp.MustCompile(`^` + valueDef + `$`) + keyRe = keyReValidator{} + valueRe = valReValidator{} propertyRe = regexp.MustCompile(`^(?:\s*` + keyDef + `\s*|` + keyValueDef + `)$`) ) @@ -550,3 +551,48 @@ func (b Baggage) String() string { } return strings.Join(members, listDelimiter) } + +// They must follow the following rules (regex syntax): +// 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]*)` +type keyReValidator struct{} + +func (keyReValidator) MatchString(s string) bool { + if len(s) == 0 { + return false + } + + for _, c := range s { + if !(c >= 0x23 && c <= 0x27) && + !(c >= 0x30 && c <= 0x39) && + !(c >= 0x41 && c <= 0x5a) && + !(c >= 0x5e && c <= 0x7a) && + c != 0x21 && + c != 0x2a && + c != 0x2b && + c != 0x2d && + c != 0x2e && + c != 0x7c && + c != 0x7e { + return false + } + } + + return true +} + +type valReValidator struct{} + +func (valReValidator) MatchString(s string) bool { + for _, c := range s { + if !(c >= 0x23 && c <= 0x2b) && + !(c >= 0x2d && c <= 0x3a) && + !(c >= 0x3c && c <= 0x5b) && + !(c >= 0x5d && c <= 0x7e) && + c != 0x21 { + return false + } + } + + return true +} From d657b76271e008e868dd5503ba035950cc328b61 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Tue, 5 Dec 2023 00:30:48 +0000 Subject: [PATCH 02/22] Update Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0abcf5fb1d..479d99d6567 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Improve `go.opentelemetry.io/otel/trace.TraceState`'s performance. (#4722) - Improve `go.opentelemetry.io/otel/propagation.TraceContext`'s performance. (#4721) +- Improve `go.opentelemetry.io/otel/baggage.NewMember`'s performance. (#4743) ## [1.21.0/0.44.0] 2023-11-16 From 903d51cadd2e3b1a4d24bda3862e7f95289cdff6 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Tue, 5 Dec 2023 20:12:34 +0000 Subject: [PATCH 03/22] Fix lint --- baggage/baggage.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index f7e556d4d5a..44faab27de3 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -33,7 +33,7 @@ const ( keyValueDelimiter = "=" propertyDelimiter = ";" - // if you update these 2 values you must update the static implementation below: keyReValidator and valReValidator + // if you update these 2 values you must update the static implementation below: keyReValidator and valReValidator. 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*` + keyValueDelimiter + `\s*` + valueDef + `\s*` @@ -554,7 +554,7 @@ func (b Baggage) String() string { // They must follow the following rules (regex syntax): // 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]*)` +// valueDef = `([\x21\x23-\x2b\x2d-\x3a\x3c-\x5B\x5D-\x7e]*)`. type keyReValidator struct{} func (keyReValidator) MatchString(s string) bool { From 62539fb5944d596dfbeb0b5d5d3423473f89d313 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Wed, 6 Dec 2023 16:37:05 +0000 Subject: [PATCH 04/22] Make types as functions --- baggage/baggage.go | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 44faab27de3..18d0a50c06d 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -40,8 +40,6 @@ const ( ) var ( - keyRe = keyReValidator{} - valueRe = valReValidator{} propertyRe = regexp.MustCompile(`^(?:\s*` + keyDef + `\s*|` + keyValueDef + `)$`) ) @@ -68,7 +66,7 @@ type Property struct { // // If key is invalid, an error will be returned. func NewKeyProperty(key string) (Property, error) { - if !keyRe.MatchString(key) { + if !keyMatchString(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } @@ -80,10 +78,10 @@ func NewKeyProperty(key string) (Property, error) { // // If key or value are invalid, an error will be returned. func NewKeyValueProperty(key, value string) (Property, error) { - if !keyRe.MatchString(key) { + if !keyMatchString(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } - if !valueRe.MatchString(value) { + if !valueMatchString(value) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) } @@ -131,10 +129,10 @@ func (p Property) validate() error { return fmt.Errorf("invalid property: %w", err) } - if !keyRe.MatchString(p.key) { + if !keyMatchString(p.key) { return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key)) } - if p.hasValue && !valueRe.MatchString(p.value) { + if p.hasValue && !valueMatchString(p.value) { return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value)) } if !p.hasValue && p.value != "" { @@ -306,10 +304,10 @@ func parseMember(member string) (Member, error) { if err != nil { return newInvalidMember(), fmt.Errorf("%w: %q", err, value) } - if !keyRe.MatchString(key) { + if !keyMatchString(key) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key) } - if !valueRe.MatchString(value) { + if !valueMatchString(value) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) } @@ -324,10 +322,10 @@ func (m Member) validate() error { return fmt.Errorf("%w: %q", errInvalidMember, m) } - if !keyRe.MatchString(m.key) { + if !keyMatchString(m.key) { return fmt.Errorf("%w: %q", errInvalidKey, m.key) } - if !valueRe.MatchString(m.value) { + if !valueMatchString(m.value) { return fmt.Errorf("%w: %q", errInvalidValue, m.value) } return m.properties.validate() @@ -555,9 +553,8 @@ func (b Baggage) String() string { // They must follow the following rules (regex syntax): // 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]*)`. -type keyReValidator struct{} -func (keyReValidator) MatchString(s string) bool { +func keyMatchString(s string) bool { if len(s) == 0 { return false } @@ -581,9 +578,7 @@ func (keyReValidator) MatchString(s string) bool { return true } -type valReValidator struct{} - -func (valReValidator) MatchString(s string) bool { +func valueMatchString(s string) bool { for _, c := range s { if !(c >= 0x23 && c <= 0x2b) && !(c >= 0x2d && c <= 0x3a) && From 3f3e54f082fab829d116f3d97f2909787087c4c4 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Wed, 6 Dec 2023 17:16:33 +0000 Subject: [PATCH 05/22] Remove property regex too --- baggage/baggage.go | 119 ++++++++++++++++++++++++++++++---------- baggage/baggage_test.go | 5 +- 2 files changed, 93 insertions(+), 31 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 18d0a50c06d..59c481b6bc0 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -18,7 +18,6 @@ import ( "errors" "fmt" "net/url" - "regexp" "strings" "go.opentelemetry.io/otel/internal/baggage" @@ -32,15 +31,6 @@ const ( listDelimiter = "," keyValueDelimiter = "=" propertyDelimiter = ";" - - // if you update these 2 values you must update the static implementation below: keyReValidator and valReValidator. - 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*` + keyValueDelimiter + `\s*` + valueDef + `\s*` -) - -var ( - propertyRe = regexp.MustCompile(`^(?:\s*` + keyDef + `\s*|` + keyValueDef + `)$`) ) var ( @@ -105,7 +95,7 @@ func parseProperty(property string) (Property, error) { return newInvalidProperty(), nil } - match := propertyRe.FindStringSubmatch(property) + match := propertyFindStringSubmatch(property) if len(match) != 4 { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidProperty, property) } @@ -552,7 +542,72 @@ func (b Baggage) String() string { // They must follow the following rules (regex syntax): // 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]*)`. +// valueDef = `([\x21\x23-\x2b\x2d-\x3a\x3c-\x5B\x5D-\x7e]*)` +// propertyRe ^(?:\s*([\x21\x23-\x27\x2A\x2B\x2D\x2E\x30-\x39\x41-\x5a\x5e-\x7a\x7c\x7e]+)\s*|\s*([\x21\x23-\x27\x2A\x2B\x2D\x2E\x30-\x39\x41-\x5a\x5e-\x7a\x7c\x7e]+)\s*=\s*([\x21\x23-\x2b\x2d-\x3a\x3c-\x5B\x5D-\x7e]*)\s*)$. + +func propertyFindStringSubmatch(s string) []string { + index := skipSpace(s, 0) + + keyStart := index + keyEnd := index + for _, c := range s[keyStart:] { + if !keyValidChar(c) { + break + } + keyEnd++ + } + + if keyStart == keyEnd { + return nil + } + + index = skipSpace(s, keyEnd) + + // this matches only the key + if index == len(s) { + return []string{s, s[keyStart:keyEnd], "", ""} + } + + // now let see if it matches the key and the value + if s[index] != keyValueDelimiter[0] { + return nil + } + + index = skipSpace(s, index+1) + + valueStart := index + valueEnd := index + for _, c := range s[valueStart:] { + if !valueValidChar(c) { + break + } + valueEnd++ + } + + index = skipSpace(s, valueEnd) + if index != len(s) { + return nil + } + + // value can be empty, so no need to do the same check here + return []string{s, "", s[keyStart:keyEnd], s[valueStart:valueEnd]} +} + +func skipSpace(s string, offset int) int { + i := offset + for ; i < len(s); i++ { + c := s[i] + if c != ' ' && + c != '\t' && + c != '\n' && + c != '\v' && + c != '\f' && + c != '\r' { + break + } + } + return i +} func keyMatchString(s string) bool { if len(s) == 0 { @@ -560,17 +615,7 @@ func keyMatchString(s string) bool { } for _, c := range s { - if !(c >= 0x23 && c <= 0x27) && - !(c >= 0x30 && c <= 0x39) && - !(c >= 0x41 && c <= 0x5a) && - !(c >= 0x5e && c <= 0x7a) && - c != 0x21 && - c != 0x2a && - c != 0x2b && - c != 0x2d && - c != 0x2e && - c != 0x7c && - c != 0x7e { + if !keyValidChar(c) { return false } } @@ -578,16 +623,34 @@ func keyMatchString(s string) bool { return true } +func keyValidChar(c int32) bool { + return (c >= 0x23 && c <= 0x27) || + (c >= 0x30 && c <= 0x39) || + (c >= 0x41 && c <= 0x5a) || + (c >= 0x5e && c <= 0x7a) || + c == 0x21 || + c == 0x2a || + c == 0x2b || + c == 0x2d || + c == 0x2e || + c == 0x7c || + c == 0x7e +} + func valueMatchString(s string) bool { for _, c := range s { - if !(c >= 0x23 && c <= 0x2b) && - !(c >= 0x2d && c <= 0x3a) && - !(c >= 0x3c && c <= 0x5b) && - !(c >= 0x5d && c <= 0x7e) && - c != 0x21 { + if !valueValidChar(c) { return false } } return true } + +func valueValidChar(c int32) bool { + return (c >= 0x23 && c <= 0x2b) || + (c >= 0x2d && c <= 0x3a) || + (c >= 0x3c && c <= 0x5b) || + (c >= 0x5d && c <= 0x7e) || + c == 0x21 +} diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 4bac6707ea0..95ed7c8576d 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -22,7 +22,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "go.opentelemetry.io/otel/internal/baggage" ) @@ -45,7 +44,7 @@ func TestKeyRegExp(t *testing.T) { } for _, ch := range invalidKeyRune { - assert.NotRegexp(t, keyDef, fmt.Sprintf("%c", ch)) + assert.False(t, keyValidChar(ch)) } } @@ -60,7 +59,7 @@ func TestValueRegExp(t *testing.T) { } for _, ch := range invalidValueRune { - assert.NotRegexp(t, `^`+valueDef+`$`, fmt.Sprintf("invalid-%c-value", ch)) + assert.False(t, valueValidChar(ch)) } } From bb39a11e5e789fe37477f6d059ab3852da249a22 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Thu, 7 Dec 2023 23:11:15 +0000 Subject: [PATCH 06/22] address comments --- baggage/baggage.go | 74 +++++++++++++++++++---------------------- baggage/baggage_test.go | 5 +-- 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 59c481b6bc0..57e3d931ff2 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -56,7 +56,7 @@ type Property struct { // // If key is invalid, an error will be returned. func NewKeyProperty(key string) (Property, error) { - if !keyMatchString(key) { + if !validateKey(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } @@ -68,10 +68,10 @@ func NewKeyProperty(key string) (Property, error) { // // If key or value are invalid, an error will be returned. func NewKeyValueProperty(key, value string) (Property, error) { - if !keyMatchString(key) { + if !validateKey(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } - if !valueMatchString(value) { + if !validateValue(value) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) } @@ -95,20 +95,11 @@ func parseProperty(property string) (Property, error) { return newInvalidProperty(), nil } - match := propertyFindStringSubmatch(property) - if len(match) != 4 { + p, ok := parsePropertyInternal(property) + if !ok { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidProperty, property) } - var p Property - if match[1] != "" { - p.key = match[1] - } else { - p.key = match[2] - p.value = match[3] - p.hasValue = true - } - return p, nil } @@ -119,10 +110,10 @@ func (p Property) validate() error { return fmt.Errorf("invalid property: %w", err) } - if !keyMatchString(p.key) { + if !validateKey(p.key) { return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key)) } - if p.hasValue && !valueMatchString(p.value) { + if p.hasValue && !validateValue(p.value) { return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value)) } if !p.hasValue && p.value != "" { @@ -294,10 +285,10 @@ func parseMember(member string) (Member, error) { if err != nil { return newInvalidMember(), fmt.Errorf("%w: %q", err, value) } - if !keyMatchString(key) { + if !validateKey(key) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key) } - if !valueMatchString(value) { + if !validateValue(value) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) } @@ -312,10 +303,10 @@ func (m Member) validate() error { return fmt.Errorf("%w: %q", errInvalidMember, m) } - if !keyMatchString(m.key) { + if !validateKey(m.key) { return fmt.Errorf("%w: %q", errInvalidKey, m.key) } - if !valueMatchString(m.value) { + if !validateValue(m.value) { return fmt.Errorf("%w: %q", errInvalidValue, m.value) } return m.properties.validate() @@ -540,45 +531,48 @@ func (b Baggage) String() string { return strings.Join(members, listDelimiter) } -// They must follow the following rules (regex syntax): -// 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]*)` -// propertyRe ^(?:\s*([\x21\x23-\x27\x2A\x2B\x2D\x2E\x30-\x39\x41-\x5a\x5e-\x7a\x7c\x7e]+)\s*|\s*([\x21\x23-\x27\x2A\x2B\x2D\x2E\x30-\x39\x41-\x5a\x5e-\x7a\x7c\x7e]+)\s*=\s*([\x21\x23-\x2b\x2d-\x3a\x3c-\x5B\x5D-\x7e]*)\s*)$. - -func propertyFindStringSubmatch(s string) []string { +// parsePropertyInternal attempts to decode a Property from the passed string which must ensure it follows the spec +// at https://www.w3.org/TR/baggage/ +func parsePropertyInternal(s string) (p Property, ok bool) { index := skipSpace(s, 0) keyStart := index keyEnd := index for _, c := range s[keyStart:] { - if !keyValidChar(c) { + if !validateKeyChar(c) { break } keyEnd++ } if keyStart == keyEnd { - return nil + return } index = skipSpace(s, keyEnd) + p.key = s[keyStart:keyEnd] + // this matches only the key if index == len(s) { - return []string{s, s[keyStart:keyEnd], "", ""} + ok = true + return } // now let see if it matches the key and the value if s[index] != keyValueDelimiter[0] { - return nil + return } + // we set it to true as soon as we find the delimiter even if it is empty + p.hasValue = true + index = skipSpace(s, index+1) valueStart := index valueEnd := index for _, c := range s[valueStart:] { - if !valueValidChar(c) { + if !validateValueChar(c) { break } valueEnd++ @@ -586,11 +580,13 @@ func propertyFindStringSubmatch(s string) []string { index = skipSpace(s, valueEnd) if index != len(s) { - return nil + return } // value can be empty, so no need to do the same check here - return []string{s, "", s[keyStart:keyEnd], s[valueStart:valueEnd]} + ok = true + p.value = s[valueStart:valueEnd] + return } func skipSpace(s string, offset int) int { @@ -609,13 +605,13 @@ func skipSpace(s string, offset int) int { return i } -func keyMatchString(s string) bool { +func validateKey(s string) bool { if len(s) == 0 { return false } for _, c := range s { - if !keyValidChar(c) { + if !validateKeyChar(c) { return false } } @@ -623,7 +619,7 @@ func keyMatchString(s string) bool { return true } -func keyValidChar(c int32) bool { +func validateKeyChar(c int32) bool { return (c >= 0x23 && c <= 0x27) || (c >= 0x30 && c <= 0x39) || (c >= 0x41 && c <= 0x5a) || @@ -637,9 +633,9 @@ func keyValidChar(c int32) bool { c == 0x7e } -func valueMatchString(s string) bool { +func validateValue(s string) bool { for _, c := range s { - if !valueValidChar(c) { + if !validateValueChar(c) { return false } } @@ -647,7 +643,7 @@ func valueMatchString(s string) bool { return true } -func valueValidChar(c int32) bool { +func validateValueChar(c int32) bool { return (c >= 0x23 && c <= 0x2b) || (c >= 0x2d && c <= 0x3a) || (c >= 0x3c && c <= 0x5b) || diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 95ed7c8576d..081f832212e 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" ) @@ -44,7 +45,7 @@ func TestKeyRegExp(t *testing.T) { } for _, ch := range invalidKeyRune { - assert.False(t, keyValidChar(ch)) + assert.False(t, validateKeyChar(ch)) } } @@ -59,7 +60,7 @@ func TestValueRegExp(t *testing.T) { } for _, ch := range invalidValueRune { - assert.False(t, valueValidChar(ch)) + assert.False(t, validateValueChar(ch)) } } From ff7e331bb2f3fc2e23fe9b139889d40cd3b7182d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 8 Dec 2023 09:23:18 +0100 Subject: [PATCH 07/22] Update baggage.go --- baggage/baggage.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 57e3d931ff2..f7866cfd94a 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -531,8 +531,7 @@ func (b Baggage) String() string { return strings.Join(members, listDelimiter) } -// parsePropertyInternal attempts to decode a Property from the passed string which must ensure it follows the spec -// at https://www.w3.org/TR/baggage/ +// parsePropertyInternal attempts to decode a Property from the passed string. func parsePropertyInternal(s string) (p Property, ok bool) { index := skipSpace(s, 0) From fba4b9e54d9f4bb5f936abd1da0dbc34b0844c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 8 Dec 2023 09:38:24 +0100 Subject: [PATCH 08/22] parsePropertyInternal sets p.hasValue only for a valid input --- baggage/baggage.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index f7866cfd94a..d215a57d468 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -533,6 +533,7 @@ func (b Baggage) String() string { // parsePropertyInternal attempts to decode a Property from the passed string. func parsePropertyInternal(s string) (p Property, ok bool) { + // Attempting to parse the key. index := skipSpace(s, 0) keyStart := index @@ -545,6 +546,7 @@ func parsePropertyInternal(s string) (p Property, ok bool) { } if keyStart == keyEnd { + // Invalid key. return } @@ -552,20 +554,19 @@ func parsePropertyInternal(s string) (p Property, ok bool) { p.key = s[keyStart:keyEnd] - // this matches only the key if index == len(s) { + // There is only a key (no value). ok = true return } - // now let see if it matches the key and the value + if s[index] != keyValueDelimiter[0] { + // Bad key-value delimiter. return } - // we set it to true as soon as we find the delimiter even if it is empty - p.hasValue = true - + // Attempting to parse the value. index = skipSpace(s, index+1) valueStart := index @@ -579,11 +580,13 @@ func parsePropertyInternal(s string) (p Property, ok bool) { index = skipSpace(s, valueEnd) if index != len(s) { + // Invalid value. return } - // value can be empty, so no need to do the same check here ok = true + // If there is a delimiter, we set hasValue to true even if the value is empty. + p.hasValue = true p.value = s[valueStart:valueEnd] return } From 6dc6ce187597fb17d6f669e14e90ec451bc9d721 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 8 Dec 2023 09:39:22 +0100 Subject: [PATCH 09/22] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index daa49a760e7..0f34b64daff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Improve `go.opentelemetry.io/otel/trace.TraceState`'s performance. (#4722) - Improve `go.opentelemetry.io/otel/propagation.TraceContext`'s performance. (#4721) -- Improve `go.opentelemetry.io/otel/baggage.NewMember`'s performance. (#4743) +- Improve `go.opentelemetry.io/otel/baggage` performance. (#4743) ## [1.21.0/0.44.0] 2023-11-16 From ad00a1209cb29f3825451aeda7fe3a6a106d7f90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 8 Dec 2023 10:02:59 +0100 Subject: [PATCH 10/22] gofumpt --- baggage/baggage.go | 1 - 1 file changed, 1 deletion(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index d215a57d468..ce025184f3e 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -560,7 +560,6 @@ func parsePropertyInternal(s string) (p Property, ok bool) { return } - if s[index] != keyValueDelimiter[0] { // Bad key-value delimiter. return From a80a2611354f4e7cac68866d978ce0c18d5b176b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 8 Dec 2023 10:11:11 +0100 Subject: [PATCH 11/22] Update baggage.go --- baggage/baggage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index ce025184f3e..e0247a29cbc 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -546,7 +546,7 @@ func parsePropertyInternal(s string) (p Property, ok bool) { } if keyStart == keyEnd { - // Invalid key. + // Empty string after skipping whitespaces. return } From 6b5de85645692081d314d20006fcf1fd5ce8296f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 8 Dec 2023 10:44:41 +0100 Subject: [PATCH 12/22] Apply suggestions from code review --- baggage/baggage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index e0247a29cbc..ed252bb08a5 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -561,7 +561,7 @@ func parsePropertyInternal(s string) (p Property, ok bool) { } if s[index] != keyValueDelimiter[0] { - // Bad key-value delimiter. + // Bad key-value delimiter or invalid key. return } From 00be1c90f8c0ad795a30760514452b1bd941c89d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 8 Dec 2023 11:06:16 +0100 Subject: [PATCH 13/22] Update baggage_test.go --- baggage/baggage_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 081f832212e..197f8ff81be 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -415,10 +415,15 @@ func TestBaggageParse(t *testing.T) { err: errInvalidValue, }, { - name: "invalid property: invalid key", + name: "invalid property: no key", in: "foo=1;=v", err: errInvalidProperty, }, + { + name: "invalid property: invalid key", + in: "foo=1;key\\=v", + err: errInvalidProperty, + }, { name: "invalid property: invalid value", in: "foo=1;key=\\", From 7503e88b8514640818e5ec2838773a5abafb99ba Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Fri, 8 Dec 2023 17:02:36 +0000 Subject: [PATCH 14/22] change unit tests names --- baggage/baggage_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 197f8ff81be..ff8748e576a 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -22,7 +22,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "go.opentelemetry.io/otel/internal/baggage" ) @@ -33,7 +32,7 @@ func init() { rng = rand.New(rand.NewSource(1)) } -func TestKeyRegExp(t *testing.T) { +func TestKeyValidChar(t *testing.T) { // ASCII only invalidKeyRune := []rune{ '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', @@ -49,7 +48,7 @@ func TestKeyRegExp(t *testing.T) { } } -func TestValueRegExp(t *testing.T) { +func TestValueValidChar(t *testing.T) { // ASCII only invalidValueRune := []rune{ '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', From c0da462f033b7f407b67d0e8314aa78701d57eb7 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Fri, 8 Dec 2023 19:11:52 +0000 Subject: [PATCH 15/22] fix tests names --- baggage/baggage_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index ff8748e576a..07461554de1 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -32,7 +32,7 @@ func init() { rng = rand.New(rand.NewSource(1)) } -func TestKeyValidChar(t *testing.T) { +func TestValidateKeyChar(t *testing.T) { // ASCII only invalidKeyRune := []rune{ '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', @@ -48,7 +48,7 @@ func TestKeyValidChar(t *testing.T) { } } -func TestValueValidChar(t *testing.T) { +func TestValidateValueChar(t *testing.T) { // ASCII only invalidValueRune := []rune{ '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', From 97a1986be74dd8c3e79d4d2c4a82606657bb6615 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Fri, 8 Dec 2023 19:16:04 +0000 Subject: [PATCH 16/22] set key value only when match found --- baggage/baggage.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index ed252bb08a5..9f475fefd33 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -552,11 +552,10 @@ func parsePropertyInternal(s string) (p Property, ok bool) { index = skipSpace(s, keyEnd) - p.key = s[keyStart:keyEnd] - if index == len(s) { // There is only a key (no value). ok = true + p.key = s[keyStart:keyEnd] return } @@ -584,6 +583,7 @@ func parsePropertyInternal(s string) (p Property, ok bool) { } ok = true + p.key = s[keyStart:keyEnd] // If there is a delimiter, we set hasValue to true even if the value is empty. p.hasValue = true p.value = s[valueStart:valueEnd] From a8c3d1aa1535ce3ade35ad9f3611fd0c1e963abb Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Fri, 8 Dec 2023 19:42:25 +0000 Subject: [PATCH 17/22] fix lint --- baggage/baggage_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 07461554de1..04395efbca8 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" ) From 8f1d45b3744585eff812f7fa6b0db5cfa9ebcdc9 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Fri, 8 Dec 2023 19:52:34 +0000 Subject: [PATCH 18/22] fix comment --- baggage/baggage.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 9f475fefd33..5d76bd0ec5b 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -531,7 +531,8 @@ func (b Baggage) String() string { return strings.Join(members, listDelimiter) } -// parsePropertyInternal attempts to decode a Property from the passed string. +// parsePropertyInternal attempts to decode a Property from the passed string. It follows the spec at +// https://www.w3.org/TR/baggage/#definition. func parsePropertyInternal(s string) (p Property, ok bool) { // Attempting to parse the key. index := skipSpace(s, 0) From 89a7be329ea7f7968e605fd4162706da95b0ed62 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Mon, 11 Dec 2023 16:59:49 +0000 Subject: [PATCH 19/22] Add more comments --- baggage/baggage.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 5d76bd0ec5b..dbc68e6a372 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -534,9 +534,12 @@ func (b Baggage) String() string { // parsePropertyInternal attempts to decode a Property from the passed string. It follows the spec at // https://www.w3.org/TR/baggage/#definition. func parsePropertyInternal(s string) (p Property, ok bool) { + // For the entire function we will use " key = value " as an example. // Attempting to parse the key. + // First skip spaces at the beginning "< >key = value " (they could be empty). index := skipSpace(s, 0) + // now parse the key: " = value ". keyStart := index keyEnd := index for _, c := range s[keyStart:] { @@ -546,28 +549,32 @@ func parsePropertyInternal(s string) (p Property, ok bool) { keyEnd++ } + // if we couldn't find any valid key character, it means the key is either empty or invalid. if keyStart == keyEnd { - // Empty string after skipping whitespaces. return } + // now skip spaces after the key: " key< >= value ". index = skipSpace(s, keyEnd) + // a key could have no value, like: " key " if index == len(s) { - // There is only a key (no value). ok = true p.key = s[keyStart:keyEnd] return } + // if we have not reached the end and we can't find the '=' delimiter, it means the property is invalid. if s[index] != keyValueDelimiter[0] { - // Bad key-value delimiter or invalid key. return } // Attempting to parse the value. + // now match: " key =< >value ". index = skipSpace(s, index+1) + // now match the value string: " key = ". A valid property can be: " key =". So, we don't have + // to check if the value is empty. valueStart := index valueEnd := index for _, c := range s[valueStart:] { @@ -577,15 +584,17 @@ func parsePropertyInternal(s string) (p Property, ok bool) { valueEnd++ } + // skip all trailing whitespaces: " key = value< >". index = skipSpace(s, valueEnd) + + // If after looking for the value and skipping whitespaces we have not reached the end, it means the property is + // invalid, something like: " key = value value1". if index != len(s) { - // Invalid value. return } ok = true p.key = s[keyStart:keyEnd] - // If there is a delimiter, we set hasValue to true even if the value is empty. p.hasValue = true p.value = s[valueStart:valueEnd] return From 14e7c0fda63057f36e2320ff89328fb0f6c2ebf3 Mon Sep 17 00:00:00 2001 From: Cristian Velazquez Date: Tue, 12 Dec 2023 17:11:06 +0000 Subject: [PATCH 20/22] change skipSpace to space and tab only --- baggage/baggage.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index dbc68e6a372..6b2d9fb59eb 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -604,12 +604,7 @@ func skipSpace(s string, offset int) int { i := offset for ; i < len(s); i++ { c := s[i] - if c != ' ' && - c != '\t' && - c != '\n' && - c != '\v' && - c != '\f' && - c != '\r' { + if c != ' ' && c != '\t' { break } } From c5728bb855898f06f4ec8a85759c35ae0a094111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 14 Dec 2023 16:38:00 +0100 Subject: [PATCH 21/22] Update baggage.go --- baggage/baggage.go | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 6b2d9fb59eb..8e669ce4426 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -531,15 +531,15 @@ func (b Baggage) String() string { return strings.Join(members, listDelimiter) } -// parsePropertyInternal attempts to decode a Property from the passed string. It follows the spec at -// https://www.w3.org/TR/baggage/#definition. +// parsePropertyInternal attempts to decode a Property from the passed string. +// It follows the spec at https://www.w3.org/TR/baggage/#definition. func parsePropertyInternal(s string) (p Property, ok bool) { // For the entire function we will use " key = value " as an example. // Attempting to parse the key. // First skip spaces at the beginning "< >key = value " (they could be empty). index := skipSpace(s, 0) - // now parse the key: " = value ". + // Parse the key: " = value ". keyStart := index keyEnd := index for _, c := range s[keyStart:] { @@ -549,32 +549,35 @@ func parsePropertyInternal(s string) (p Property, ok bool) { keyEnd++ } - // if we couldn't find any valid key character, it means the key is either empty or invalid. + // If we couldn't find any valid key character, + // it means the key is either empty or invalid. if keyStart == keyEnd { return } - // now skip spaces after the key: " key< >= value ". + // Skip spaces after the key: " key< >= value ". index = skipSpace(s, keyEnd) - // a key could have no value, like: " key " if index == len(s) { + // A key can have no value, like: " key ". ok = true p.key = s[keyStart:keyEnd] return } - // if we have not reached the end and we can't find the '=' delimiter, it means the property is invalid. - if s[index] != keyValueDelimiter[0] { + // If we have not reached the end and we can't find the '=' delimiter, + // it means the property is invalid. + if s[index] != keyValueDelimiter[0] { return } // Attempting to parse the value. - // now match: " key =< >value ". + // Match: " key =< >value ". index = skipSpace(s, index+1) - // now match the value string: " key = ". A valid property can be: " key =". So, we don't have - // to check if the value is empty. + // Match the value string: " key = ". + // A valid property can be: " key =". + // Therefore, we don't have to check if the value is empty. valueStart := index valueEnd := index for _, c := range s[valueStart:] { @@ -584,10 +587,11 @@ func parsePropertyInternal(s string) (p Property, ok bool) { valueEnd++ } - // skip all trailing whitespaces: " key = value< >". + // Skip all trailing whitespaces: " key = value< >". index = skipSpace(s, valueEnd) - // If after looking for the value and skipping whitespaces we have not reached the end, it means the property is + // If after looking for the value and skipping whitespaces + // we have not reached the end, it means the property is // invalid, something like: " key = value value1". if index != len(s) { return From 412bf43ee19c2c413bcfb28c4145760b3e909d30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 14 Dec 2023 16:39:56 +0100 Subject: [PATCH 22/22] Update baggage.go --- baggage/baggage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 8e669ce4426..c1f06fab18c 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -567,7 +567,7 @@ func parsePropertyInternal(s string) (p Property, ok bool) { // If we have not reached the end and we can't find the '=' delimiter, // it means the property is invalid. - if s[index] != keyValueDelimiter[0] { + if s[index] != keyValueDelimiter[0] { return }