Add parent context to SpanProcessor.OnStart

The spec requires doing so. Right now SpanProcessor implementations aren't doing anything with this argument.
open-telemetry · Nov 13, 2020 · 56907fe · 56907fe
1 parent f6df5df
commit 56907fe
Show file tree

Hide file tree

Showing 8 changed files with 63 additions and 23 deletions.
diff --git a/sdk/trace/batch_span_processor.go b/sdk/trace/batch_span_processor.go
@@ -112,7 +112,7 @@ func NewBatchSpanProcessor(exporter export.SpanExporter, options ...BatchSpanPro
 }
 
 // OnStart method does nothing.
-func (bsp *BatchSpanProcessor) OnStart(sd *export.SpanData) {}
+func (bsp *BatchSpanProcessor) OnStart(parent context.Context, sd *export.SpanData) {}
 
 // OnEnd method enqueues export.SpanData for later processing.
 func (bsp *BatchSpanProcessor) OnEnd(sd *export.SpanData) {

diff --git a/sdk/trace/batch_span_processor_test.go b/sdk/trace/batch_span_processor_test.go
@@ -63,7 +63,7 @@ var _ export.SpanExporter = (*testBatchExporter)(nil)
 func TestNewBatchSpanProcessorWithNilExporter(t *testing.T) {
 	bsp := sdktrace.NewBatchSpanProcessor(nil)
 	// These should not panic.
-	bsp.OnStart(&export.SpanData{})
+	bsp.OnStart(context.Background(), &export.SpanData{})
 	bsp.OnEnd(&export.SpanData{})
 	bsp.ForceFlush()
 	err := bsp.Shutdown(context.Background())

diff --git a/sdk/trace/provider_test.go b/sdk/trace/provider_test.go
@@ -30,9 +30,9 @@ func (t *basicSpanProcesor) Shutdown(context.Context) error {
 	return nil
 }
 
-func (t *basicSpanProcesor) OnStart(s *export.SpanData) {}
-func (t *basicSpanProcesor) OnEnd(s *export.SpanData)   {}
-func (t *basicSpanProcesor) ForceFlush()                {}
+func (t *basicSpanProcesor) OnStart(parent context.Context, s *export.SpanData) {}
+func (t *basicSpanProcesor) OnEnd(s *export.SpanData)                           {}
+func (t *basicSpanProcesor) ForceFlush()                                        {}
 
 func TestShutdownTraceProvider(t *testing.T) {
 	stp := NewTracerProvider()

diff --git a/sdk/trace/simple_span_processor.go b/sdk/trace/simple_span_processor.go
@@ -39,7 +39,7 @@ func NewSimpleSpanProcessor(exporter export.SpanExporter) *SimpleSpanProcessor {
 }
 
 // OnStart method does nothing.
-func (ssp *SimpleSpanProcessor) OnStart(sd *export.SpanData) {
+func (ssp *SimpleSpanProcessor) OnStart(parent context.Context, sd *export.SpanData) {
 }
 
 // OnEnd method exports SpanData using associated export.

diff --git a/sdk/trace/span_processor.go b/sdk/trace/span_processor.go
@@ -26,7 +26,7 @@ type SpanProcessor interface {
 
 	// OnStart method is invoked when span is started. It is a synchronous call
 	// and hence should not block.
-	OnStart(sd *export.SpanData)
+	OnStart(parent context.Context, sd *export.SpanData)
 
 	// OnEnd method is invoked when span is finished. It is a synchronous call
 	// and hence should not block.

diff --git a/sdk/trace/span_processor_example_test.go b/sdk/trace/span_processor_example_test.go
@@ -34,7 +34,9 @@ type DurationFilter struct {
 	Max time.Duration
 }
 
-func (f DurationFilter) OnStart(sd *export.SpanData)        { f.Next.OnStart(sd) }
+func (f DurationFilter) OnStart(parent context.Context, sd *export.SpanData) {
+	f.Next.OnStart(parent, sd)
+}
 func (f DurationFilter) Shutdown(ctx context.Context) error { return f.Next.Shutdown(ctx) }
 func (f DurationFilter) ForceFlush()                        { f.Next.ForceFlush() }
 func (f DurationFilter) OnEnd(sd *export.SpanData) {
@@ -60,7 +62,9 @@ type InstrumentationBlacklist struct {
 	Blacklist map[string]bool
 }
 
-func (f InstrumentationBlacklist) OnStart(sd *export.SpanData)        { f.Next.OnStart(sd) }
+func (f InstrumentationBlacklist) OnStart(parent context.Context, sd *export.SpanData) {
+	f.Next.OnStart(parent, sd)
+}
 func (f InstrumentationBlacklist) Shutdown(ctx context.Context) error { return f.Next.Shutdown(ctx) }
 func (f InstrumentationBlacklist) ForceFlush()                        { f.Next.ForceFlush() }
 func (f InstrumentationBlacklist) OnEnd(sd *export.SpanData) {

diff --git a/sdk/trace/span_processor_test.go b/sdk/trace/span_processor_test.go
@@ -20,6 +20,7 @@ import (
 
 	"go.opentelemetry.io/otel/label"
 	export "go.opentelemetry.io/otel/sdk/export/trace"
+	"go.opentelemetry.io/otel/trace"
 )
 
 type testSpanProcessor struct {
@@ -29,12 +30,27 @@ type testSpanProcessor struct {
 	shutdownCount int
 }
 
-func (t *testSpanProcessor) OnStart(s *export.SpanData) {
-	kv := label.KeyValue{
-		Key:   "OnStart",
-		Value: label.StringValue(t.name),
+func (t *testSpanProcessor) OnStart(parent context.Context, s *export.SpanData) {
+	psc := trace.RemoteSpanContextFromContext(parent)
+	kv := []label.KeyValue{
+		{
+			Key:   "SpanProcessorName",
+			Value: label.StringValue(t.name),
+		},
+		// Store parent trace ID and span ID as attributes to be read later in
+		// tests so that we "do something" with the parent argument. Real
+		// SpanProcessor implementations will likely use the parent argument in
+		// a more meaningful way.
+		{
+			Key:   "ParentTraceID",
+			Value: label.StringValue(psc.TraceID.String()),
+		},
+		{
+			Key:   "ParentSpanID",
+			Value: label.StringValue(psc.SpanID.String()),
+		},
 	}
-	s.Attributes = append(s.Attributes, kv)
+	s.Attributes = append(s.Attributes, kv...)
 	t.spansStarted = append(t.spansStarted, s)
 }
 
@@ -65,8 +81,16 @@ func TestRegisterSpanProcessort(t *testing.T) {
 		tp.RegisterSpanProcessor(sp)
 	}
 
+	tid, _ := trace.TraceIDFromHex("01020304050607080102040810203040")
+	sid, _ := trace.SpanIDFromHex("0102040810203040")
+	parent := trace.SpanContext{
+		TraceID: tid,
+		SpanID:  sid,
+	}
+	ctx := trace.ContextWithRemoteSpanContext(context.Background(), parent)
+
 	tr := tp.Tracer("SpanProcessor")
-	_, span := tr.Start(context.Background(), "OnStart")
+	_, span := tr.Start(ctx, "OnStart")
 	span.End()
 	wantCount := 1
 
@@ -82,17 +106,29 @@ func TestRegisterSpanProcessort(t *testing.T) {
 
 		c := 0
 		for _, kv := range sp.spansStarted[0].Attributes {
-			if kv.Key != "OnStart" {
+			switch kv.Key {
+			case "SpanProcessorName":
+				gotValue := kv.Value.AsString()
+				if gotValue != spNames[c] {
+					t.Errorf("%s: attributes: got %s, want %s\n", name, gotValue, spNames[c])
+				}
+				c++
+			case "ParentTraceID":
+				gotValue := kv.Value.AsString()
+				if gotValue != parent.TraceID.String() {
+					t.Errorf("%s: attributes: got %s, want %s", name, gotValue, parent.TraceID)
+				}
+			case "ParentSpanID":
+				gotValue := kv.Value.AsString()
+				if gotValue != parent.SpanID.String() {
+					t.Errorf("%s: attributes: got %s, want %s", name, gotValue, parent.SpanID)
+				}
+			default:
 				continue
 			}
-			gotValue := kv.Value.AsString()
-			if gotValue != spNames[c] {
-				t.Errorf("%s: ordered attributes: got %s, want %s\n", name, gotValue, spNames[c])
-			}
-			c++
 		}
 		if c != len(spNames) {
-			t.Errorf("%s: expected attributes(OnStart): got %d, want %d\n", name, c, len(spNames))
+			t.Errorf("%s: expected attributes(SpanProcessorName): got %d, want %d\n", name, c, len(spNames))
 		}
 	}
 }

diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go
@@ -61,7 +61,7 @@ func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanO
 	if span.IsRecording() {
 		sps, _ := tr.provider.spanProcessors.Load().(spanProcessorStates)
 		for _, sp := range sps {
-			sp.sp.OnStart(span.data)
+			sp.sp.OnStart(ctx, span.data)
 		}
 	}