Refactor Tracestate (#1931)

* Refactor TraceState

* Update tracecontext propagator to use new TraceState

* Add TraceStateFromKeyValues to oteltest

* Use oteltest to test TraceState

* Replace IsEmpty with Len for TraceState

* Replace ParseTraceState with ParseTraceStateString

* Clean up naming

* Add immutability test for TraceState

* Add changes to changelog

* Fixes

* Document argument type change in changelog

* Address feedback

Remove circularity of TestTraceStateLen.

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,14 @@ 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 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)
+- 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
 
@@ -66,13 +78,15 @@ 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
 
 - Only report errors from the `"go.opentelemetry.io/otel/sdk/resource".Environment` function when they are not `nil`. (#1850, #1851)
 - 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
 

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

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},

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

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,

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

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.ParseTraceState(strings.Join(members, ","))
+}

oteltest/tracestate_test.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 (
+	"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.Equal(t, 0, ts.Len(), "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())
+}

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.ParseTraceState(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
-}

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())
 	}

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

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 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,
 		},
 	}