Make SpanContext Immutable (open-telemetry#1573)

* Make SpanContext Immutable

* Adds NewSpanContext() constructor and SpanContextConfig{} struct for
constructing a new SpanContext when all fields are known
* Adds With<field>() methods to SpanContext for deriving a SpanContext
with a single field changed.
* Updates all uses of SpanContext to use the new API

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Update CHANGELOG.md

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Add tests for new SpanContext constructor and derivation

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Address PR feedback

* Fix new uses of SpanContext from main

CHANGELOG.md

@@ -8,12 +8,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
-## Added
+### Added
 
 - Added `Marshler` config option to `otlphttp` to enable otlp over json or protobufs. (#1586)
 - A `ForceFlush` method to the `"go.opentelemetry.io/otel/sdk/trace".TracerProvider` to flush all registered `SpanProcessor`s. (#1608)
 
-
 ### Changed
 
 - Update the `ForceFlush` method signature to the `"go.opentelemetry.io/otel/sdk/trace".SpanProcessor` to accept a `context.Context` and return an error. (#1608)
@@ -22,14 +21,15 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - `"go.opentelemetry.io/sdk/metric/controller.basic".WithPusher` is replaced with `WithExporter` to provide consistent naming across project. (#1656)
 - Added non-empty string check for trace `Attribute` keys. (#1659)
 - Add `description` to SpanStatus only when `StatusCode` is set to error. (#1662)
+- `trace.SpanContext` is now immutable and has no exported fields. (#1573)
+  - `trace.NewSpanContext()` can be used in conjunction with the `trace.SpanContextConfig` struct to initialize a new `SpanContext` where all values are known.
 
 ### Removed
 
 - Removed the exported `SimpleSpanProcessor` and `BatchSpanProcessor` structs.
    These are now returned as a SpanProcessor interface from their respective constructors. (#1638)
 - Removed setting status to `Error` while recording an error as a span event in `RecordError`. (#1663)
 
-
 ### Fixed
 
 - `SamplingResult.TraceState` is correctly propagated to a newly created

bridge/opencensus/bridge.go

@@ -186,5 +186,5 @@ func (s *span) AddLink(l octrace.Link) {
 }
 
 func (s *span) String() string {
-	return fmt.Sprintf("span %s", s.otSpan.SpanContext().SpanID.String())
+	return fmt.Sprintf("span %s", s.otSpan.SpanContext().SpanID().String())
 }

bridge/opencensus/bridge_test.go

@@ -66,8 +66,8 @@ func TestMixedAPIs(t *testing.T) {
 		// Reverse the order we look at the spans in, since they are listed in last-to-first order.
 		i = len(spans) - i - 1
 		// Verify that OpenCensus spans and opentelemetry spans have each other as parents.
-		if spans[i].ParentSpanID() != parent.SpanContext().SpanID {
-			t.Errorf("Span %v had parent %v.  Expected %d", spans[i].Name(), spans[i].ParentSpanID(), parent.SpanContext().SpanID)
+		if spans[i].ParentSpanID() != parent.SpanContext().SpanID() {
+			t.Errorf("Span %v had parent %v.  Expected %d", spans[i].Name(), spans[i].ParentSpanID(), parent.SpanContext().SpanID())
 		}
 		parent = spans[i]
 	}
@@ -111,8 +111,8 @@ func TestStartSpanWithRemoteParent(t *testing.T) {
 		t.Fatalf("Got %d spans, exepected %d", len(spans), 1)
 	}
 
-	if spans[0].ParentSpanID() != parent.SpanContext().SpanID {
-		t.Errorf("Span %v, had parent %v.  Expected %d", spans[0].Name(), spans[0].ParentSpanID(), parent.SpanContext().SpanID)
+	if spans[0].ParentSpanID() != parent.SpanContext().SpanID() {
+		t.Errorf("Span %v, had parent %v.  Expected %d", spans[0].Name(), spans[0].ParentSpanID(), parent.SpanContext().SpanID())
 	}
 }
 
@@ -150,8 +150,8 @@ func TestToFromContext(t *testing.T) {
 		// Reverse the order we look at the spans in, since they are listed in last-to-first order.
 		i = len(spans) - i - 1
 		// Verify that OpenCensus spans and opentelemetry spans have each other as parents.
-		if spans[i].ParentSpanID() != parent.SpanContext().SpanID {
-			t.Errorf("Span %v had parent %v.  Expected %d", spans[i].Name(), spans[i].ParentSpanID(), parent.SpanContext().SpanID)
+		if spans[i].ParentSpanID() != parent.SpanContext().SpanID() {
+			t.Errorf("Span %v had parent %v.  Expected %d", spans[i].Name(), spans[i].ParentSpanID(), parent.SpanContext().SpanID())
 		}
 		parent = spans[i]
 	}

bridge/opencensus/utils/utils.go

@@ -28,16 +28,16 @@ import (
 // error handler.
 func OTelSpanContextToOC(sc trace.SpanContext) octrace.SpanContext {
 	if sc.IsDebug() || sc.IsDeferred() {
-		otel.Handle(fmt.Errorf("ignoring OpenTelemetry Debug or Deferred trace flags for span %q because they are not supported by OpenCensus", sc.SpanID))
+		otel.Handle(fmt.Errorf("ignoring OpenTelemetry Debug or Deferred trace flags for span %q because they are not supported by OpenCensus", sc.SpanID()))
 	}
 	var to octrace.TraceOptions
 	if sc.IsSampled() {
 		// OpenCensus doesn't expose functions to directly set sampled
 		to = 0x1
 	}
 	return octrace.SpanContext{
-		TraceID:      octrace.TraceID(sc.TraceID),
-		SpanID:       octrace.SpanID(sc.SpanID),
+		TraceID:      octrace.TraceID(sc.TraceID()),
+		SpanID:       octrace.SpanID(sc.SpanID()),
 		TraceOptions: to,
 	}
 }
@@ -49,9 +49,9 @@ func OCSpanContextToOTel(sc octrace.SpanContext) trace.SpanContext {
 	if sc.IsSampled() {
 		traceFlags = trace.FlagsSampled
 	}
-	return trace.SpanContext{
+	return trace.NewSpanContext(trace.SpanContextConfig{
 		TraceID:    trace.TraceID(sc.TraceID),
 		SpanID:     trace.SpanID(sc.SpanID),
 		TraceFlags: traceFlags,
-	}
+	})
 }

bridge/opencensus/utils/utils_test.go

@@ -35,11 +35,11 @@ func TestOTelSpanContextToOC(t *testing.T) {
 		},
 		{
 			description: "sampled",
-			input: trace.SpanContext{
+			input: trace.NewSpanContext(trace.SpanContextConfig{
 				TraceID:    trace.TraceID([16]byte{1}),
 				SpanID:     trace.SpanID([8]byte{2}),
 				TraceFlags: trace.FlagsSampled,
-			},
+			}),
 			expected: octrace.SpanContext{
 				TraceID:      octrace.TraceID([16]byte{1}),
 				SpanID:       octrace.SpanID([8]byte{2}),
@@ -48,10 +48,10 @@ func TestOTelSpanContextToOC(t *testing.T) {
 		},
 		{
 			description: "not sampled",
-			input: trace.SpanContext{
+			input: trace.NewSpanContext(trace.SpanContextConfig{
 				TraceID: trace.TraceID([16]byte{1}),
 				SpanID:  trace.SpanID([8]byte{2}),
-			},
+			}),
 			expected: octrace.SpanContext{
 				TraceID:      octrace.TraceID([16]byte{1}),
 				SpanID:       octrace.SpanID([8]byte{2}),
@@ -60,11 +60,11 @@ func TestOTelSpanContextToOC(t *testing.T) {
 		},
 		{
 			description: "debug flag",
-			input: trace.SpanContext{
+			input: trace.NewSpanContext(trace.SpanContextConfig{
 				TraceID:    trace.TraceID([16]byte{1}),
 				SpanID:     trace.SpanID([8]byte{2}),
 				TraceFlags: trace.FlagsDebug,
-			},
+			}),
 			expected: octrace.SpanContext{
 				TraceID:      octrace.TraceID([16]byte{1}),
 				SpanID:       octrace.SpanID([8]byte{2}),
@@ -97,11 +97,11 @@ func TestOCSpanContextToOTel(t *testing.T) {
 				SpanID:       octrace.SpanID([8]byte{2}),
 				TraceOptions: octrace.TraceOptions(0x1),
 			},
-			expected: trace.SpanContext{
+			expected: trace.NewSpanContext(trace.SpanContextConfig{
 				TraceID:    trace.TraceID([16]byte{1}),
 				SpanID:     trace.SpanID([8]byte{2}),
 				TraceFlags: trace.FlagsSampled,
-			},
+			}),
 		},
 		{
 			description: "not sampled",
@@ -110,10 +110,10 @@ func TestOCSpanContextToOTel(t *testing.T) {
 				SpanID:       octrace.SpanID([8]byte{2}),
 				TraceOptions: octrace.TraceOptions(0),
 			},
-			expected: trace.SpanContext{
+			expected: trace.NewSpanContext(trace.SpanContextConfig{
 				TraceID: trace.TraceID([16]byte{1}),
 				SpanID:  trace.SpanID([8]byte{2}),
-			},
+			}),
 		},
 		{
 			description: "trace state is ignored",
@@ -122,17 +122,15 @@ func TestOCSpanContextToOTel(t *testing.T) {
 				SpanID:     octrace.SpanID([8]byte{2}),
 				Tracestate: &tracestate.Tracestate{},
 			},
-			expected: trace.SpanContext{
+			expected: trace.NewSpanContext(trace.SpanContextConfig{
 				TraceID: trace.TraceID([16]byte{1}),
 				SpanID:  trace.SpanID([8]byte{2}),
-			},
+			}),
 		},
 	} {
 		t.Run(tc.description, func(t *testing.T) {
 			output := OCSpanContextToOTel(tc.input)
-			if output.SpanID != tc.expected.SpanID ||
-				output.TraceID != tc.expected.TraceID ||
-				output.TraceFlags != tc.expected.TraceFlags {
+			if !output.Equal(tc.expected) {
 				t.Fatalf("Got %+v spancontext, exepected %+v.", output, tc.expected)
 			}
 		})

bridge/opentracing/internal/mock.go

@@ -76,11 +76,11 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...trace.SpanO
 	if startTime.IsZero() {
 		startTime = time.Now()
 	}
-	spanContext := trace.SpanContext{
+	spanContext := trace.NewSpanContext(trace.SpanContextConfig{
 		TraceID:    t.getTraceID(ctx, config),
 		SpanID:     t.getSpanID(),
 		TraceFlags: 0,
-	}
+	})
 	span := &MockSpan{
 		mockTracer:     t,
 		officialTracer: t,
@@ -117,7 +117,7 @@ func (t *MockTracer) addSpareContextValue(ctx context.Context) context.Context {
 
 func (t *MockTracer) getTraceID(ctx context.Context, config *trace.SpanConfig) trace.TraceID {
 	if parent := t.getParentSpanContext(ctx, config); parent.IsValid() {
-		return parent.TraceID
+		return parent.TraceID()
 	}
 	if len(t.SpareTraceIDs) > 0 {
 		traceID := t.SpareTraceIDs[0]
@@ -132,7 +132,7 @@ func (t *MockTracer) getTraceID(ctx context.Context, config *trace.SpanConfig) t
 
 func (t *MockTracer) getParentSpanID(ctx context.Context, config *trace.SpanConfig) trace.SpanID {
 	if parent := t.getParentSpanContext(ctx, config); parent.IsValid() {
-		return parent.SpanID
+		return parent.SpanID()
 	}
 	return trace.SpanID{}
 }

bridge/opentracing/mix_test.go

@@ -229,12 +229,12 @@ func (cast *currentActiveSpanTest) runOTOtelOT(t *testing.T, ctx context.Context
 }
 
 func (cast *currentActiveSpanTest) recordSpans(t *testing.T, ctx context.Context) context.Context {
-	spanID := trace.SpanContextFromContext(ctx).SpanID
+	spanID := trace.SpanContextFromContext(ctx).SpanID()
 	cast.recordedCurrentOtelSpanIDs = append(cast.recordedCurrentOtelSpanIDs, spanID)
 
 	spanID = trace.SpanID{}
 	if bridgeSpan, ok := ot.SpanFromContext(ctx).(*bridgeSpan); ok {
-		spanID = bridgeSpan.otelSpan.SpanContext().SpanID
+		spanID = bridgeSpan.otelSpan.SpanContext().SpanID()
 	}
 	cast.recordedActiveOTSpanIDs = append(cast.recordedActiveOTSpanIDs, spanID)
 	return ctx
@@ -637,19 +637,19 @@ func checkTraceAndSpans(t *testing.T, tracer *internal.MockTracer, expectedTrace
 	}
 	for idx, span := range tracer.FinishedSpans {
 		sctx := span.SpanContext()
-		if sctx.TraceID != expectedTraceID {
-			t.Errorf("Expected trace ID %v in span %d (%d), got %v", expectedTraceID, idx, sctx.SpanID, sctx.TraceID)
+		if sctx.TraceID() != expectedTraceID {
+			t.Errorf("Expected trace ID %v in span %d (%d), got %v", expectedTraceID, idx, sctx.SpanID(), sctx.TraceID())
 		}
 		expectedSpanID := spanIDs[idx]
 		expectedParentSpanID := parentSpanIDs[idx]
-		if sctx.SpanID != expectedSpanID {
-			t.Errorf("Expected finished span %d to have span ID %d, but got %d", idx, expectedSpanID, sctx.SpanID)
+		if sctx.SpanID() != expectedSpanID {
+			t.Errorf("Expected finished span %d to have span ID %d, but got %d", idx, expectedSpanID, sctx.SpanID())
 		}
 		if span.ParentSpanID != expectedParentSpanID {
-			t.Errorf("Expected finished span %d (span ID: %d) to have parent span ID %d, but got %d", idx, sctx.SpanID, expectedParentSpanID, span.ParentSpanID)
+			t.Errorf("Expected finished span %d (span ID: %d) to have parent span ID %d, but got %d", idx, sctx.SpanID(), expectedParentSpanID, span.ParentSpanID)
 		}
-		if span.SpanKind != sks[span.SpanContext().SpanID] {
-			t.Errorf("Expected finished span %d (span ID: %d) to have span.kind to be '%v' but was '%v'", idx, sctx.SpanID, sks[span.SpanContext().SpanID], span.SpanKind)
+		if span.SpanKind != sks[span.SpanContext().SpanID()] {
+			t.Errorf("Expected finished span %d (span ID: %d) to have span.kind to be '%v' but was '%v'", idx, sctx.SpanID(), sks[span.SpanContext().SpanID()], span.SpanKind)
 		}
 	}
 }

exporters/otlp/internal/otlptest/data.go

@@ -82,11 +82,11 @@ func (OneRecordCheckpointSet) ForEach(kindSelector exportmetric.ExportKindSelect
 // may be useful for testing driver's trace export.
 func SingleSpanSnapshot() []*exporttrace.SpanSnapshot {
 	sd := &exporttrace.SpanSnapshot{
-		SpanContext: trace.SpanContext{
+		SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 			TraceID:    trace.TraceID{2, 3, 4, 5, 6, 7, 8, 9, 2, 3, 4, 5, 6, 7, 8, 9},
 			SpanID:     trace.SpanID{3, 4, 5, 6, 7, 8, 9, 0},
 			TraceFlags: trace.FlagsSampled,
-		},
+		}),
 		ParentSpanID:             trace.SpanID{1, 2, 3, 4, 5, 6, 7, 8},
 		SpanKind:                 trace.SpanKindInternal,
 		Name:                     "foo",

exporters/otlp/internal/transform/span.go

@@ -101,10 +101,13 @@ func span(sd *export.SpanSnapshot) *tracepb.Span {
 		return nil
 	}
 
+	tid := sd.SpanContext.TraceID()
+	sid := sd.SpanContext.SpanID()
+
 	s := &tracepb.Span{
-		TraceId:                sd.SpanContext.TraceID[:],
-		SpanId:                 sd.SpanContext.SpanID[:],
-		TraceState:             sd.SpanContext.TraceState.String(),
+		TraceId:                tid[:],
+		SpanId:                 sid[:],
+		TraceState:             sd.SpanContext.TraceState().String(),
 		Status:                 status(sd.StatusCode, sd.StatusMessage),
 		StartTimeUnixNano:      uint64(sd.StartTime.UnixNano()),
 		EndTimeUnixNano:        uint64(sd.EndTime.UnixNano()),
@@ -152,9 +155,12 @@ func links(links []trace.Link) []*tracepb.Span_Link {
 		// being reused -- in short we need a new otLink per iteration.
 		otLink := otLink
 
+		tid := otLink.TraceID()
+		sid := otLink.SpanID()
+
 		sl = append(sl, &tracepb.Span_Link{
-			TraceId:    otLink.TraceID[:],
-			SpanId:     otLink.SpanID[:],
+			TraceId:    tid[:],
+			SpanId:     sid[:],
 			Attributes: Attributes(otLink.Attributes),
 		})
 	}

exporters/otlp/internal/transform/span_test.go

@@ -148,8 +148,7 @@ func TestLinks(t *testing.T) {
 	assert.Equal(t, expected, got[1])
 
 	// Changes to our links should not change the produced links.
-	l[1].TraceID[0] = byte(0x1)
-	l[1].SpanID[0] = byte(0x1)
+	l[1].SpanContext = l[1].WithTraceID(trace.TraceID{})
 	assert.Equal(t, expected, got[1])
 }
 
@@ -201,11 +200,11 @@ func TestSpanData(t *testing.T) {
 	endTime := startTime.Add(10 * time.Second)
 	traceState, _ := trace.TraceStateFromKeyValues(attribute.String("key1", "val1"), attribute.String("key2", "val2"))
 	spanData := &export.SpanSnapshot{
-		SpanContext: trace.SpanContext{
+		SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 			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},
 		Name:         "span data to span data",
@@ -225,21 +224,21 @@ func TestSpanData(t *testing.T) {
 		},
 		Links: []trace.Link{
 			{
-				SpanContext: trace.SpanContext{
+				SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 					TraceID:    trace.TraceID{0xC0, 0xC1, 0xC2, 0xC3, 0xC4, 0xC5, 0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xCB, 0xCC, 0xCD, 0xCE, 0xCF},
 					SpanID:     trace.SpanID{0xB0, 0xB1, 0xB2, 0xB3, 0xB4, 0xB5, 0xB6, 0xB7},
 					TraceFlags: 0,
-				},
+				}),
 				Attributes: []attribute.KeyValue{
 					attribute.String("LinkType", "Parent"),
 				},
 			},
 			{
-				SpanContext: trace.SpanContext{
+				SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 					TraceID:    trace.TraceID{0xE0, 0xE1, 0xE2, 0xE3, 0xE4, 0xE5, 0xE6, 0xE7, 0xE8, 0xE9, 0xEA, 0xEB, 0xEC, 0xED, 0xEE, 0xEF},
 					SpanID:     trace.SpanID{0xD0, 0xD1, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6, 0xD7},
 					TraceFlags: 0,
-				},
+				}),
 				Attributes: []attribute.KeyValue{
 					attribute.String("LinkType", "Child"),
 				},