Implement immutable SpanContext (#27)

Per opentracing/opentracing.io#106

context.go

@@ -25,9 +25,6 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
-	"sync"
-
-	"github.com/opentracing/opentracing-go"
 )
 
 const (
@@ -38,12 +35,12 @@ const (
 var (
 	errEmptyTracerStateString     = errors.New("Cannot convert empty string to tracer state")
 	errMalformedTracerStateString = errors.New("String does not match tracer state format")
+
+	emptyContext = SpanContext{}
 )
 
 // SpanContext represents propagated span identity and state
 type SpanContext struct {
-	sync.RWMutex
-
 	// traceID represents globally unique ID of the trace.
 	// Usually generated as a random number.
 	traceID uint64
@@ -59,34 +56,12 @@ type SpanContext struct {
 	// flags is a bitmap containing such bits as 'sampled' and 'debug'.
 	flags byte
 
-	// Distributed Context baggage
+	// Distributed Context baggage. The is a snapshot in time.
 	baggage map[string]string
 }
 
-// SetBaggageItem implements SetBaggageItem() of opentracing.SpanContext
-func (c *SpanContext) SetBaggageItem(key, value string) opentracing.SpanContext {
-	key = normalizeBaggageKey(key)
-	c.Lock()
-	defer c.Unlock()
-	if c.baggage == nil {
-		c.baggage = make(map[string]string)
-	}
-	c.baggage[key] = value
-	return c
-}
-
-// BaggageItem implements BaggageItem() of opentracing.SpanContext
-func (c *SpanContext) BaggageItem(key string) string {
-	key = normalizeBaggageKey(key)
-	c.RLock()
-	defer c.RUnlock()
-	return c.baggage[key]
-}
-
 // ForeachBaggageItem implements ForeachBaggageItem() of opentracing.SpanContext
-func (c *SpanContext) ForeachBaggageItem(handler func(k, v string) bool) {
-	c.RLock()
-	defer c.RUnlock()
+func (c SpanContext) ForeachBaggageItem(handler func(k, v string) bool) {
 	for k, v := range c.baggage {
 		if !handler(k, v) {
 			break
@@ -96,80 +71,74 @@ func (c *SpanContext) ForeachBaggageItem(handler func(k, v string) bool) {
 
 // IsSampled returns whether this trace was chosen for permanent storage
 // by the sampling mechanism of the tracer.
-func (c *SpanContext) IsSampled() bool {
+func (c SpanContext) IsSampled() bool {
 	return (c.flags & flagSampled) == flagSampled
 }
 
-func (c *SpanContext) String() string {
-	c.RLock()
-	defer c.RUnlock()
+func (c SpanContext) String() string {
 	return fmt.Sprintf("%x:%x:%x:%x", c.traceID, c.spanID, c.parentID, c.flags)
 }
 
 // ContextFromString reconstructs the Context encoded in a string
-func ContextFromString(value string) (*SpanContext, error) {
-	var context = new(SpanContext)
+func ContextFromString(value string) (SpanContext, error) {
+	var context SpanContext
 	if value == "" {
-		return nil, errEmptyTracerStateString
+		return emptyContext, errEmptyTracerStateString
 	}
 	parts := strings.Split(value, ":")
 	if len(parts) != 4 {
-		return nil, errMalformedTracerStateString
+		return emptyContext, errMalformedTracerStateString
 	}
 	var err error
 	if context.traceID, err = strconv.ParseUint(parts[0], 16, 64); err != nil {
-		return nil, err
+		return emptyContext, err
 	}
 	if context.spanID, err = strconv.ParseUint(parts[1], 16, 64); err != nil {
-		return nil, err
+		return emptyContext, err
 	}
 	if context.parentID, err = strconv.ParseUint(parts[2], 16, 64); err != nil {
-		return nil, err
+		return emptyContext, err
 	}
 	flags, err := strconv.ParseUint(parts[3], 10, 8)
 	if err != nil {
-		return nil, err
+		return emptyContext, err
 	}
 	context.flags = byte(flags)
 	return context, nil
 }
 
 // TraceID implements TraceID() of SpanID
-func (c *SpanContext) TraceID() uint64 {
+func (c SpanContext) TraceID() uint64 {
 	return c.traceID
 }
 
 // SpanID implements SpanID() of SpanID
-func (c *SpanContext) SpanID() uint64 {
+func (c SpanContext) SpanID() uint64 {
 	return c.spanID
 }
 
 // ParentID implements ParentID() of SpanID
-func (c *SpanContext) ParentID() uint64 {
+func (c SpanContext) ParentID() uint64 {
 	return c.parentID
 }
 
 // NewSpanContext creates a new instance of SpanContext
-func NewSpanContext(traceID, spanID, parentID uint64, sampled bool) *SpanContext {
+func NewSpanContext(traceID, spanID, parentID uint64, sampled bool, baggage map[string]string) SpanContext {
 	flags := byte(0)
 	if sampled {
 		flags = flagSampled
 	}
-	return &SpanContext{
+	return SpanContext{
 		traceID:  traceID,
 		spanID:   spanID,
 		parentID: parentID,
-		flags:    flags}
+		flags:    flags,
+		baggage:  baggage}
 }
 
 // CopyFrom copies data from ctx into this context, including span identity and baggage.
+// TODO This is only used by interop.go. Remove once TChannel Go supports OpenTracing.
 func (c *SpanContext) CopyFrom(ctx *SpanContext) {
-	c.Lock()
-	defer c.Unlock()
-
-	ctx.RLock()
-	defer ctx.RUnlock()
-
 	c.traceID = ctx.traceID
 	c.spanID = ctx.spanID
 	c.parentID = ctx.parentID
@@ -183,3 +152,19 @@ func (c *SpanContext) CopyFrom(ctx *SpanContext) {
 		c.baggage = nil
 	}
 }
+
+// WithBaggageItem creates a new context with an extra baggage item.
+func (c SpanContext) WithBaggageItem(key, value string) SpanContext {
+	var newBaggage map[string]string
+	if c.baggage == nil {
+		newBaggage = map[string]string{key: value}
+	} else {
+		newBaggage = make(map[string]string, len(c.baggage)+1)
+		for k, v := range c.baggage {
+			newBaggage[k] = v
+		}
+		newBaggage[key] = value
+	}
+	// Use positional parameters so the compiler will help catch new fields.
+	return SpanContext{c.traceID, c.spanID, c.parentID, c.flags, newBaggage}
+}

context_test.go

@@ -27,9 +27,17 @@ func TestContextFromString(t *testing.T) {
 	assert.EqualValues(t, 1, ctx.spanID)
 	assert.EqualValues(t, 1, ctx.parentID)
 	assert.EqualValues(t, 1, ctx.flags)
-	ctx = NewSpanContext(1, 1, 1, true)
+	ctx = NewSpanContext(1, 1, 1, true, nil)
 	assert.EqualValues(t, 1, ctx.traceID)
 	assert.EqualValues(t, 1, ctx.spanID)
 	assert.EqualValues(t, 1, ctx.parentID)
 	assert.EqualValues(t, 1, ctx.flags)
 }
+
+func TestSpanContext_WithBaggageItem(t *testing.T) {
+	var ctx SpanContext
+	ctx = ctx.WithBaggageItem("some-KEY", "Some-Value")
+	assert.Equal(t, map[string]string{"some-KEY": "Some-Value"}, ctx.baggage)
+	ctx = ctx.WithBaggageItem("some-KEY", "Some-Other-Value")
+	assert.Equal(t, map[string]string{"some-KEY": "Some-Other-Value"}, ctx.baggage)
+}

crossdock/server/server_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/opentracing/opentracing-go"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/net/context"
 
@@ -46,3 +48,25 @@ func TestServerJSON(t *testing.T) {
 	require.NoError(t, err)
 	fmt.Printf("response=%+v\n", &result)
 }
+
+func TestObserveSpan(t *testing.T) {
+	tracer, tCloser := jaeger.NewTracer(
+		"crossdock",
+		jaeger.NewConstSampler(true),
+		jaeger.NewNullReporter())
+	defer tCloser.Close()
+
+	_, err := observeSpan(context.Background(), tracer)
+	assert.Error(t, err)
+
+	span := tracer.StartSpan("hi")
+	span.SetBaggageItem(BaggageKey, "xyz")
+	ctx := opentracing.ContextWithSpan(context.Background(), span)
+
+	s, err := observeSpan(ctx, tracer)
+	assert.NoError(t, err)
+	assert.True(t, s.Sampled)
+	traceID := fmt.Sprintf("%x", span.Context().(jaeger.SpanContext).TraceID())
+	assert.Equal(t, traceID, s.TraceId)
+	assert.Equal(t, "xyz", s.Baggage)
+}

crossdock/server/tchannel.go

@@ -66,7 +66,7 @@ func (s *Server) StartTrace(ctx thrift.Context, request *tracetest.StartTraceReq
 
 // JoinTrace implements JoinTrace() of TChanTracedService
 func (s *Server) JoinTrace(ctx thrift.Context, request *tracetest.JoinTraceRequest) (*tracetest.TraceResponse, error) {
-	log.Printf("tchannel server handling JoinTrace")
+	log.Printf("tchannel server handling JoinTrace request %+v", request)
 	ctx2 := setupOpenTracingContext(s.Tracer, ctx, "tchannel", ctx.Headers())
 	return s.prepareResponse(ctx2, request.ServerRole, request.Downstream)
 }
@@ -113,18 +113,22 @@ func WrapContext(ctx context.Context, timeout time.Duration) (thrift.Context, co
 // Once TChannel supports OpenTracing API directly, this bridging will not be required.
 func convertOpenTracingSpan(ctx context.Context, builder *tchannel.ContextBuilder) {
 	span := opentracing.SpanFromContext(ctx)
+	log.Printf("convertOpenTracingSpan converting span %+v", span)
 	if span == nil {
 		return
 	}
 	sc := new(jaeger.SpanContext)
 	if err := span.Tracer().Inject(span.Context(), jaeger.SpanContextFormat, sc); err != nil {
+		log.Printf("convertOpenTracingSpan ran into error converting span: %+v", err)
 		return
 	}
 	builder.SetExternalSpan(sc.TraceID(), sc.SpanID(), sc.ParentID(), sc.IsSampled())
 	sc.ForeachBaggageItem(func(k, v string) bool {
+		log.Printf("convertOpenTracingSpan is adding header %s=%s", k, v)
 		builder.AddHeader(k, v)
 		return true
 	})
+	log.Printf("convertOpenTracingSpan is returning tBuilder: %+v", builder)
 }
 
 // setupOpenTracingContext extracts a TChannel tracing Span from the context, converts
@@ -142,17 +146,16 @@ func convertOpenTracingSpan(ctx context.Context, builder *tchannel.ContextBuilde
 //
 func setupOpenTracingContext(tracer opentracing.Tracer, ctx context.Context, method string, headers map[string]string) thrift.Context {
 	tSpan := tchannel.CurrentSpan(ctx)
+	log.Printf("setupOpenTracingContext, tSpan=%+v, headers=%+v", tSpan, headers)
 	if tSpan != nil {
 		// populate a fake carrier and try to create OpenTracing Span
 		sc := jaeger.NewSpanContext(
-			tSpan.TraceID(), tSpan.SpanID(), tSpan.ParentID(), tSpan.TracingEnabled())
-		for k, v := range headers {
-			sc.SetBaggageItem(k, v)
-		}
+			tSpan.TraceID(), tSpan.SpanID(), tSpan.ParentID(), tSpan.TracingEnabled(), headers)
 		if tracer == nil {
 			tracer = opentracing.GlobalTracer()
 		}
 		span := tracer.StartSpan(method, ext.RPCServerOption(sc))
+		log.Printf("setupOpenTracingContext started new span %+v", span)
 		ctx = opentracing.ContextWithSpan(ctx, span)
 	}
 	return thrift.WithHeaders(ctx, headers)

crossdock/server/trace.go

@@ -38,7 +38,7 @@ func (s *Server) doStartTrace(req *tracetest.StartTraceRequest) (*tracetest.Trac
 	if req.Sampled {
 		ext.SamplingPriority.Set(span, 1)
 	}
-	span.Context().SetBaggageItem(BaggageKey, req.Baggage)
+	span.SetBaggageItem(BaggageKey, req.Baggage)
 	defer span.Finish()
 
 	ctx := opentracing.ContextWithSpan(context.Background(), span)
@@ -98,10 +98,10 @@ func observeSpan(ctx context.Context, tracer opentracing.Tracer) (*tracetest.Obs
 	if span == nil {
 		return nil, errNoSpanObserved
 	}
-	sc := span.Context().(*jaeger.SpanContext)
+	sc := span.Context().(jaeger.SpanContext)
 	observedSpan := tracetest.NewObservedSpan()
 	observedSpan.TraceId = fmt.Sprintf("%x", sc.TraceID())
 	observedSpan.Sampled = sc.IsSampled()
-	observedSpan.Baggage = sc.BaggageItem(BaggageKey)
+	observedSpan.Baggage = span.BaggageItem(BaggageKey)
 	return observedSpan, nil
 }

glide.yaml

@@ -5,7 +5,7 @@ import:
   subpackages:
   - lib/go/thrift
 - package: github.com/opentracing/opentracing-go
-  version: ce84834b482d236a829419f99eb78866b58751fd
+  version: 855519783f479520497c6b3445611b05fc42f009
   subpackages:
   - ext
 - package: golang.org/x/net

interop.go

@@ -38,24 +38,24 @@ type jaegerTraceContextPropagator struct {
 }
 
 func (p *jaegerTraceContextPropagator) Inject(
-	ctx *SpanContext,
+	ctx SpanContext,
 	abstractCarrier interface{},
 ) error {
 	carrier, ok := abstractCarrier.(*SpanContext)
 	if !ok {
 		return opentracing.ErrInvalidCarrier
 	}
 
-	carrier.CopyFrom(ctx)
+	carrier.CopyFrom(&ctx)
 	return nil
 }
 
-func (p *jaegerTraceContextPropagator) Extract(abstractCarrier interface{}) (*SpanContext, error) {
+func (p *jaegerTraceContextPropagator) Extract(abstractCarrier interface{}) (SpanContext, error) {
 	carrier, ok := abstractCarrier.(*SpanContext)
 	if !ok {
-		return nil, opentracing.ErrInvalidCarrier
+		return emptyContext, opentracing.ErrInvalidCarrier
 	}
 	ctx := new(SpanContext)
 	ctx.CopyFrom(carrier)
-	return ctx, nil
+	return *ctx, nil
 }