Add TraceState to SpanContext in API (open-telemetry#1340)

* Add TraceState to API * Add tests for TraceState * Update related tests - stdout exporter test - SDK test * Update OTLP span transform * Update CHANGELOG * Change TraceState to struct instead of pointer - Adjust tests for trace API - Adjust adjacent parts of codebase (test utils, SDK etc.) * Add methods to assert equality - for type SpanContext, if SpanID, TraceID, TraceFlag and TraceState are equal - for type TraceState, if entries of both respective trace states are equal Signed-off-by: Matej Gera <matejgera@gmail.com> * Copy values for new TraceState, adjust tests * Use IsEqualWith in remaining tests instead of assertion func * Further feedback, minor improvements - Move IsEqualWith method to be only in test package - Minor improvements, typos etc. Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
ctsquires · Dec 21, 2020 · 439cd31 · 439cd31
1 parent 3521526
commit 439cd31
Show file tree

Hide file tree

Showing 14 changed files with 573 additions and 64 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -54,6 +54,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - An `EventOption` and the related `NewEventConfig` function are added to the `go.opentelemetry.io/otel` package to configure Span events. (#1254)
 - A `TextMapPropagator` and associated `TextMapCarrier` are added to the `go.opentelemetry.io/otel/oteltest` package to test `TextMap` type propagators and their use. (#1259)
 - `SpanContextFromContext` returns `SpanContext` from context. (#1255)
+- `TraceState` has been added to `SpanContext`. (#1340)
 - `DeploymentEnvironmentKey` added to `go.opentelemetry.io/otel/semconv` package. (#1323)
 - Add an OpenCensus to OpenTelemetry tracing bridge. (#1305)
 - Add a parent context argument to `SpanProcessor.OnStart` to follow the specification. (#1333)

diff --git a/bridge/opencensus/utils/utils_test.go b/bridge/opencensus/utils/utils_test.go
@@ -130,7 +130,9 @@ func TestOCSpanContextToOTel(t *testing.T) {
 	} {
 		t.Run(tc.description, func(t *testing.T) {
 			output := OCSpanContextToOTel(tc.input)
-			if output != tc.expected {
+			if output.SpanID != tc.expected.SpanID ||
+				output.TraceID != tc.expected.TraceID ||
+				output.TraceFlags != tc.expected.TraceFlags {
 				t.Fatalf("Got %+v spancontext, exepected %+v.", output, tc.expected)
 			}
 		})

diff --git a/exporters/otlp/internal/transform/span.go b/exporters/otlp/internal/transform/span.go
@@ -102,17 +102,17 @@ func span(sd *export.SpanSnapshot) *tracepb.Span {
 	}
 
 	s := &tracepb.Span{
-		TraceId:           sd.SpanContext.TraceID[:],
-		SpanId:            sd.SpanContext.SpanID[:],
-		Status:            status(sd.StatusCode, sd.StatusMessage),
-		StartTimeUnixNano: uint64(sd.StartTime.UnixNano()),
-		EndTimeUnixNano:   uint64(sd.EndTime.UnixNano()),
-		Links:             links(sd.Links),
-		Kind:              spanKind(sd.SpanKind),
-		Name:              sd.Name,
-		Attributes:        Attributes(sd.Attributes),
-		Events:            spanEvents(sd.MessageEvents),
-		// TODO (rghetia): Add Tracestate: when supported.
+		TraceId:                sd.SpanContext.TraceID[:],
+		SpanId:                 sd.SpanContext.SpanID[:],
+		TraceState:             sd.SpanContext.TraceState.String(),
+		Status:                 status(sd.StatusCode, sd.StatusMessage),
+		StartTimeUnixNano:      uint64(sd.StartTime.UnixNano()),
+		EndTimeUnixNano:        uint64(sd.EndTime.UnixNano()),
+		Links:                  links(sd.Links),
+		Kind:                   spanKind(sd.SpanKind),
+		Name:                   sd.Name,
+		Attributes:             Attributes(sd.Attributes),
+		Events:                 spanEvents(sd.MessageEvents),
 		DroppedAttributesCount: uint32(sd.DroppedAttributeCount),
 		DroppedEventsCount:     uint32(sd.DroppedMessageEventCount),
 		DroppedLinksCount:      uint32(sd.DroppedLinkCount),

diff --git a/exporters/otlp/internal/transform/span_test.go b/exporters/otlp/internal/transform/span_test.go
@@ -199,10 +199,12 @@ 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(label.String("key1", "val1"), label.String("key2", "val2"))
 	spanData := &export.SpanSnapshot{
 		SpanContext: trace.SpanContext{
-			TraceID: trace.TraceID{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F},
-			SpanID:  trace.SpanID{0xFF, 0xFE, 0xFD, 0xFC, 0xFB, 0xFA, 0xF9, 0xF8},
+			TraceID:    trace.TraceID{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F},
+			SpanID:     trace.SpanID{0xFF, 0xFE, 0xFD, 0xFC, 0xFB, 0xFA, 0xF9, 0xF8},
+			TraceState: traceState,
 		},
 		SpanKind:     trace.SpanKindServer,
 		ParentSpanID: trace.SpanID{0xEF, 0xEE, 0xED, 0xEC, 0xEB, 0xEA, 0xE9, 0xE8},
@@ -266,6 +268,7 @@ func TestSpanData(t *testing.T) {
 		TraceId:                []byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F},
 		SpanId:                 []byte{0xFF, 0xFE, 0xFD, 0xFC, 0xFB, 0xFA, 0xF9, 0xF8},
 		ParentSpanId:           []byte{0xEF, 0xEE, 0xED, 0xEC, 0xEB, 0xEA, 0xE9, 0xE8},
+		TraceState:             "key1=val1,key2=val2",
 		Name:                   spanData.Name,
 		Kind:                   tracepb.Span_SPAN_KIND_SERVER,
 		StartTimeUnixNano:      uint64(startTime.UnixNano()),

diff --git a/exporters/stdout/trace_test.go b/exporters/stdout/trace_test.go
@@ -42,14 +42,16 @@ func TestExporter_ExportSpan(t *testing.T) {
 	now := time.Now()
 	traceID, _ := trace.TraceIDFromHex("0102030405060708090a0b0c0d0e0f10")
 	spanID, _ := trace.SpanIDFromHex("0102030405060708")
+	traceState, _ := trace.TraceStateFromKeyValues(label.String("key", "val"))
 	keyValue := "value"
 	doubleValue := 123.456
 	resource := resource.NewWithAttributes(label.String("rk1", "rv11"))
 
 	testSpan := &export.SpanSnapshot{
 		SpanContext: trace.SpanContext{
-			TraceID: traceID,
-			SpanID:  spanID,
+			TraceID:    traceID,
+			SpanID:     spanID,
+			TraceState: traceState,
 		},
 		Name:      "/foo",
 		StartTime: now,
@@ -76,7 +78,12 @@ func TestExporter_ExportSpan(t *testing.T) {
 	got := b.String()
 	expectedOutput := `[{"SpanContext":{` +
 		`"TraceID":"0102030405060708090a0b0c0d0e0f10",` +
-		`"SpanID":"0102030405060708","TraceFlags":0},` +
+		`"SpanID":"0102030405060708","TraceFlags":0,` +
+		`"TraceState":[` +
+		`{` +
+		`"Key":"key",` +
+		`"Value":{"Type":"STRING","Value":"val"}` +
+		`}]},` +
 		`"ParentSpanID":"0000000000000000",` +
 		`"SpanKind":1,` +
 		`"Name":"/foo",` +

diff --git a/oteltest/span.go b/oteltest/span.go
@@ -47,7 +47,7 @@ type Span struct {
 	statusMessage string
 	attributes    map[label.Key]label.Value
 	events        []Event
-	links         map[trace.SpanContext][]label.KeyValue
+	links         []trace.Link
 	spanKind      trace.SpanKind
 }
 
@@ -206,15 +206,7 @@ func (s *Span) Events() []Event { return s.events }
 
 // Links returns the links set on s at creation time. If multiple links for
 // the same SpanContext were set, the last link will be used.
-func (s *Span) Links() map[trace.SpanContext][]label.KeyValue {
-	links := make(map[trace.SpanContext][]label.KeyValue)
-
-	for sc, attributes := range s.links {
-		links[sc] = append([]label.KeyValue{}, attributes...)
-	}
-
-	return links
-}
+func (s *Span) Links() []trace.Link { return s.links }
 
 // StartTime returns the time at which s was started. This will be the
 // wall-clock time unless a specific start time was provided.

diff --git a/oteltest/tracer.go b/oteltest/tracer.go
@@ -47,7 +47,7 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOptio
 		tracer:     t,
 		startTime:  startTime,
 		attributes: make(map[label.Key]label.Value),
-		links:      make(map[trace.SpanContext][]label.KeyValue),
+		links:      []trace.Link{},
 		spanKind:   c.SpanKind,
 	}
 
@@ -56,10 +56,16 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOptio
 
 		iodKey := label.Key("ignored-on-demand")
 		if lsc := trace.SpanContextFromContext(ctx); lsc.IsValid() {
-			span.links[lsc] = []label.KeyValue{iodKey.String("current")}
+			span.links = append(span.links, trace.Link{
+				SpanContext: lsc,
+				Attributes:  []label.KeyValue{iodKey.String("current")},
+			})
 		}
 		if rsc := trace.RemoteSpanContextFromContext(ctx); rsc.IsValid() {
-			span.links[rsc] = []label.KeyValue{iodKey.String("remote")}
+			span.links = append(span.links, trace.Link{
+				SpanContext: rsc,
+				Attributes:  []label.KeyValue{iodKey.String("remote")},
+			})
 		}
 	} else {
 		span.spanContext = t.config.SpanContextFunc(ctx)
@@ -73,7 +79,16 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOptio
 	}
 
 	for _, link := range c.Links {
-		span.links[link.SpanContext] = link.Attributes
+		for i, sl := range span.links {
+			if sl.SpanContext.SpanID == link.SpanContext.SpanID &&
+				sl.SpanContext.TraceID == link.SpanContext.TraceID &&
+				sl.SpanContext.TraceFlags == link.SpanContext.TraceFlags &&
+				sl.SpanContext.TraceState.String() == link.SpanContext.TraceState.String() {
+				span.links[i].Attributes = link.Attributes
+				break
+			}
+		}
+		span.links = append(span.links, link)
 	}
 
 	span.SetName(name)

diff --git a/oteltest/tracer_test.go b/oteltest/tracer_test.go
@@ -211,14 +211,7 @@ func TestTracer(t *testing.T) {
 					},
 				},
 			}
-			tsLinks := testSpan.Links()
-			gotLinks := make([]trace.Link, 0, len(tsLinks))
-			for sc, attributes := range tsLinks {
-				gotLinks = append(gotLinks, trace.Link{
-					SpanContext: sc,
-					Attributes:  attributes,
-				})
-			}
+			gotLinks := testSpan.Links()
 			e.Expect(gotLinks).ToMatchInAnyOrder(expectedLinks)
 		})
 
@@ -251,8 +244,8 @@ func TestTracer(t *testing.T) {
 			e.Expect(ok).ToBeTrue()
 
 			links := testSpan.Links()
-			e.Expect(links[link1.SpanContext]).ToEqual(link1.Attributes)
-			e.Expect(links[link2.SpanContext]).ToEqual(link2.Attributes)
+			e.Expect(links[0].Attributes).ToEqual(link1.Attributes)
+			e.Expect(links[1].Attributes).ToEqual(link2.Attributes)
 		})
 	})
 }

diff --git a/propagation/trace_context_test.go b/propagation/trace_context_test.go
@@ -113,7 +113,7 @@ func TestExtractValidTraceContextFromHTTPReq(t *testing.T) {
 			ctx := context.Background()
 			ctx = prop.Extract(ctx, req.Header)
 			gotSc := trace.RemoteSpanContextFromContext(ctx)
-			if diff := cmp.Diff(gotSc, tt.wantSc); diff != "" {
+			if diff := cmp.Diff(gotSc, tt.wantSc, cmp.AllowUnexported(trace.TraceState{})); diff != "" {
 				t.Errorf("Extract Tracecontext: %s: -got +want %s", tt.name, diff)
 			}
 		})
@@ -201,7 +201,7 @@ func TestExtractInvalidTraceContextFromHTTPReq(t *testing.T) {
 			ctx := context.Background()
 			ctx = prop.Extract(ctx, req.Header)
 			gotSc := trace.RemoteSpanContextFromContext(ctx)
-			if diff := cmp.Diff(gotSc, wantSc); diff != "" {
+			if diff := cmp.Diff(gotSc, wantSc, cmp.AllowUnexported(trace.TraceState{})); diff != "" {
 				t.Errorf("Extract Tracecontext: %s: -got +want %s", tt.name, diff)
 			}
 		})

diff --git a/sdk/trace/span.go b/sdk/trace/span.go
@@ -473,7 +473,7 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trac
 
 	cfg := tr.provider.config.Load().(*Config)
 
-	if parent == emptySpanContext {
+	if hasEmptySpanContext(parent) {
 		// Generate both TraceID and SpanID
 		span.spanContext.TraceID, span.spanContext.SpanID = cfg.IDGenerator.NewIDs(ctx)
 	} else {
@@ -486,7 +486,7 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trac
 	span.links = newEvictedQueue(cfg.MaxLinksPerSpan)
 
 	data := samplingData{
-		noParent:     parent == emptySpanContext,
+		noParent:     hasEmptySpanContext(parent),
 		remoteParent: remoteParent,
 		parent:       parent,
 		name:         name,
@@ -521,6 +521,13 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trac
 	return span
 }
 
+func hasEmptySpanContext(parent trace.SpanContext) bool {
+	return parent.SpanID == emptySpanContext.SpanID &&
+		parent.TraceID == emptySpanContext.TraceID &&
+		parent.TraceFlags == emptySpanContext.TraceFlags &&
+		parent.TraceState.IsEmpty()
+}
+
 type samplingData struct {
 	noParent     bool
 	remoteParent bool

diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go
@@ -316,34 +316,38 @@ func TestStartSpanWithParent(t *testing.T) {
 		TraceFlags: 0x1,
 	}
 	_, s1 := tr.Start(trace.ContextWithRemoteSpanContext(ctx, sc1), "span1-unsampled-parent1")
-	if err := checkChild(sc1, s1); err != nil {
+	if err := checkChild(t, sc1, s1); err != nil {
 		t.Error(err)
 	}
 
 	_, s2 := tr.Start(trace.ContextWithRemoteSpanContext(ctx, sc1), "span2-unsampled-parent1")
-	if err := checkChild(sc1, s2); err != nil {
+	if err := checkChild(t, sc1, s2); err != nil {
 		t.Error(err)
 	}
 
+	ts, err := trace.TraceStateFromKeyValues(label.String("k", "v"))
+	if err != nil {
+		t.Error(err)
+	}
 	sc2 := trace.SpanContext{
 		TraceID:    tid,
 		SpanID:     sid,
 		TraceFlags: 0x1,
-		//Tracestate:   testTracestate,
+		TraceState: ts,
 	}
 	_, s3 := tr.Start(trace.ContextWithRemoteSpanContext(ctx, sc2), "span3-sampled-parent2")
-	if err := checkChild(sc2, s3); err != nil {
+	if err := checkChild(t, sc2, s3); err != nil {
 		t.Error(err)
 	}
 
 	ctx2, s4 := tr.Start(trace.ContextWithRemoteSpanContext(ctx, sc2), "span4-sampled-parent2")
-	if err := checkChild(sc2, s4); err != nil {
+	if err := checkChild(t, sc2, s4); err != nil {
 		t.Error(err)
 	}
 
 	s4Sc := s4.SpanContext()
 	_, s5 := tr.Start(ctx2, "span5-implicit-childof-span4")
-	if err := checkChild(s4Sc, s5); err != nil {
+	if err := checkChild(t, s4Sc, s5); err != nil {
 		t.Error(err)
 	}
 }
@@ -751,7 +755,8 @@ func TestSetSpanStatus(t *testing.T) {
 func cmpDiff(x, y interface{}) string {
 	return cmp.Diff(x, y,
 		cmp.AllowUnexported(label.Value{}),
-		cmp.AllowUnexported(export.Event{}))
+		cmp.AllowUnexported(export.Event{}),
+		cmp.AllowUnexported(trace.TraceState{}))
 }
 
 func remoteSpanContext() trace.SpanContext {
@@ -764,7 +769,7 @@ func remoteSpanContext() trace.SpanContext {
 
 // checkChild is test utility function that tests that c has fields set appropriately,
 // given that it is a child span of p.
-func checkChild(p trace.SpanContext, apiSpan trace.Span) error {
+func checkChild(t *testing.T, p trace.SpanContext, apiSpan trace.Span) error {
 	s := apiSpan.(*span)
 	if s == nil {
 		return fmt.Errorf("got nil child span, want non-nil")
@@ -778,10 +783,8 @@ func checkChild(p trace.SpanContext, apiSpan trace.Span) error {
 	if got, want := s.spanContext.TraceFlags, p.TraceFlags; got != want {
 		return fmt.Errorf("got child trace options %d, want %d", got, want)
 	}
-	// TODO [rgheita] : Fix tracestate test
-	//if got, want := c.spanContext.Tracestate, p.Tracestate; got != want {
-	//	return fmt.Errorf("got child tracestate %v, want %v", got, want)
-	//}
+	got, want := s.spanContext.TraceState, p.TraceState
+	assert.Equal(t, want, got)
 	return nil
 }