From 7dab468ad38e086aed1e3a9f3097a342715ccdaf Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 19 May 2021 13:36:35 -0700 Subject: [PATCH 01/12] Refactor TraceState --- trace/trace.go | 163 +---------- trace/trace_test.go | 564 +-------------------------------------- trace/tracestate.go | 229 ++++++++++++++++ trace/tracestate_test.go | 485 +++++++++++++++++++++++++++++++++ 4 files changed, 724 insertions(+), 717 deletions(-) create mode 100644 trace/tracestate.go create mode 100644 trace/tracestate_test.go diff --git a/trace/trace.go b/trace/trace.go index 41a2e27970b..d368760e066 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -19,8 +19,6 @@ import ( "context" "encoding/hex" "encoding/json" - "regexp" - "strings" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" @@ -38,18 +36,6 @@ const ( errInvalidSpanIDLength errorConst = "hex encoded span-id must have length equals to 16" errNilSpanID errorConst = "span-id can't be all zero" - - // based on the W3C Trace Context specification, see https://www.w3.org/TR/trace-context-1/#tracestate-header - traceStateKeyFormat = `[a-z][_0-9a-z\-\*\/]{0,255}` - traceStateKeyFormatWithMultiTenantVendor = `[a-z0-9][_0-9a-z\-\*\/]{0,240}@[a-z][_0-9a-z\-\*\/]{0,13}` - traceStateValueFormat = `[\x20-\x2b\x2d-\x3c\x3e-\x7e]{0,255}[\x21-\x2b\x2d-\x3c\x3e-\x7e]` - - traceStateMaxListMembers = 32 - - errInvalidTraceStateKeyValue errorConst = "provided key or value is not valid according to the" + - " W3C Trace Context specification" - errInvalidTraceStateMembersNumber errorConst = "trace state would exceed the maximum limit of members (32)" - errInvalidTraceStateDuplicate errorConst = "trace state key/value pairs with duplicate keys provided" ) type errorConst string @@ -165,153 +151,6 @@ func decodeHex(h string, b []byte) error { return nil } -// TraceState provides additional vendor-specific trace identification information -// across different distributed tracing systems. It represents an immutable list consisting -// of key/value pairs. There can be a maximum of 32 entries in the list. -// -// Key and value of each list member must be valid according to the W3C Trace Context specification -// (see https://www.w3.org/TR/trace-context-1/#key and https://www.w3.org/TR/trace-context-1/#value -// respectively). -// -// Trace state must be valid according to the W3C Trace Context specification at all times. All -// mutating operations validate their input and, in case of valid parameters, return a new TraceState. -type TraceState struct { //nolint:golint - // TODO @matej-g: Consider implementing this as attribute.Set, see - // comment https://github.com/open-telemetry/opentelemetry-go/pull/1340#discussion_r540599226 - kvs []attribute.KeyValue -} - -var _ json.Marshaler = TraceState{} -var _ json.Marshaler = SpanContext{} - -var keyFormatRegExp = regexp.MustCompile( - `^((` + traceStateKeyFormat + `)|(` + traceStateKeyFormatWithMultiTenantVendor + `))$`, -) -var valueFormatRegExp = regexp.MustCompile(`^(` + traceStateValueFormat + `)$`) - -// MarshalJSON implements a custom marshal function to encode trace state. -func (ts TraceState) MarshalJSON() ([]byte, error) { - return json.Marshal(ts.kvs) -} - -// String returns trace state as a string valid according to the -// W3C Trace Context specification. -func (ts TraceState) String() string { - var sb strings.Builder - - for i, kv := range ts.kvs { - sb.WriteString((string)(kv.Key)) - sb.WriteByte('=') - sb.WriteString(kv.Value.Emit()) - - if i != len(ts.kvs)-1 { - sb.WriteByte(',') - } - } - - return sb.String() -} - -// Get returns a value for given key from the trace state. -// If no key is found or provided key is invalid, returns an empty value. -func (ts TraceState) Get(key attribute.Key) attribute.Value { - if !isTraceStateKeyValid(key) { - return attribute.Value{} - } - - for _, kv := range ts.kvs { - if kv.Key == key { - return kv.Value - } - } - - return attribute.Value{} -} - -// Insert adds a new key/value, if one doesn't exists; otherwise updates the existing entry. -// The new or updated entry is always inserted at the beginning of the TraceState, i.e. -// on the left side, as per the W3C Trace Context specification requirement. -func (ts TraceState) Insert(entry attribute.KeyValue) (TraceState, error) { - if !isTraceStateKeyValueValid(entry) { - return ts, errInvalidTraceStateKeyValue - } - - ckvs := ts.copyKVsAndDeleteEntry(entry.Key) - if len(ckvs)+1 > traceStateMaxListMembers { - return ts, errInvalidTraceStateMembersNumber - } - - ckvs = append(ckvs, attribute.KeyValue{}) - copy(ckvs[1:], ckvs) - ckvs[0] = entry - - return TraceState{ckvs}, nil -} - -// Delete removes specified entry from the trace state. -func (ts TraceState) Delete(key attribute.Key) (TraceState, error) { - if !isTraceStateKeyValid(key) { - return ts, errInvalidTraceStateKeyValue - } - - return TraceState{ts.copyKVsAndDeleteEntry(key)}, nil -} - -// IsEmpty returns true if the TraceState does not contain any entries -func (ts TraceState) IsEmpty() bool { - return len(ts.kvs) == 0 -} - -func (ts TraceState) copyKVsAndDeleteEntry(key attribute.Key) []attribute.KeyValue { - ckvs := make([]attribute.KeyValue, len(ts.kvs)) - copy(ckvs, ts.kvs) - for i, kv := range ts.kvs { - if kv.Key == key { - ckvs = append(ckvs[:i], ckvs[i+1:]...) - break - } - } - - return ckvs -} - -// TraceStateFromKeyValues is a convenience method to create a new TraceState from -// provided key/value pairs. -func TraceStateFromKeyValues(kvs ...attribute.KeyValue) (TraceState, error) { //nolint:golint - if len(kvs) == 0 { - return TraceState{}, nil - } - - if len(kvs) > traceStateMaxListMembers { - return TraceState{}, errInvalidTraceStateMembersNumber - } - - km := make(map[attribute.Key]bool) - for _, kv := range kvs { - if !isTraceStateKeyValueValid(kv) { - return TraceState{}, errInvalidTraceStateKeyValue - } - _, ok := km[kv.Key] - if ok { - return TraceState{}, errInvalidTraceStateDuplicate - } - km[kv.Key] = true - } - - ckvs := make([]attribute.KeyValue, len(kvs)) - copy(ckvs, kvs) - return TraceState{ckvs}, nil -} - -func isTraceStateKeyValid(key attribute.Key) bool { - return keyFormatRegExp.MatchString(string(key)) -} - -func isTraceStateKeyValueValid(kv attribute.KeyValue) bool { - return isTraceStateKeyValid(kv.Key) && - valueFormatRegExp.MatchString(kv.Value.Emit()) -} - // TraceFlags contains flags that can be set on a SpanContext type TraceFlags byte //nolint:golint @@ -371,6 +210,8 @@ type SpanContext struct { remote bool } +var _ json.Marshaler = SpanContext{} + // IsValid returns if the SpanContext is valid. A valid span context has a // valid TraceID and SpanID. func (sc SpanContext) IsValid() bool { diff --git a/trace/trace_test.go b/trace/trace_test.go index dc8afe47e50..dc02334b9e1 100644 --- a/trace/trace_test.go +++ b/trace/trace_test.go @@ -15,14 +15,9 @@ package trace import ( - "fmt" "testing" "github.com/google/go-cmp/cmp" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "go.opentelemetry.io/otel/attribute" ) func TestIsValid(t *testing.T) { @@ -370,540 +365,6 @@ func TestSpanKindString(t *testing.T) { } } -func TestTraceStateString(t *testing.T) { - testCases := []struct { - name string - traceState TraceState - expectedStr string - }{ - { - name: "Non-empty trace state", - traceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3@vendor", "val3"), - }, - }, - expectedStr: "key1=val1,key2=val2,key3@vendor=val3", - }, - { - name: "Empty trace state", - traceState: TraceState{}, - expectedStr: "", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expectedStr, tc.traceState.String()) - }) - } -} - -func TestTraceStateGet(t *testing.T) { - testCases := []struct { - name string - traceState TraceState - key attribute.Key - expectedValue string - }{ - { - name: "OK case", - traceState: TraceState{kvsWithMaxMembers}, - key: "key16", - expectedValue: "value16", - }, - { - name: "Not found", - traceState: TraceState{kvsWithMaxMembers}, - key: "keyxx", - expectedValue: "", - }, - { - name: "Invalid key", - traceState: TraceState{kvsWithMaxMembers}, - key: "key!", - expectedValue: "", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - kv := tc.traceState.Get(tc.key) - assert.Equal(t, tc.expectedValue, kv.AsString()) - }) - } -} - -func TestTraceStateDelete(t *testing.T) { - testCases := []struct { - name string - traceState TraceState - key attribute.Key - expectedTraceState TraceState - expectedErr error - }{ - { - name: "OK case", - traceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - key: "key2", - expectedTraceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key3", "val3"), - }, - }, - }, - { - name: "Non-existing key", - traceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - key: "keyx", - expectedTraceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - }, - { - name: "Invalid key", - traceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - key: "in va lid", - expectedTraceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - expectedErr: errInvalidTraceStateKeyValue, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - result, err := tc.traceState.Delete(tc.key) - if tc.expectedErr != nil { - require.Error(t, err) - assert.Equal(t, tc.expectedErr, err) - assert.Equal(t, tc.traceState, result) - } else { - require.NoError(t, err) - assert.Equal(t, tc.expectedTraceState, result) - } - }) - } -} - -func TestTraceStateInsert(t *testing.T) { - testCases := []struct { - name string - traceState TraceState - keyValue attribute.KeyValue - expectedTraceState TraceState - expectedErr error - }{ - { - name: "OK case - add new", - traceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - keyValue: attribute.String("key4@vendor", "val4"), - expectedTraceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key4@vendor", "val4"), - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - }, - { - name: "OK case - replace", - traceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key2", "val2"), - attribute.String("key3", "val3"), - }, - }, - keyValue: attribute.String("key2", "valX"), - expectedTraceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key2", "valX"), - attribute.String("key1", "val1"), - attribute.String("key3", "val3"), - }, - }, - }, - { - name: "Invalid key/value", - traceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - }, - }, - keyValue: attribute.String("key!", "val!"), - expectedTraceState: TraceState{ - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - }, - }, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Too many entries", - traceState: TraceState{kvsWithMaxMembers}, - keyValue: attribute.String("keyx", "valx"), - expectedTraceState: TraceState{kvsWithMaxMembers}, - expectedErr: errInvalidTraceStateMembersNumber, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - result, err := tc.traceState.Insert(tc.keyValue) - if tc.expectedErr != nil { - require.Error(t, err) - assert.Equal(t, tc.expectedErr, err) - assert.Equal(t, tc.traceState, result) - } else { - require.NoError(t, err) - assert.Equal(t, tc.expectedTraceState, result) - } - }) - } -} - -func TestTraceStateFromKeyValues(t *testing.T) { - testCases := []struct { - name string - kvs []attribute.KeyValue - expectedTraceState TraceState - expectedErr error - }{ - { - name: "OK case", - kvs: kvsWithMaxMembers, - expectedTraceState: TraceState{kvsWithMaxMembers}, - }, - { - name: "OK case (empty)", - expectedTraceState: TraceState{}, - }, - { - name: "Too many entries", - kvs: func() []attribute.KeyValue { - kvs := kvsWithMaxMembers - kvs = append(kvs, attribute.String("keyx", "valX")) - return kvs - }(), - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateMembersNumber, - }, - { - name: "Duplicate key", - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key1", "val2"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateDuplicate, - }, - { - name: "Duplicate key/value", - kvs: []attribute.KeyValue{ - attribute.String("key1", "val1"), - attribute.String("key1", "val1"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateDuplicate, - }, - { - name: "Invalid key/value", - kvs: []attribute.KeyValue{ - attribute.String("key!", "val!"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Full character set", - kvs: []attribute.KeyValue{ - attribute.String( - "abcdefghijklmnopqrstuvwxyz0123456789_-*/", - " !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", - ), - }, - expectedTraceState: TraceState{[]attribute.KeyValue{ - attribute.String( - "abcdefghijklmnopqrstuvwxyz0123456789_-*/", - " !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", - ), - }}, - }, - { - name: "Full character set with vendor", - kvs: []attribute.KeyValue{ - attribute.String( - "abcdefghijklmnopqrstuvwxyz0123456789_-*/@a-z0-9_-*/", - "!\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", - ), - }, - expectedTraceState: TraceState{[]attribute.KeyValue{ - attribute.String( - "abcdefghijklmnopqrstuvwxyz0123456789_-*/@a-z0-9_-*/", - "!\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", - ), - }}, - }, - { - name: "Full character with vendor starting with number", - kvs: []attribute.KeyValue{ - attribute.String( - "0123456789_-*/abcdefghijklmnopqrstuvwxyz@a-z0-9_-*/", - "!\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", - ), - }, - expectedTraceState: TraceState{[]attribute.KeyValue{ - attribute.String( - "0123456789_-*/abcdefghijklmnopqrstuvwxyz@a-z0-9_-*/", - "!\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", - ), - }}, - }, - { - name: "One field", - kvs: []attribute.KeyValue{ - attribute.String("foo", "1"), - }, - expectedTraceState: TraceState{[]attribute.KeyValue{ - attribute.String("foo", "1"), - }}, - }, - { - name: "Two fields", - kvs: []attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String("bar", "2"), - }, - expectedTraceState: TraceState{[]attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String("bar", "2"), - }}, - }, - { - name: "Long field key", - kvs: []attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String( - "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", - "1", - ), - }, - expectedTraceState: TraceState{[]attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String( - "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", - "1", - ), - }}, - }, - { - name: "Long field key with vendor", - kvs: []attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String( - "ttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@vvvvvvvvvvvvvv", - "1", - ), - }, - expectedTraceState: TraceState{[]attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String( - "ttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@vvvvvvvvvvvvvv", - "1", - ), - }}, - }, - { - name: "Invalid whitespace value", - kvs: []attribute.KeyValue{ - attribute.String("foo", "1 \t "), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Invalid whitespace key", - kvs: []attribute.KeyValue{ - attribute.String(" \t bar", "2"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Empty header value", - kvs: []attribute.KeyValue{ - attribute.String("", ""), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Space in key", - kvs: []attribute.KeyValue{ - attribute.String("foo ", "1"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Capitalized key", - kvs: []attribute.KeyValue{ - attribute.String("FOO", "1"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Period in key", - kvs: []attribute.KeyValue{ - attribute.String("foo.bar", "1"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Empty vendor", - kvs: []attribute.KeyValue{ - attribute.String("foo@", "1"), - attribute.String("bar", "2"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Empty key for vendor", - kvs: []attribute.KeyValue{ - attribute.String("@foo", "1"), - attribute.String("bar", "2"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Double @", - kvs: []attribute.KeyValue{ - attribute.String("foo@@bar", "1"), - attribute.String("bar", "2"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Compound vendor", - kvs: []attribute.KeyValue{ - attribute.String("foo@bar@baz", "1"), - attribute.String("bar", "2"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Key too long", - kvs: []attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String("zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", "1"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Key too long with vendor", - kvs: []attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String("tttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@v", "1"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Vendor too long", - kvs: []attribute.KeyValue{ - attribute.String("foo", "1"), - attribute.String("t@vvvvvvvvvvvvvvv", "1"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Equal sign in value", - kvs: []attribute.KeyValue{ - attribute.String("foo", "bar=baz"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - { - name: "Empty value", - kvs: []attribute.KeyValue{ - attribute.String("foo", ""), - attribute.String("bar", "3"), - }, - expectedTraceState: TraceState{}, - expectedErr: errInvalidTraceStateKeyValue, - }, - } - - messageFunc := func(kvs []attribute.KeyValue) []string { - var out []string - for _, kv := range kvs { - out = append(out, fmt.Sprintf("%s=%s", kv.Key, kv.Value.AsString())) - } - return out - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - result, err := TraceStateFromKeyValues(tc.kvs...) - if tc.expectedErr != nil { - require.Error(t, err, messageFunc(tc.kvs)) - assert.Equal(t, TraceState{}, result) - assert.Equal(t, tc.expectedErr, err) - } else { - require.NoError(t, err, messageFunc(tc.kvs)) - assert.NotNil(t, tc.expectedTraceState) - assert.Equal(t, tc.expectedTraceState, result) - } - }) - } -} - func assertSpanContextEqual(got SpanContext, want SpanContext) bool { return got.spanID == want.spanID && got.traceID == want.traceID && @@ -913,12 +374,12 @@ func assertSpanContextEqual(got SpanContext, want SpanContext) bool { } func assertTraceStateEqual(got TraceState, want TraceState) bool { - if len(got.kvs) != len(want.kvs) { + if len(got.members) != len(want.members) { return false } - for i, kv := range got.kvs { - if kv != want.kvs[i] { + for i, kv := range got.members { + if kv != want.members[i] { return false } } @@ -926,15 +387,6 @@ func assertTraceStateEqual(got TraceState, want TraceState) bool { return true } -var kvsWithMaxMembers = func() []attribute.KeyValue { - kvs := make([]attribute.KeyValue, traceStateMaxListMembers) - for i := 0; i < traceStateMaxListMembers; i++ { - kvs[i] = attribute.String(fmt.Sprintf("key%d", i+1), - fmt.Sprintf("value%d", i+1)) - } - return kvs -}() - func TestNewSpanContext(t *testing.T) { testCases := []struct { name string @@ -947,16 +399,16 @@ func TestNewSpanContext(t *testing.T) { TraceID: TraceID([16]byte{1}), SpanID: SpanID([8]byte{42}), TraceFlags: 0x1, - TraceState: TraceState{kvs: []attribute.KeyValue{ - attribute.String("foo", "bar"), + TraceState: TraceState{members: []member{ + {"foo", "bar"}, }}, }, expectedSpanContext: SpanContext{ traceID: TraceID([16]byte{1}), spanID: SpanID([8]byte{42}), traceFlags: 0x1, - traceState: TraceState{kvs: []attribute.KeyValue{ - attribute.String("foo", "bar"), + traceState: TraceState{members: []member{ + {"foo", "bar"}, }}, }, }, @@ -1016,7 +468,7 @@ func TestSpanContextDerivation(t *testing.T) { } from = to - to.traceState = TraceState{kvs: []attribute.KeyValue{attribute.String("foo", "bar")}} + to.traceState = TraceState{members: []member{{"foo", "bar"}}} modified = from.WithTraceState(to.TraceState()) if !assertSpanContextEqual(modified, to) { diff --git a/trace/tracestate.go b/trace/tracestate.go new file mode 100644 index 00000000000..9669911a82a --- /dev/null +++ b/trace/tracestate.go @@ -0,0 +1,229 @@ +// 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 trace + +import ( + "encoding/json" + "fmt" + "regexp" + "strings" +) + +var ( + maxListMembers = 32 + + listDelimiter = "," + + // based on the W3C Trace Context specification, see + // https://www.w3.org/TR/trace-context-1/#tracestate-header + noTenantKeyFormat = `[a-z][_0-9a-z\-\*\/]{0,255}` + withTenantKeyFormat = `[a-z0-9][_0-9a-z\-\*\/]{0,240}@[a-z][_0-9a-z\-\*\/]{0,13}` + valueFormat = `[\x20-\x2b\x2d-\x3c\x3e-\x7e]{0,255}[\x21-\x2b\x2d-\x3c\x3e-\x7e]` + + keyRe = regexp.MustCompile(`^((` + noTenantKeyFormat + `)|(` + withTenantKeyFormat + `))$`) + valueRe = regexp.MustCompile(`^(` + valueFormat + `)$`) + memberRe = regexp.MustCompile(`^\s*((` + noTenantKeyFormat + `)|(` + withTenantKeyFormat + `))=(` + valueFormat + `)\s*$`) + + errInvalidKey errorConst = "invalid tracestate key" + errInvalidValue errorConst = "invalid tracestate value" + errInvalidMember errorConst = "invalid tracestate list member" + errMemberNumber errorConst = "too many list members in tracestate" + errDuplicate errorConst = "duplicate list member in tracestate" +) + +type member struct { + Key string + Value string +} + +func newMember(key, value string) (member, error) { + if !keyRe.MatchString(key) { + return member{}, fmt.Errorf("%w: %s", errInvalidKey, key) + } + if !valueRe.MatchString(value) { + return member{}, fmt.Errorf("%w: %s", errInvalidValue, value) + } + return member{Key: key, Value: value}, nil +} + +func parseMemeber(m string) (member, error) { + matches := memberRe.FindStringSubmatch(m) + if len(matches) != 5 { + return member{}, fmt.Errorf("%w: %s", errInvalidMember, m) + } + + return member{ + Key: matches[1], + Value: matches[4], + }, nil + +} + +// String encodes member into a string compliant with the W3C tracecontext +// specification. +func (m member) String() string { + return fmt.Sprintf("%s=%s", m.Key, m.Value) +} + +// TraceState provides additional vendor-specific trace identification +// information across different distributed tracing systems. It represents an +// immutable list consisting of key/value pairs, each pair is referred to as a +// member. There can be a maximum of 32 list members. +// +// The key and value of each list member must be valid according to the W3C +// Trace Context specification (see https://www.w3.org/TR/trace-context-1/#key +// and https://www.w3.org/TR/trace-context-1/#value respectively). +// +// Trace state must be valid according to the W3C Trace Context specification +// at all times. All mutating operations validate their input and, in case of +// valid parameters, return a new TraceState. +type TraceState struct { //nolint:golint + members []member +} + +var _ json.Marshaler = TraceState{} + +// ParseTraceState attempts to decode a TraceState from the passed byte slice. +// It returns an error if the input is invalid according to the W3C +// tracecontext specification. +func ParseTraceState(tracestate []byte) (TraceState, error) { + return ParseTraceStateString(string(tracestate)) +} + +// ParseTraceStateString attempts to decode a TraceState from the passed +// string. It returns an error if the input is invalid according to the W3C +// tracecontext specification. +func ParseTraceStateString(tracestate string) (TraceState, error) { + if tracestate == "" { + return TraceState{}, nil + } + + wrapErr := func(err error) error { + return fmt.Errorf("failed to parse tracestate: %w", err) + } + + var members []member + found := make(map[string]struct{}) + for _, memberStr := range strings.Split(tracestate, listDelimiter) { + if len(memberStr) == 0 { + continue + } + + m, err := parseMemeber(memberStr) + if err != nil { + return TraceState{}, wrapErr(err) + } + + if _, ok := found[m.Key]; ok { + return TraceState{}, wrapErr(errDuplicate) + } + found[m.Key] = struct{}{} + + members = append(members, m) + if n := len(members); n > maxListMembers { + return TraceState{}, wrapErr(errMemberNumber) + } + } + + return TraceState{members: members}, nil +} + +// MarshalJSON marshals the TraceState into JSON. +func (ts TraceState) MarshalJSON() ([]byte, error) { + return json.Marshal(ts.String()) +} + +// String encodes the TraceState into a string compliant with the W3C +// tracecontext specification. The returned string will be invalid if the +// TraceState contains any invalid members. +func (ts TraceState) String() string { + members := make([]string, len(ts.members)) + for i, m := range ts.members { + members[i] = m.String() + } + return strings.Join(members, listDelimiter) +} + +// Get returns a value for given key from the trace state. +// If no key is found or provided key is invalid, returns an empty string. +func (ts TraceState) Get(key string) string { + if !keyRe.MatchString(key) { + return "" + } + + for _, member := range ts.members { + if member.Key == key { + return member.Value + } + } + + return "" +} + +// Insert adds a new key/value, if one doesn't exists; otherwise updates the existing entry. +// The new or updated entry is always inserted at the beginning of the TraceState, i.e. +// on the left side, as per the W3C Trace Context specification requirement. +func (ts TraceState) Insert(key, value string) (TraceState, error) { + m, err := newMember(key, value) + if err != nil { + return ts, err + } + + cTS := ts.remove(key) + if cTS.len()+1 > maxListMembers { + return ts, fmt.Errorf("failed to insert: %w", errMemberNumber) + } + + cTS.members = append(cTS.members, member{}) + copy(cTS.members[1:], cTS.members) + cTS.members[0] = m + + return cTS, nil +} + +// Delete removes specified entry from the trace state. +func (ts TraceState) Delete(key string) (TraceState, error) { + if !keyRe.MatchString(key) { + return ts, fmt.Errorf("%w: %s", errInvalidKey, key) + } + + return ts.remove(key), nil +} + +// IsEmpty returns true if the TraceState does not contain any entries +func (ts TraceState) IsEmpty() bool { + return ts.len() == 0 +} + +// remove returns a copy of ts with key removed, if it exists. +func (ts TraceState) remove(key string) TraceState { + // allocate the same size in case key is not contained in ts. + members := make([]member, ts.len()) + copy(members, ts.members) + + for i, member := range ts.members { + if member.Key == key { + members = append(members[:i], members[i+1:]...) + break + } + } + + return TraceState{members: members} +} + +// len returns the number of list-members are in the TraceState. +func (ts TraceState) len() int { + return len(ts.members) +} diff --git a/trace/tracestate_test.go b/trace/tracestate_test.go new file mode 100644 index 00000000000..ae64be32835 --- /dev/null +++ b/trace/tracestate_test.go @@ -0,0 +1,485 @@ +// 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 trace + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Taken from the W3C tests: +// https://github.com/w3c/trace-context/blob/dcd3ad9b7d6ac36f70ff3739874b73c11b0302a1/test/test_data.json +var testcases = []struct { + in string + tracestate TraceState + out string + err error +}{ + { + in: "foo=1,foo=1", + err: errDuplicate, + }, + { + in: "foo=1,foo=2", + err: errDuplicate, + }, + { + in: "foo =1", + err: errInvalidMember, + }, + { + in: "FOO=1", + err: errInvalidMember, + }, + { + in: "foo.bar=1", + err: errInvalidMember, + }, + { + in: "foo@=1,bar=2", + err: errInvalidMember, + }, + { + in: "@foo=1,bar=2", + err: errInvalidMember, + }, + { + in: "foo@@bar=1,bar=2", + err: errInvalidMember, + }, + { + in: "foo@bar@baz=1,bar=2", + err: errInvalidMember, + }, + { + in: "foo=1,zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz=1", + err: errInvalidMember, + }, + { + in: "foo=1,tttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@v=1", + err: errInvalidMember, + }, + { + in: "foo=1,t@vvvvvvvvvvvvvvv=1", + err: errInvalidMember, + }, + { + in: "foo=bar=baz", + err: errInvalidMember, + }, + { + in: "foo=,bar=3", + err: errInvalidMember, + }, + { + in: "bar01=01,bar02=02,bar03=03,bar04=04,bar05=05,bar06=06,bar07=07,bar08=08,bar09=09,bar10=10,bar11=11,bar12=12,bar13=13,bar14=14,bar15=15,bar16=16,bar17=17,bar18=18,bar19=19,bar20=20,bar21=21,bar22=22,bar23=23,bar24=24,bar25=25,bar26=26,bar27=27,bar28=28,bar29=29,bar30=30,bar31=31,bar32=32,bar33=33", + err: errMemberNumber, + }, + { + in: "abcdefghijklmnopqrstuvwxyz0123456789_-*/= !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", + out: "abcdefghijklmnopqrstuvwxyz0123456789_-*/= !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", + tracestate: TraceState{members: []member{ + { + Key: "abcdefghijklmnopqrstuvwxyz0123456789_-*/", + Value: " !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", + }, + }}, + }, + { + in: "abcdefghijklmnopqrstuvwxyz0123456789_-*/@a-z0-9_-*/= !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", + out: "abcdefghijklmnopqrstuvwxyz0123456789_-*/@a-z0-9_-*/= !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", + tracestate: TraceState{members: []member{ + { + Key: "abcdefghijklmnopqrstuvwxyz0123456789_-*/@a-z0-9_-*/", + Value: " !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", + }, + }}, + }, + { + // Empty input should result in no error and a zero value + // TraceState being returned, that TraceState should be encoded as an + // empty string. + }, + { + in: "foo=1", + out: "foo=1", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + }}, + }, + { + in: "foo=1,", + out: "foo=1", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + }}, + }, + { + in: "foo=1,bar=2", + out: "foo=1,bar=2", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + {Key: "bar", Value: "2"}, + }}, + }, + { + in: "foo=1,zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz=1", + out: "foo=1,zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz=1", + tracestate: TraceState{members: []member{ + { + Key: "foo", + Value: "1", + }, + { + Key: "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", + Value: "1", + }, + }}, + }, + { + in: "foo=1,ttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@vvvvvvvvvvvvvv=1", + out: "foo=1,ttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@vvvvvvvvvvvvvv=1", + tracestate: TraceState{members: []member{ + { + Key: "foo", + Value: "1", + }, + { + Key: "ttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@vvvvvvvvvvvvvv", + Value: "1", + }, + }}, + }, + { + in: "bar01=01,bar02=02,bar03=03,bar04=04,bar05=05,bar06=06,bar07=07,bar08=08,bar09=09,bar10=10,bar11=11,bar12=12,bar13=13,bar14=14,bar15=15,bar16=16,bar17=17,bar18=18,bar19=19,bar20=20,bar21=21,bar22=22,bar23=23,bar24=24,bar25=25,bar26=26,bar27=27,bar28=28,bar29=29,bar30=30,bar31=31,bar32=32", + out: "bar01=01,bar02=02,bar03=03,bar04=04,bar05=05,bar06=06,bar07=07,bar08=08,bar09=09,bar10=10,bar11=11,bar12=12,bar13=13,bar14=14,bar15=15,bar16=16,bar17=17,bar18=18,bar19=19,bar20=20,bar21=21,bar22=22,bar23=23,bar24=24,bar25=25,bar26=26,bar27=27,bar28=28,bar29=29,bar30=30,bar31=31,bar32=32", + tracestate: TraceState{members: []member{ + {Key: "bar01", Value: "01"}, + {Key: "bar02", Value: "02"}, + {Key: "bar03", Value: "03"}, + {Key: "bar04", Value: "04"}, + {Key: "bar05", Value: "05"}, + {Key: "bar06", Value: "06"}, + {Key: "bar07", Value: "07"}, + {Key: "bar08", Value: "08"}, + {Key: "bar09", Value: "09"}, + {Key: "bar10", Value: "10"}, + {Key: "bar11", Value: "11"}, + {Key: "bar12", Value: "12"}, + {Key: "bar13", Value: "13"}, + {Key: "bar14", Value: "14"}, + {Key: "bar15", Value: "15"}, + {Key: "bar16", Value: "16"}, + {Key: "bar17", Value: "17"}, + {Key: "bar18", Value: "18"}, + {Key: "bar19", Value: "19"}, + {Key: "bar20", Value: "20"}, + {Key: "bar21", Value: "21"}, + {Key: "bar22", Value: "22"}, + {Key: "bar23", Value: "23"}, + {Key: "bar24", Value: "24"}, + {Key: "bar25", Value: "25"}, + {Key: "bar26", Value: "26"}, + {Key: "bar27", Value: "27"}, + {Key: "bar28", Value: "28"}, + {Key: "bar29", Value: "29"}, + {Key: "bar30", Value: "30"}, + {Key: "bar31", Value: "31"}, + {Key: "bar32", Value: "32"}, + }}, + }, + { + in: "foo=1,bar=2,rojo=1,congo=2,baz=3", + out: "foo=1,bar=2,rojo=1,congo=2,baz=3", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + {Key: "bar", Value: "2"}, + {Key: "rojo", Value: "1"}, + {Key: "congo", Value: "2"}, + {Key: "baz", Value: "3"}, + }}, + }, + { + in: "foo=1 \t , \t bar=2, \t baz=3", + out: "foo=1,bar=2,baz=3", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + {Key: "bar", Value: "2"}, + {Key: "baz", Value: "3"}, + }}, + }, + { + in: "foo=1\t \t,\t \tbar=2,\t \tbaz=3", + out: "foo=1,bar=2,baz=3", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + {Key: "bar", Value: "2"}, + {Key: "baz", Value: "3"}, + }}, + }, + { + in: "foo=1 ", + out: "foo=1", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + }}, + }, + { + in: "foo=1\t", + out: "foo=1", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + }}, + }, + { + in: "foo=1 \t", + out: "foo=1", + tracestate: TraceState{members: []member{ + {Key: "foo", Value: "1"}, + }}, + }, +} + +var maxMembers = func() TraceState { + members := make([]member, maxListMembers) + for i := 0; i < maxListMembers; i++ { + members[i] = member{ + Key: fmt.Sprintf("key%d", i+1), + Value: fmt.Sprintf("value%d", i+1), + } + } + return TraceState{members: members} +}() + +func TestParseTraceStateString(t *testing.T) { + for _, tc := range testcases { + got, err := ParseTraceState([]byte(tc.in)) + assert.Equal(t, tc.tracestate, got) + if tc.err != nil { + assert.ErrorIs(t, err, tc.err, tc.in) + } else { + assert.NoError(t, err, tc.in) + } + } +} + +func TestTraceStateString(t *testing.T) { + for _, tc := range testcases { + if tc.err != nil { + // Only test non-zero value TraceState. + continue + } + + assert.Equal(t, tc.out, tc.tracestate.String()) + } +} + +func TestTraceStateMarshalJSON(t *testing.T) { + for _, tc := range testcases { + if tc.err != nil { + // Only test non-zero value TraceState. + continue + } + + // Encode UTF-8. + expected, err := json.Marshal(tc.out) + require.NoError(t, err) + + actual, err := json.Marshal(tc.tracestate) + require.NoError(t, err) + + assert.Equal(t, expected, actual) + } +} + +func TestTraceStateGet(t *testing.T) { + testCases := []struct { + name string + key string + expected string + }{ + { + name: "OK case", + key: "key16", + expected: "value16", + }, + { + name: "not found", + key: "keyxx", + expected: "", + }, + { + name: "invalid W3C key", + key: "key!", + expected: "", + }, + } + + for _, tc := range testCases { + assert.Equal(t, tc.expected, maxMembers.Get(tc.key), tc.name) + } +} + +func TestTraceStateDelete(t *testing.T) { + ts := TraceState{members: []member{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, + {Key: "key3", Value: "val3"}, + }} + + testCases := []struct { + name string + key string + expected TraceState + err error + }{ + { + name: "OK case", + key: "key2", + expected: TraceState{members: []member{ + {Key: "key1", Value: "val1"}, + {Key: "key3", Value: "val3"}, + }}, + }, + { + name: "Non-existing key", + key: "keyx", + expected: TraceState{members: []member{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, + {Key: "key3", Value: "val3"}, + }}, + }, + { + name: "Invalid key", + key: "in va lid", + expected: TraceState{members: []member{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, + {Key: "key3", Value: "val3"}, + }}, + err: errInvalidKey, + }, + } + + for _, tc := range testCases { + actual, err := ts.Delete(tc.key) + assert.ErrorIs(t, err, tc.err, tc.name) + if tc.err != nil { + assert.Equal(t, ts, actual, tc.name) + } else { + assert.Equal(t, tc.expected, actual, tc.name) + } + } +} + +func TestTraceStateInsert(t *testing.T) { + ts := TraceState{members: []member{ + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, + {Key: "key3", Value: "val3"}, + }} + + testCases := []struct { + name string + tracestate TraceState + key, value string + expected TraceState + err error + }{ + { + name: "add new", + tracestate: ts, + key: "key4@vendor", + value: "val4", + expected: TraceState{members: []member{ + {Key: "key4@vendor", Value: "val4"}, + {Key: "key1", Value: "val1"}, + {Key: "key2", Value: "val2"}, + {Key: "key3", Value: "val3"}, + }}, + }, + { + name: "replace", + tracestate: ts, + key: "key2", + value: "valX", + expected: TraceState{members: []member{ + {Key: "key2", Value: "valX"}, + {Key: "key1", Value: "val1"}, + {Key: "key3", Value: "val3"}, + }}, + }, + { + name: "invalid key", + tracestate: ts, + key: "key!", + value: "val", + expected: ts, + err: errInvalidKey, + }, + { + name: "invalid value", + tracestate: ts, + key: "key", + value: "v=l", + expected: ts, + err: errInvalidValue, + }, + { + name: "invalid key/value", + tracestate: ts, + key: "key!", + value: "v=l", + expected: ts, + err: errInvalidKey, + }, + { + name: "too many entries", + tracestate: maxMembers, + key: "keyx", + value: "valx", + expected: maxMembers, + err: errMemberNumber, + }, + } + + for _, tc := range testCases { + actual, err := tc.tracestate.Insert(tc.key, tc.value) + assert.ErrorIs(t, err, tc.err, tc.name) + if tc.err != nil { + assert.Equal(t, tc.tracestate, actual, tc.name) + } else { + assert.Equal(t, tc.expected, actual, tc.name) + } + } +} + +func TestTraceStateIsEmpty(t *testing.T) { + ts := TraceState{} + assert.True(t, ts.IsEmpty(), "zero value TraceState is empty") + + ts, err := ts.Insert("key", "value") + require.NoError(t, err) + assert.False(t, ts.IsEmpty(), "TraceState with inserted value") + + ts, err = ts.Delete("key") + require.NoError(t, err) + assert.True(t, ts.IsEmpty(), "TraceState with all values deleted") +} From 3fe1951e010c3d309186a9985f7e442b9387e9bb Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 19 May 2021 14:01:53 -0700 Subject: [PATCH 02/12] Update tracecontext propagator to use new TraceState --- propagation/trace_context.go | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/propagation/trace_context.go b/propagation/trace_context.go index 82de416bea6..f75d3300002 100644 --- a/propagation/trace_context.go +++ b/propagation/trace_context.go @@ -19,9 +19,7 @@ import ( "encoding/hex" "fmt" "regexp" - "strings" - "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" ) @@ -139,7 +137,10 @@ func (tc TraceContext) extract(carrier TextMapCarrier) trace.SpanContext { // Clear all flags other than the trace-context supported sampling bit. scc.TraceFlags = trace.TraceFlags(opts[0]) & trace.FlagsSampled - scc.TraceState = parseTraceState(carrier.Get(tracestateHeader)) + // Ignore the error returned here. Failure to parse tracestate MUST NOT + // affect the parsing of traceparent according to the W3C tracecontext + // specification. + scc.TraceState, _ = trace.ParseTraceStateString(carrier.Get(tracestateHeader)) scc.Remote = true sc := trace.NewSpanContext(scc) @@ -154,25 +155,3 @@ func (tc TraceContext) extract(carrier TextMapCarrier) trace.SpanContext { func (tc TraceContext) Fields() []string { return []string{traceparentHeader, tracestateHeader} } - -func parseTraceState(in string) trace.TraceState { - if in == "" { - return trace.TraceState{} - } - - kvs := []attribute.KeyValue{} - for _, entry := range strings.Split(in, ",") { - parts := strings.SplitN(entry, "=", 2) - if len(parts) != 2 { - // Parse failure, abort! - return trace.TraceState{} - } - kvs = append(kvs, attribute.String(parts[0], parts[1])) - } - - // Ignoring error here as "failure to parse tracestate MUST NOT - // affect the parsing of traceparent." - // https://www.w3.org/TR/trace-context/#tracestate-header - ts, _ := trace.TraceStateFromKeyValues(kvs...) - return ts -} From f52660632c675028a03ea959d4d79ed436112de0 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 19 May 2021 14:14:16 -0700 Subject: [PATCH 03/12] Add TraceStateFromKeyValues to oteltest --- oteltest/go.mod | 1 + oteltest/tracestate.go | 41 +++++++++++++++++++++++++++++++++++++ oteltest/tracestate_test.go | 40 ++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 oteltest/tracestate.go create mode 100644 oteltest/tracestate_test.go diff --git a/oteltest/go.mod b/oteltest/go.mod index f13318dcda0..8a6478d3469 100644 --- a/oteltest/go.mod +++ b/oteltest/go.mod @@ -47,6 +47,7 @@ replace go.opentelemetry.io/otel/sdk/metric => ../sdk/metric replace go.opentelemetry.io/otel/trace => ../trace require ( + github.com/stretchr/testify v1.7.0 go.opentelemetry.io/otel v0.20.0 go.opentelemetry.io/otel/metric v0.20.0 go.opentelemetry.io/otel/trace v0.20.0 diff --git a/oteltest/tracestate.go b/oteltest/tracestate.go new file mode 100644 index 00000000000..647f7134866 --- /dev/null +++ b/oteltest/tracestate.go @@ -0,0 +1,41 @@ +// 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 oteltest + +import ( + "fmt" + "strings" + + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" +) + +// TraceStateFromKeyValues is a convenience function to create a +// trace.TraceState from provided key/value pairs. There is no inverse to this +// function, returning attributes from a TraceState, because the TraceState, +// by definition from the W3C tracecontext specification, stores values as +// opaque strings. Therefore, it is not possible to decode the original value +// type from TraceState. Be sure to not use this outside of testing purposes. +func TraceStateFromKeyValues(kvs ...attribute.KeyValue) (trace.TraceState, error) { + if len(kvs) == 0 { + return trace.TraceState{}, nil + } + + members := make([]string, len(kvs)) + for i, kv := range kvs { + members[i] = fmt.Sprintf("%s=%s", string(kv.Key), kv.Value.Emit()) + } + return trace.ParseTraceStateString(strings.Join(members, ",")) +} diff --git a/oteltest/tracestate_test.go b/oteltest/tracestate_test.go new file mode 100644 index 00000000000..f1d48d6add3 --- /dev/null +++ b/oteltest/tracestate_test.go @@ -0,0 +1,40 @@ +// 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 oteltest + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" +) + +func TestTraceStateFromKeyValues(t *testing.T) { + ts, err := TraceStateFromKeyValues() + require.NoError(t, err) + assert.True(t, ts.IsEmpty(), "empty attributes creats zero value TraceState") + + ts, err = TraceStateFromKeyValues( + attribute.String("key0", "string"), + attribute.Bool("key1", true), + attribute.Int64("key2", 1), + attribute.Float64("key3", 1.1), + attribute.Array("key4", []int{1, 1}), + ) + require.NoError(t, err) + expected := "key0=string,key1=true,key2=1,key3=1.1,key4=[1 1]" + assert.Equal(t, expected, ts.String()) +} From ecc481a9d4d03b7ee0398306e2c3604b533691dd Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 19 May 2021 14:28:13 -0700 Subject: [PATCH 04/12] Use oteltest to test TraceState --- exporters/otlp/go.mod | 1 + .../otlp/internal/transform/span_test.go | 3 +- exporters/stdout/go.mod | 1 + exporters/stdout/trace_test.go | 15 +++----- oteltest/tracestate_test.go | 1 + propagation/trace_context_test.go | 2 +- sdk/trace/sampling_test.go | 3 +- sdk/trace/trace_test.go | 34 +++++++++---------- 8 files changed, 29 insertions(+), 31 deletions(-) diff --git a/exporters/otlp/go.mod b/exporters/otlp/go.mod index 3ab54c7e96c..98f5deeae56 100644 --- a/exporters/otlp/go.mod +++ b/exporters/otlp/go.mod @@ -13,6 +13,7 @@ require ( github.com/stretchr/testify v1.7.0 go.opentelemetry.io/otel v0.20.0 go.opentelemetry.io/otel/metric v0.20.0 + go.opentelemetry.io/otel/oteltest v0.20.0 go.opentelemetry.io/otel/sdk v0.20.0 go.opentelemetry.io/otel/sdk/export/metric v0.20.0 go.opentelemetry.io/otel/sdk/metric v0.20.0 diff --git a/exporters/otlp/internal/transform/span_test.go b/exporters/otlp/internal/transform/span_test.go index d32fefed14e..9fec4f44780 100644 --- a/exporters/otlp/internal/transform/span_test.go +++ b/exporters/otlp/internal/transform/span_test.go @@ -25,6 +25,7 @@ import ( "google.golang.org/protobuf/proto" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/oteltest" "go.opentelemetry.io/otel/trace" tracepb "go.opentelemetry.io/proto/otlp/trace/v1" @@ -199,7 +200,7 @@ func TestSpanData(t *testing.T) { // March 31, 2020 5:01:26 1234nanos (UTC) startTime := time.Unix(1585674086, 1234) endTime := startTime.Add(10 * time.Second) - traceState, _ := trace.TraceStateFromKeyValues(attribute.String("key1", "val1"), attribute.String("key2", "val2")) + traceState, _ := oteltest.TraceStateFromKeyValues(attribute.String("key1", "val1"), attribute.String("key2", "val2")) spanData := tracetest.SpanStub{ SpanContext: trace.NewSpanContext(trace.SpanContextConfig{ TraceID: trace.TraceID{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F}, diff --git a/exporters/stdout/go.mod b/exporters/stdout/go.mod index 95f5b5d991a..912439c7591 100644 --- a/exporters/stdout/go.mod +++ b/exporters/stdout/go.mod @@ -11,6 +11,7 @@ require ( github.com/stretchr/testify v1.7.0 go.opentelemetry.io/otel v0.20.0 go.opentelemetry.io/otel/metric v0.20.0 + go.opentelemetry.io/otel/oteltest v0.20.0 go.opentelemetry.io/otel/sdk v0.20.0 go.opentelemetry.io/otel/sdk/export/metric v0.20.0 go.opentelemetry.io/otel/sdk/metric v0.20.0 diff --git a/exporters/stdout/trace_test.go b/exporters/stdout/trace_test.go index 8b60790dfcb..f9cccef9b7c 100644 --- a/exporters/stdout/trace_test.go +++ b/exporters/stdout/trace_test.go @@ -27,6 +27,7 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/exporters/stdout" + "go.opentelemetry.io/otel/oteltest" "go.opentelemetry.io/otel/sdk/resource" tracesdk "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" @@ -45,7 +46,7 @@ func TestExporter_ExportSpan(t *testing.T) { now := time.Now() traceID, _ := trace.TraceIDFromHex("0102030405060708090a0b0c0d0e0f10") spanID, _ := trace.SpanIDFromHex("0102030405060708") - traceState, _ := trace.TraceStateFromKeyValues(attribute.String("key", "val")) + traceState, _ := oteltest.TraceStateFromKeyValues(attribute.String("key", "val")) keyValue := "value" doubleValue := 123.456 resource := resource.NewWithAttributes(attribute.String("rk1", "rv11")) @@ -90,22 +91,14 @@ func TestExporter_ExportSpan(t *testing.T) { "TraceID": "0102030405060708090a0b0c0d0e0f10", "SpanID": "0102030405060708", "TraceFlags": "00", - "TraceState": [ - { - "Key": "key", - "Value": { - "Type": "STRING", - "Value": "val" - } - } - ], + "TraceState": "key=val", "Remote": false }, "Parent": { "TraceID": "00000000000000000000000000000000", "SpanID": "0000000000000000", "TraceFlags": "00", - "TraceState": null, + "TraceState": "", "Remote": false }, "SpanKind": 1, diff --git a/oteltest/tracestate_test.go b/oteltest/tracestate_test.go index f1d48d6add3..0defe6f527d 100644 --- a/oteltest/tracestate_test.go +++ b/oteltest/tracestate_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" ) diff --git a/propagation/trace_context_test.go b/propagation/trace_context_test.go index fb712d82aec..3f92c5f1961 100644 --- a/propagation/trace_context_test.go +++ b/propagation/trace_context_test.go @@ -288,7 +288,7 @@ func TestTraceStatePropagation(t *testing.T) { prop := propagation.TraceContext{} stateHeader := "tracestate" parentHeader := "traceparent" - state, err := trace.TraceStateFromKeyValues(attribute.String("key1", "value1"), attribute.String("key2", "value2")) + state, err := oteltest.TraceStateFromKeyValues(attribute.String("key1", "value1"), attribute.String("key2", "value2")) if err != nil { t.Fatalf("Unable to construct expected TraceState: %s", err.Error()) } diff --git a/sdk/trace/sampling_test.go b/sdk/trace/sampling_test.go index 98bb4f42150..b2ff015d545 100644 --- a/sdk/trace/sampling_test.go +++ b/sdk/trace/sampling_test.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/oteltest" "go.opentelemetry.io/otel/trace" ) @@ -240,7 +241,7 @@ func TestTracestateIsPassed(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - traceState, err := trace.TraceStateFromKeyValues(attribute.String("k", "v")) + traceState, err := oteltest.TraceStateFromKeyValues(attribute.String("k", "v")) if err != nil { t.Error(err) } diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index a340ddb1bae..03d6ae75380 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -354,7 +354,7 @@ func TestStartSpanWithParent(t *testing.T) { t.Error(err) } - ts, err := trace.TraceStateFromKeyValues(attribute.String("k", "v")) + ts, err := oteltest.TraceStateFromKeyValues(attribute.String("k", "v")) if err != nil { t.Error(err) } @@ -1597,13 +1597,13 @@ func TestSamplerTraceState(t *testing.T) { makeInserter := func(k attribute.KeyValue, prefix string) Sampler { return &stateSampler{ prefix: prefix, - f: func(t trace.TraceState) trace.TraceState { return mustTS(t.Insert(k)) }, + f: func(t trace.TraceState) trace.TraceState { return mustTS(t.Insert(string(k.Key), k.Value.Emit())) }, } } makeDeleter := func(k attribute.Key, prefix string) Sampler { return &stateSampler{ prefix: prefix, - f: func(t trace.TraceState) trace.TraceState { return mustTS(t.Delete(k)) }, + f: func(t trace.TraceState) trace.TraceState { return mustTS(t.Delete(string(k))) }, } } clearer := func(prefix string) Sampler { @@ -1624,55 +1624,55 @@ func TestSamplerTraceState(t *testing.T) { { name: "alwaysOn", sampler: AlwaysSample(), - input: mustTS(trace.TraceStateFromKeyValues(kv1)), - want: mustTS(trace.TraceStateFromKeyValues(kv1)), + input: mustTS(oteltest.TraceStateFromKeyValues(kv1)), + want: mustTS(oteltest.TraceStateFromKeyValues(kv1)), exportSpan: true, }, { name: "alwaysOff", sampler: NeverSample(), - input: mustTS(trace.TraceStateFromKeyValues(kv1)), - want: mustTS(trace.TraceStateFromKeyValues(kv1)), + input: mustTS(oteltest.TraceStateFromKeyValues(kv1)), + want: mustTS(oteltest.TraceStateFromKeyValues(kv1)), exportSpan: false, }, { name: "insertKeySampled", sampler: makeInserter(kv2, "span"), spanName: "span0", - input: mustTS(trace.TraceStateFromKeyValues(kv1)), - want: mustTS(trace.TraceStateFromKeyValues(kv2, kv1)), + input: mustTS(oteltest.TraceStateFromKeyValues(kv1)), + want: mustTS(oteltest.TraceStateFromKeyValues(kv2, kv1)), exportSpan: true, }, { name: "insertKeyDropped", sampler: makeInserter(kv2, "span"), spanName: "nospan0", - input: mustTS(trace.TraceStateFromKeyValues(kv1)), - want: mustTS(trace.TraceStateFromKeyValues(kv2, kv1)), + input: mustTS(oteltest.TraceStateFromKeyValues(kv1)), + want: mustTS(oteltest.TraceStateFromKeyValues(kv2, kv1)), exportSpan: false, }, { name: "deleteKeySampled", sampler: makeDeleter(k1, "span"), spanName: "span0", - input: mustTS(trace.TraceStateFromKeyValues(kv1, kv2)), - want: mustTS(trace.TraceStateFromKeyValues(kv2)), + input: mustTS(oteltest.TraceStateFromKeyValues(kv1, kv2)), + want: mustTS(oteltest.TraceStateFromKeyValues(kv2)), exportSpan: true, }, { name: "deleteKeyDropped", sampler: makeDeleter(k1, "span"), spanName: "nospan0", - input: mustTS(trace.TraceStateFromKeyValues(kv1, kv2, kv3)), - want: mustTS(trace.TraceStateFromKeyValues(kv2, kv3)), + input: mustTS(oteltest.TraceStateFromKeyValues(kv1, kv2, kv3)), + want: mustTS(oteltest.TraceStateFromKeyValues(kv2, kv3)), exportSpan: false, }, { name: "clearer", sampler: clearer("span"), spanName: "span0", - input: mustTS(trace.TraceStateFromKeyValues(kv1, kv3)), - want: mustTS(trace.TraceStateFromKeyValues()), + input: mustTS(oteltest.TraceStateFromKeyValues(kv1, kv3)), + want: mustTS(oteltest.TraceStateFromKeyValues()), exportSpan: true, }, } From 65cc4c9ed3bd61c51c1bfd0974c0696491ad1345 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 20 May 2021 09:17:35 -0700 Subject: [PATCH 05/12] Replace IsEmpty with Len for TraceState --- oteltest/tracestate_test.go | 2 +- trace/tracestate.go | 17 ++++++----------- trace/tracestate_test.go | 6 +++--- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/oteltest/tracestate_test.go b/oteltest/tracestate_test.go index 0defe6f527d..df3c926f574 100644 --- a/oteltest/tracestate_test.go +++ b/oteltest/tracestate_test.go @@ -26,7 +26,7 @@ import ( func TestTraceStateFromKeyValues(t *testing.T) { ts, err := TraceStateFromKeyValues() require.NoError(t, err) - assert.True(t, ts.IsEmpty(), "empty attributes creats zero value TraceState") + assert.Equal(t, 0, ts.Len(), "empty attributes creats zero value TraceState") ts, err = TraceStateFromKeyValues( attribute.String("key0", "string"), diff --git a/trace/tracestate.go b/trace/tracestate.go index 9669911a82a..031df112cd1 100644 --- a/trace/tracestate.go +++ b/trace/tracestate.go @@ -172,7 +172,7 @@ func (ts TraceState) Get(key string) string { return "" } -// Insert adds a new key/value, if one doesn't exists; otherwise updates the existing entry. +// Insert adds a new key/value pair, if one doesn't exists; otherwise updates the existing entry. // The new or updated entry is always inserted at the beginning of the TraceState, i.e. // on the left side, as per the W3C Trace Context specification requirement. func (ts TraceState) Insert(key, value string) (TraceState, error) { @@ -182,7 +182,7 @@ func (ts TraceState) Insert(key, value string) (TraceState, error) { } cTS := ts.remove(key) - if cTS.len()+1 > maxListMembers { + if cTS.Len()+1 > maxListMembers { return ts, fmt.Errorf("failed to insert: %w", errMemberNumber) } @@ -202,15 +202,15 @@ func (ts TraceState) Delete(key string) (TraceState, error) { return ts.remove(key), nil } -// IsEmpty returns true if the TraceState does not contain any entries -func (ts TraceState) IsEmpty() bool { - return ts.len() == 0 +// Len returns the number of list-members in the TraceState. +func (ts TraceState) Len() int { + return len(ts.members) } // remove returns a copy of ts with key removed, if it exists. func (ts TraceState) remove(key string) TraceState { // allocate the same size in case key is not contained in ts. - members := make([]member, ts.len()) + members := make([]member, ts.Len()) copy(members, ts.members) for i, member := range ts.members { @@ -222,8 +222,3 @@ func (ts TraceState) remove(key string) TraceState { return TraceState{members: members} } - -// len returns the number of list-members are in the TraceState. -func (ts TraceState) len() int { - return len(ts.members) -} diff --git a/trace/tracestate_test.go b/trace/tracestate_test.go index ae64be32835..8165583fecb 100644 --- a/trace/tracestate_test.go +++ b/trace/tracestate_test.go @@ -473,13 +473,13 @@ func TestTraceStateInsert(t *testing.T) { func TestTraceStateIsEmpty(t *testing.T) { ts := TraceState{} - assert.True(t, ts.IsEmpty(), "zero value TraceState is empty") + assert.Equal(t, 0, ts.Len(), "zero value TraceState is empty") ts, err := ts.Insert("key", "value") require.NoError(t, err) - assert.False(t, ts.IsEmpty(), "TraceState with inserted value") + assert.Equal(t, 1, ts.Len(), "TraceState with inserted value") ts, err = ts.Delete("key") require.NoError(t, err) - assert.True(t, ts.IsEmpty(), "TraceState with all values deleted") + assert.Equal(t, 0, ts.Len(), "TraceState with all values deleted") } From 18e10749cca472de7a1530827d33602c12a8bfb5 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 20 May 2021 11:12:12 -0700 Subject: [PATCH 06/12] Replace ParseTraceState with ParseTraceStateString --- oteltest/tracestate.go | 2 +- propagation/trace_context.go | 2 +- trace/tracestate.go | 11 ++--------- trace/tracestate_test.go | 4 ++-- 4 files changed, 6 insertions(+), 13 deletions(-) diff --git a/oteltest/tracestate.go b/oteltest/tracestate.go index 647f7134866..b12743fd35f 100644 --- a/oteltest/tracestate.go +++ b/oteltest/tracestate.go @@ -37,5 +37,5 @@ func TraceStateFromKeyValues(kvs ...attribute.KeyValue) (trace.TraceState, error for i, kv := range kvs { members[i] = fmt.Sprintf("%s=%s", string(kv.Key), kv.Value.Emit()) } - return trace.ParseTraceStateString(strings.Join(members, ",")) + return trace.ParseTraceState(strings.Join(members, ",")) } diff --git a/propagation/trace_context.go b/propagation/trace_context.go index f75d3300002..445f81f73d4 100644 --- a/propagation/trace_context.go +++ b/propagation/trace_context.go @@ -140,7 +140,7 @@ func (tc TraceContext) extract(carrier TextMapCarrier) trace.SpanContext { // Ignore the error returned here. Failure to parse tracestate MUST NOT // affect the parsing of traceparent according to the W3C tracecontext // specification. - scc.TraceState, _ = trace.ParseTraceStateString(carrier.Get(tracestateHeader)) + scc.TraceState, _ = trace.ParseTraceState(carrier.Get(tracestateHeader)) scc.Remote = true sc := trace.NewSpanContext(scc) diff --git a/trace/tracestate.go b/trace/tracestate.go index 031df112cd1..baf836a8edd 100644 --- a/trace/tracestate.go +++ b/trace/tracestate.go @@ -95,17 +95,10 @@ type TraceState struct { //nolint:golint var _ json.Marshaler = TraceState{} -// ParseTraceState attempts to decode a TraceState from the passed byte slice. -// It returns an error if the input is invalid according to the W3C -// tracecontext specification. -func ParseTraceState(tracestate []byte) (TraceState, error) { - return ParseTraceStateString(string(tracestate)) -} - -// ParseTraceStateString attempts to decode a TraceState from the passed +// ParseTraceState attempts to decode a TraceState from the passed // string. It returns an error if the input is invalid according to the W3C // tracecontext specification. -func ParseTraceStateString(tracestate string) (TraceState, error) { +func ParseTraceState(tracestate string) (TraceState, error) { if tracestate == "" { return TraceState{}, nil } diff --git a/trace/tracestate_test.go b/trace/tracestate_test.go index 8165583fecb..3054ff26dfa 100644 --- a/trace/tracestate_test.go +++ b/trace/tracestate_test.go @@ -267,9 +267,9 @@ var maxMembers = func() TraceState { return TraceState{members: members} }() -func TestParseTraceStateString(t *testing.T) { +func TestParseTraceState(t *testing.T) { for _, tc := range testcases { - got, err := ParseTraceState([]byte(tc.in)) + got, err := ParseTraceState(tc.in) assert.Equal(t, tc.tracestate, got) if tc.err != nil { assert.ErrorIs(t, err, tc.err, tc.in) From 43b7fab48f7cbd6c0f17ea4f876d28c1fa399f3d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 20 May 2021 12:47:25 -0700 Subject: [PATCH 07/12] Clean up naming --- trace/trace_test.go | 22 ++------ trace/tracestate.go | 106 +++++++++++++++++++-------------------- trace/tracestate_test.go | 62 ++++++++++------------- 3 files changed, 84 insertions(+), 106 deletions(-) diff --git a/trace/trace_test.go b/trace/trace_test.go index dc02334b9e1..9e6b6aae8c3 100644 --- a/trace/trace_test.go +++ b/trace/trace_test.go @@ -370,21 +370,7 @@ func assertSpanContextEqual(got SpanContext, want SpanContext) bool { got.traceID == want.traceID && got.traceFlags == want.traceFlags && got.remote == want.remote && - assertTraceStateEqual(got.traceState, want.traceState) -} - -func assertTraceStateEqual(got TraceState, want TraceState) bool { - if len(got.members) != len(want.members) { - return false - } - - for i, kv := range got.members { - if kv != want.members[i] { - return false - } - } - - return true + got.traceState.String() == want.traceState.String() } func TestNewSpanContext(t *testing.T) { @@ -399,7 +385,7 @@ func TestNewSpanContext(t *testing.T) { TraceID: TraceID([16]byte{1}), SpanID: SpanID([8]byte{42}), TraceFlags: 0x1, - TraceState: TraceState{members: []member{ + TraceState: TraceState{list: []member{ {"foo", "bar"}, }}, }, @@ -407,7 +393,7 @@ func TestNewSpanContext(t *testing.T) { traceID: TraceID([16]byte{1}), spanID: SpanID([8]byte{42}), traceFlags: 0x1, - traceState: TraceState{members: []member{ + traceState: TraceState{list: []member{ {"foo", "bar"}, }}, }, @@ -468,7 +454,7 @@ func TestSpanContextDerivation(t *testing.T) { } from = to - to.traceState = TraceState{members: []member{{"foo", "bar"}}} + to.traceState = TraceState{list: []member{{"foo", "bar"}}} modified = from.WithTraceState(to.TraceState()) if !assertSpanContextEqual(modified, to) { diff --git a/trace/tracestate.go b/trace/tracestate.go index baf836a8edd..d743264e1a7 100644 --- a/trace/tracestate.go +++ b/trace/tracestate.go @@ -38,9 +38,9 @@ var ( errInvalidKey errorConst = "invalid tracestate key" errInvalidValue errorConst = "invalid tracestate value" - errInvalidMember errorConst = "invalid tracestate list member" - errMemberNumber errorConst = "too many list members in tracestate" - errDuplicate errorConst = "duplicate list member in tracestate" + errInvalidMember errorConst = "invalid tracestate list-member" + errMemberNumber errorConst = "too many list-members in tracestate" + errDuplicate errorConst = "duplicate list-member in tracestate" ) type member struct { @@ -71,7 +71,7 @@ func parseMemeber(m string) (member, error) { } -// String encodes member into a string compliant with the W3C tracecontext +// String encodes member into a string compliant with the W3C Trace Context // specification. func (m member) String() string { return fmt.Sprintf("%s=%s", m.Key, m.Value) @@ -80,24 +80,24 @@ func (m member) String() string { // TraceState provides additional vendor-specific trace identification // information across different distributed tracing systems. It represents an // immutable list consisting of key/value pairs, each pair is referred to as a -// member. There can be a maximum of 32 list members. +// list-member. // -// The key and value of each list member must be valid according to the W3C -// Trace Context specification (see https://www.w3.org/TR/trace-context-1/#key -// and https://www.w3.org/TR/trace-context-1/#value respectively). -// -// Trace state must be valid according to the W3C Trace Context specification -// at all times. All mutating operations validate their input and, in case of -// valid parameters, return a new TraceState. +// TraceState conforms to the W3C Trace Context specification +// (https://www.w3.org/TR/trace-context-1). All operations that create or copy +// a TraceState do so by validating all input and will only produce TraceState +// that conform to the specification. Specifically, this means that all +// list-member's key/value pairs are valid, no duplicate list-members exist, +// and the maximum number of list-members (32) is not exceeded. type TraceState struct { //nolint:golint - members []member + // list is the members in order. + list []member } var _ json.Marshaler = TraceState{} // ParseTraceState attempts to decode a TraceState from the passed // string. It returns an error if the input is invalid according to the W3C -// tracecontext specification. +// Trace Context specification. func ParseTraceState(tracestate string) (TraceState, error) { if tracestate == "" { return TraceState{}, nil @@ -130,7 +130,7 @@ func ParseTraceState(tracestate string) (TraceState, error) { } } - return TraceState{members: members}, nil + return TraceState{list: members}, nil } // MarshalJSON marshals the TraceState into JSON. @@ -139,24 +139,20 @@ func (ts TraceState) MarshalJSON() ([]byte, error) { } // String encodes the TraceState into a string compliant with the W3C -// tracecontext specification. The returned string will be invalid if the +// Trace Context specification. The returned string will be invalid if the // TraceState contains any invalid members. func (ts TraceState) String() string { - members := make([]string, len(ts.members)) - for i, m := range ts.members { + members := make([]string, len(ts.list)) + for i, m := range ts.list { members[i] = m.String() } return strings.Join(members, listDelimiter) } -// Get returns a value for given key from the trace state. -// If no key is found or provided key is invalid, returns an empty string. +// Get returns the value paired with key from the corresponding TraceState +// list-member if it exists, otherwise an empty string is returned. func (ts TraceState) Get(key string) string { - if !keyRe.MatchString(key) { - return "" - } - - for _, member := range ts.members { + for _, member := range ts.list { if member.Key == key { return member.Value } @@ -165,53 +161,57 @@ func (ts TraceState) Get(key string) string { return "" } -// Insert adds a new key/value pair, if one doesn't exists; otherwise updates the existing entry. -// The new or updated entry is always inserted at the beginning of the TraceState, i.e. -// on the left side, as per the W3C Trace Context specification requirement. +// Insert adds a new list-member defined by the key/value pair to the +// TraceState. If a list-member already exists for the given key, that +// list-member's value is updated. The new or updated list-member is always +// moved to the beginning of the TraceState as specified by the W3C Trace +// Context specification. +// +// If key or value are invalid according to the W3C Trace Context +// specification an error is returned with the original TraceState. +// +// If adding a new list-member means the TraceState would have more members +// than is allowed an error is returned instead with the original TraceState. func (ts TraceState) Insert(key, value string) (TraceState, error) { m, err := newMember(key, value) if err != nil { return ts, err } - cTS := ts.remove(key) + cTS := ts.Delete(key) if cTS.Len()+1 > maxListMembers { + // TODO (MrAlias): When the second version of the Trace Context + // specification is published this needs to not return an error. + // Instead it should drop the "right-most" member and insert the new + // member at the front. + // + // https://github.com/w3c/trace-context/pull/448 return ts, fmt.Errorf("failed to insert: %w", errMemberNumber) } - cTS.members = append(cTS.members, member{}) - copy(cTS.members[1:], cTS.members) - cTS.members[0] = m + cTS.list = append(cTS.list, member{}) + copy(cTS.list[1:], cTS.list) + cTS.list[0] = m return cTS, nil } -// Delete removes specified entry from the trace state. -func (ts TraceState) Delete(key string) (TraceState, error) { - if !keyRe.MatchString(key) { - return ts, fmt.Errorf("%w: %s", errInvalidKey, key) - } - - return ts.remove(key), nil -} - -// Len returns the number of list-members in the TraceState. -func (ts TraceState) Len() int { - return len(ts.members) -} - -// remove returns a copy of ts with key removed, if it exists. -func (ts TraceState) remove(key string) TraceState { - // allocate the same size in case key is not contained in ts. +// Delete returns a copy of the TraceState with the list-member identified by +// key removed. +func (ts TraceState) Delete(key string) TraceState { members := make([]member, ts.Len()) - copy(members, ts.members) - - for i, member := range ts.members { + copy(members, ts.list) + for i, member := range ts.list { if member.Key == key { members = append(members[:i], members[i+1:]...) + // TraceState should contain no duplicate members. break } } + return TraceState{list: members} +} - return TraceState{members: members} +// Len returns the number of list-members in the TraceState. +func (ts TraceState) Len() int { + return len(ts.list) } diff --git a/trace/tracestate_test.go b/trace/tracestate_test.go index 3054ff26dfa..b503bf25d12 100644 --- a/trace/tracestate_test.go +++ b/trace/tracestate_test.go @@ -94,7 +94,7 @@ var testcases = []struct { { in: "abcdefghijklmnopqrstuvwxyz0123456789_-*/= !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", out: "abcdefghijklmnopqrstuvwxyz0123456789_-*/= !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ { Key: "abcdefghijklmnopqrstuvwxyz0123456789_-*/", Value: " !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", @@ -104,7 +104,7 @@ var testcases = []struct { { in: "abcdefghijklmnopqrstuvwxyz0123456789_-*/@a-z0-9_-*/= !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", out: "abcdefghijklmnopqrstuvwxyz0123456789_-*/@a-z0-9_-*/= !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ { Key: "abcdefghijklmnopqrstuvwxyz0123456789_-*/@a-z0-9_-*/", Value: " !\"#$%&'()*+-./0123456789:;<>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", @@ -119,21 +119,21 @@ var testcases = []struct { { in: "foo=1", out: "foo=1", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, }}, }, { in: "foo=1,", out: "foo=1", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, }}, }, { in: "foo=1,bar=2", out: "foo=1,bar=2", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, {Key: "bar", Value: "2"}, }}, @@ -141,7 +141,7 @@ var testcases = []struct { { in: "foo=1,zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz=1", out: "foo=1,zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz=1", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ { Key: "foo", Value: "1", @@ -155,7 +155,7 @@ var testcases = []struct { { in: "foo=1,ttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@vvvvvvvvvvvvvv=1", out: "foo=1,ttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@vvvvvvvvvvvvvv=1", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ { Key: "foo", Value: "1", @@ -169,7 +169,7 @@ var testcases = []struct { { in: "bar01=01,bar02=02,bar03=03,bar04=04,bar05=05,bar06=06,bar07=07,bar08=08,bar09=09,bar10=10,bar11=11,bar12=12,bar13=13,bar14=14,bar15=15,bar16=16,bar17=17,bar18=18,bar19=19,bar20=20,bar21=21,bar22=22,bar23=23,bar24=24,bar25=25,bar26=26,bar27=27,bar28=28,bar29=29,bar30=30,bar31=31,bar32=32", out: "bar01=01,bar02=02,bar03=03,bar04=04,bar05=05,bar06=06,bar07=07,bar08=08,bar09=09,bar10=10,bar11=11,bar12=12,bar13=13,bar14=14,bar15=15,bar16=16,bar17=17,bar18=18,bar19=19,bar20=20,bar21=21,bar22=22,bar23=23,bar24=24,bar25=25,bar26=26,bar27=27,bar28=28,bar29=29,bar30=30,bar31=31,bar32=32", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "bar01", Value: "01"}, {Key: "bar02", Value: "02"}, {Key: "bar03", Value: "03"}, @@ -207,7 +207,7 @@ var testcases = []struct { { in: "foo=1,bar=2,rojo=1,congo=2,baz=3", out: "foo=1,bar=2,rojo=1,congo=2,baz=3", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, {Key: "bar", Value: "2"}, {Key: "rojo", Value: "1"}, @@ -218,7 +218,7 @@ var testcases = []struct { { in: "foo=1 \t , \t bar=2, \t baz=3", out: "foo=1,bar=2,baz=3", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, {Key: "bar", Value: "2"}, {Key: "baz", Value: "3"}, @@ -227,7 +227,7 @@ var testcases = []struct { { in: "foo=1\t \t,\t \tbar=2,\t \tbaz=3", out: "foo=1,bar=2,baz=3", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, {Key: "bar", Value: "2"}, {Key: "baz", Value: "3"}, @@ -236,21 +236,21 @@ var testcases = []struct { { in: "foo=1 ", out: "foo=1", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, }}, }, { in: "foo=1\t", out: "foo=1", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, }}, }, { in: "foo=1 \t", out: "foo=1", - tracestate: TraceState{members: []member{ + tracestate: TraceState{list: []member{ {Key: "foo", Value: "1"}, }}, }, @@ -264,7 +264,7 @@ var maxMembers = func() TraceState { Value: fmt.Sprintf("value%d", i+1), } } - return TraceState{members: members} + return TraceState{list: members} }() func TestParseTraceState(t *testing.T) { @@ -337,7 +337,7 @@ func TestTraceStateGet(t *testing.T) { } func TestTraceStateDelete(t *testing.T) { - ts := TraceState{members: []member{ + ts := TraceState{list: []member{ {Key: "key1", Value: "val1"}, {Key: "key2", Value: "val2"}, {Key: "key3", Value: "val3"}, @@ -347,12 +347,11 @@ func TestTraceStateDelete(t *testing.T) { name string key string expected TraceState - err error }{ { name: "OK case", key: "key2", - expected: TraceState{members: []member{ + expected: TraceState{list: []member{ {Key: "key1", Value: "val1"}, {Key: "key3", Value: "val3"}, }}, @@ -360,7 +359,7 @@ func TestTraceStateDelete(t *testing.T) { { name: "Non-existing key", key: "keyx", - expected: TraceState{members: []member{ + expected: TraceState{list: []member{ {Key: "key1", Value: "val1"}, {Key: "key2", Value: "val2"}, {Key: "key3", Value: "val3"}, @@ -369,28 +368,21 @@ func TestTraceStateDelete(t *testing.T) { { name: "Invalid key", key: "in va lid", - expected: TraceState{members: []member{ + expected: TraceState{list: []member{ {Key: "key1", Value: "val1"}, {Key: "key2", Value: "val2"}, {Key: "key3", Value: "val3"}, }}, - err: errInvalidKey, }, } for _, tc := range testCases { - actual, err := ts.Delete(tc.key) - assert.ErrorIs(t, err, tc.err, tc.name) - if tc.err != nil { - assert.Equal(t, ts, actual, tc.name) - } else { - assert.Equal(t, tc.expected, actual, tc.name) - } + assert.Equal(t, tc.expected, ts.Delete(tc.key), tc.name) } } func TestTraceStateInsert(t *testing.T) { - ts := TraceState{members: []member{ + ts := TraceState{list: []member{ {Key: "key1", Value: "val1"}, {Key: "key2", Value: "val2"}, {Key: "key3", Value: "val3"}, @@ -408,7 +400,7 @@ func TestTraceStateInsert(t *testing.T) { tracestate: ts, key: "key4@vendor", value: "val4", - expected: TraceState{members: []member{ + expected: TraceState{list: []member{ {Key: "key4@vendor", Value: "val4"}, {Key: "key1", Value: "val1"}, {Key: "key2", Value: "val2"}, @@ -420,7 +412,7 @@ func TestTraceStateInsert(t *testing.T) { tracestate: ts, key: "key2", value: "valX", - expected: TraceState{members: []member{ + expected: TraceState{list: []member{ {Key: "key2", Value: "valX"}, {Key: "key1", Value: "val1"}, {Key: "key3", Value: "val3"}, @@ -471,15 +463,15 @@ func TestTraceStateInsert(t *testing.T) { } } -func TestTraceStateIsEmpty(t *testing.T) { +func TestTraceStateLen(t *testing.T) { ts := TraceState{} assert.Equal(t, 0, ts.Len(), "zero value TraceState is empty") - ts, err := ts.Insert("key", "value") + key := "key" + ts, err := ts.Insert(key, "value") require.NoError(t, err) assert.Equal(t, 1, ts.Len(), "TraceState with inserted value") - ts, err = ts.Delete("key") - require.NoError(t, err) + ts = ts.Delete(key) assert.Equal(t, 0, ts.Len(), "TraceState with all values deleted") } From 60a21a250fdc7cb616e5486f8f97d6752e10d7d5 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 20 May 2021 13:57:09 -0700 Subject: [PATCH 08/12] Add immutability test for TraceState --- trace/tracestate_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/trace/tracestate_test.go b/trace/tracestate_test.go index b503bf25d12..3d87d0eb618 100644 --- a/trace/tracestate_test.go +++ b/trace/tracestate_test.go @@ -475,3 +475,36 @@ func TestTraceStateLen(t *testing.T) { ts = ts.Delete(key) assert.Equal(t, 0, ts.Len(), "TraceState with all values deleted") } + +func TestTraceStateImmutable(t *testing.T) { + k0, v0 := "k0", "v0" + ts0 := TraceState{list: []member{{k0, v0}}} + assert.Equal(t, v0, ts0.Get(k0)) + + // Insert should not modify the original. + k1, v1 := "k1", "v1" + ts1, err := ts0.Insert(k1, v1) + require.NoError(t, err) + assert.Equal(t, v0, ts0.Get(k0)) + assert.Equal(t, "", ts0.Get(k1)) + assert.Equal(t, v0, ts1.Get(k0)) + assert.Equal(t, v1, ts1.Get(k1)) + + // Update should not modify the original. + v2 := "v2" + ts2, err := ts1.Insert(k1, v2) + require.NoError(t, err) + assert.Equal(t, v0, ts0.Get(k0)) + assert.Equal(t, "", ts0.Get(k1)) + assert.Equal(t, v0, ts1.Get(k0)) + assert.Equal(t, v1, ts1.Get(k1)) + assert.Equal(t, v0, ts2.Get(k0)) + assert.Equal(t, v2, ts2.Get(k1)) + + // Delete should not modify the original. + ts3 := ts2.Delete(k0) + assert.Equal(t, v0, ts0.Get(k0)) + assert.Equal(t, v0, ts1.Get(k0)) + assert.Equal(t, v0, ts2.Get(k0)) + assert.Equal(t, "", ts3.Get(k0)) +} From 67be5221fbe1dd841d54a73278751d97a704633d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 20 May 2021 14:40:53 -0700 Subject: [PATCH 09/12] Add changes to changelog --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c59e6c59c8f..a4d2e45a828 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm This type can be used as a testing replacement for the `SpanSnapshot` that was removed from the `go.opentelemetry.io/otel/sdk/trace` package. (#1873) - Adds support for scheme in `OTEL_EXPORTER_OTLP_ENDPOINT` according to the spec. (#1886) - An example of using OpenTelemetry Go as a trace context forwarder. (#1912) +- `ParseTraceState` is added to the `go.opentelemetry.io/otel/trace` package. + It can be used to decode a `TraceState` from a `tracestate` header string value. (#1937) +- The `Len` method is added to the `TraceState` type in the `go.opentelemetry.io/otel/trace` package. + This method returns the number of list-members the `TraceState` holds. (#1937) ### Changed @@ -47,6 +51,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `"go.opentelemetry.io/otel".Tracer` function now accepts tracer options. (#1902) - Move the `go.opentelemetry.io/otel/unit` package to `go.opentelemetry.io/otel/metric/unit`. (#1903) - Refactor option types according to the contribution style guide. (#1882) +- Move the `go.opentelemetry.io/otel/trace.TraceStateFromKeyValues` function to the `go.opentelemetry.io/otel/oteltest` package. + This function is perserved for testing purposes where it may be useful to create a `TraceState` from `attribute.KeyValue`s, but it is not intended for production use. + The new `ParseTraceState` function should be used to create a `TraceState`. (#1931) +- The `MarshalJSON` method of the `go.opentelemetry.io/otel/trace.TraceState` type is updated to marshal the type in to the string representation of the `TraceState`. (#1931) +- The `TraceState.Delete` method from the `go.opentelemetry.io/otel/trace` package no longer returns an error in addition to a `TraceState`. (#1931) ### Deprecated @@ -66,6 +75,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm Using the same tracer that created a span introduces the error where an instrumentation library's `Tracer` is used by other code instead of their own. 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) ### Fixed @@ -73,6 +83,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `Shutdown` method of the simple `SpanProcessor` in the `go.opentelemetry.io/otel/sdk/trace` package now honors the context deadline or cancellation. (#1616, #1856) - BatchSpanProcessor now drops span batches that failed to be exported. (#1860) - Use `http://localhost:14268/api/traces` as default Jaeger collector endpoint instead of `http://localhost:14250`. (#1898) +- Allow trailing and leading whitespace in the parsing of a `tracestate` header. (#1931) ### Security From 1adb12472e7bfa8d75f5d5458344d9cfc6a58602 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 20 May 2021 14:51:59 -0700 Subject: [PATCH 10/12] Fixes --- CHANGELOG.md | 2 +- sdk/trace/trace_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4d2e45a828..065764e13ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,7 +52,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Move the `go.opentelemetry.io/otel/unit` package to `go.opentelemetry.io/otel/metric/unit`. (#1903) - Refactor option types according to the contribution style guide. (#1882) - Move the `go.opentelemetry.io/otel/trace.TraceStateFromKeyValues` function to the `go.opentelemetry.io/otel/oteltest` package. - This function is perserved for testing purposes where it may be useful to create a `TraceState` from `attribute.KeyValue`s, but it is not intended for production use. + This function is preserved for testing purposes where it may be useful to create a `TraceState` from `attribute.KeyValue`s, but it is not intended for production use. The new `ParseTraceState` function should be used to create a `TraceState`. (#1931) - The `MarshalJSON` method of the `go.opentelemetry.io/otel/trace.TraceState` type is updated to marshal the type in to the string representation of the `TraceState`. (#1931) - The `TraceState.Delete` method from the `go.opentelemetry.io/otel/trace` package no longer returns an error in addition to a `TraceState`. (#1931) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 03d6ae75380..46454e8f019 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -1603,7 +1603,7 @@ func TestSamplerTraceState(t *testing.T) { makeDeleter := func(k attribute.Key, prefix string) Sampler { return &stateSampler{ prefix: prefix, - f: func(t trace.TraceState) trace.TraceState { return mustTS(t.Delete(string(k))) }, + f: func(t trace.TraceState) trace.TraceState { return t.Delete(string(k)) }, } } clearer := func(prefix string) Sampler { From f08651e6daa3e33cf8c50c610ac40766a514e0d4 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 20 May 2021 15:28:35 -0700 Subject: [PATCH 11/12] Document argument type change in changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 065764e13ef..905eb249b43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm The new `ParseTraceState` function should be used to create a `TraceState`. (#1931) - The `MarshalJSON` method of the `go.opentelemetry.io/otel/trace.TraceState` type is updated to marshal the type in to the string representation of the `TraceState`. (#1931) - The `TraceState.Delete` method from the `go.opentelemetry.io/otel/trace` package no longer returns an error in addition to a `TraceState`. (#1931) +- The `Get` method of the `TraceState` type from the `go.opentelemetry.io/otel/trace` package has been updated to accept a `string` instead of an `attribute.Key` type. (#1931) +- The `Insert` method of the `TraceState` type from the `go.opentelemetry.io/otel/trace` package has been updated to accept a pair of `string`s instead of an `attribute.KeyValue` type. (#1931) +- The `Delete` method of the `TraceState` type from the `go.opentelemetry.io/otel/trace` package has been updated to accept a `string` instead of an `attribute.Key` type. (#1931) ### Deprecated From b4b2f6c973406488e4c046c2011b11d985837844 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 21 May 2021 14:36:23 -0700 Subject: [PATCH 12/12] Address feedback Remove circularity of TestTraceStateLen. --- trace/tracestate_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/trace/tracestate_test.go b/trace/tracestate_test.go index 3d87d0eb618..edb89073de8 100644 --- a/trace/tracestate_test.go +++ b/trace/tracestate_test.go @@ -468,12 +468,8 @@ func TestTraceStateLen(t *testing.T) { assert.Equal(t, 0, ts.Len(), "zero value TraceState is empty") key := "key" - ts, err := ts.Insert(key, "value") - require.NoError(t, err) - assert.Equal(t, 1, ts.Len(), "TraceState with inserted value") - - ts = ts.Delete(key) - assert.Equal(t, 0, ts.Len(), "TraceState with all values deleted") + ts = TraceState{list: []member{{key, "value"}}} + assert.Equal(t, 1, ts.Len(), "TraceState with one value") } func TestTraceStateImmutable(t *testing.T) {