Allow setting the name of the span after starting it (#102)

* Allow setting the name of the span after starting it

* Add test for setting the name of the span

api/trace/api.go

@@ -74,6 +74,9 @@ type Span interface {
 	// even after span is finished.
 	SetStatus(codes.Code)
 
+	// SetName sets the name of the span.
+	SetName(name string)
+
 	// Set span attributes
 	SetAttribute(core.KeyValue)
 	SetAttributes(...core.KeyValue)

api/trace/current_test.go

@@ -72,6 +72,10 @@ func (mockSpan) IsRecordingEvents() bool {
 func (mockSpan) SetStatus(status codes.Code) {
 }
 
+// SetName does nothing.
+func (mockSpan) SetName(name string) {
+}
+
 // SetError does nothing.
 func (mockSpan) SetError(v bool) {
 }

api/trace/noop_span.go

@@ -79,3 +79,7 @@ func (NoopSpan) AddEvent(ctx context.Context, event event.Event) {
 // Event does nothing.
 func (NoopSpan) Event(ctx context.Context, msg string, attrs ...core.KeyValue) {
 }
+
+// SetName does nothing.
+func (NoopSpan) SetName(name string) {
+}

experimental/streaming/exporter/observer/eventtype_string.go

@@ -19,11 +19,12 @@ func _() {
 	_ = x[MODIFY_ATTR-8]
 	_ = x[RECORD_STATS-9]
 	_ = x[SET_STATUS-10]
+	_ = x[SET_NAME-11]
 }
 
-const _EventType_name = "INVALIDSTART_SPANFINISH_SPANADD_EVENTADD_EVENTFNEW_SCOPENEW_MEASURENEW_METRICMODIFY_ATTRRECORD_STATSSET_STATUS"
+const _EventType_name = "INVALIDSTART_SPANFINISH_SPANADD_EVENTADD_EVENTFNEW_SCOPENEW_MEASURENEW_METRICMODIFY_ATTRRECORD_STATSSET_STATUSSET_NAME"
 
-var _EventType_index = [...]uint8{0, 7, 17, 28, 37, 47, 56, 67, 77, 88, 100, 110}
+var _EventType_index = [...]uint8{0, 7, 17, 28, 37, 47, 56, 67, 77, 88, 100, 110, 118}
 
 func (i EventType) String() string {
 	if i < 0 || i >= EventType(len(_EventType_index)-1) {

experimental/streaming/exporter/observer/observer.go

@@ -56,7 +56,7 @@ type Event struct {
 	Status     codes.Code      // SET_STATUS
 
 	// Values
-	String  string // START_SPAN, EVENT, ...
+	String  string // START_SPAN, EVENT, SET_NAME, ...
 	Float64 float64
 	Parent  ScopeID // START_SPAN
 	Stats   []stats.Measurement
@@ -83,6 +83,7 @@ const (
 	MODIFY_ATTR
 	RECORD_STATS
 	SET_STATUS
+	SET_NAME
 )
 
 var (

experimental/streaming/exporter/reader/format/format.go

@@ -107,6 +107,10 @@ func AppendEvent(buf *strings.Builder, data reader.Event) {
 		buf.WriteString("set status ")
 		buf.WriteString(data.Status.String())
 
+	case reader.SET_NAME:
+		buf.WriteString("set name ")
+		buf.WriteString(data.Name)
+
 	default:
 		buf.WriteString(fmt.Sprintf("WAT? %d", data.Type))
 	}

experimental/streaming/exporter/reader/reader.go

@@ -102,6 +102,7 @@ const (
 	MODIFY_ATTR
 	RECORD_STATS
 	SET_STATUS
+	SET_NAME
 )
 
 // NewReaderObserver returns an implementation that computes the
@@ -287,6 +288,10 @@ func (ro *readerObserver) orderedObserve(event observer.Event) {
 			read.SpanContext = span.spanContext
 		}
 
+	case observer.SET_NAME:
+		read.Type = SET_NAME
+		read.Name = event.String
+
 	default:
 		panic(fmt.Sprint("Unhandled case: ", event.Type))
 	}

experimental/streaming/sdk/span.go

@@ -120,3 +120,10 @@ func (sp *span) Event(ctx context.Context, msg string, attrs ...core.KeyValue) {
 		Context:    ctx,
 	})
 }
+
+func (sp *span) SetName(name string) {
+	observer.Record(observer.Event{
+		Type:   observer.SET_NAME,
+		String: name,
+	})
+}

sdk/trace/span.go

@@ -164,6 +164,33 @@ func (s *span) Event(ctx context.Context, msg string, attrs ...core.KeyValue) {
 	s.mu.Unlock()
 }
 
+func (s *span) SetName(name string) {
+	if s.data == nil {
+		// TODO: now what?
+		return
+	}
+	s.data.Name = name
+	// SAMPLING
+	noParent := s.data.ParentSpanID == 0
+	var ctx core.SpanContext
+	if noParent {
+		ctx = core.EmptySpanContext()
+	} else {
+		// FIXME: Where do we get the parent context from?
+		// From SpanStore?
+		ctx = s.data.SpanContext
+	}
+	data := samplingData{
+		noParent:     noParent,
+		remoteParent: s.data.HasRemoteParent,
+		parent:       ctx,
+		name:         name,
+		cfg:          config.Load().(*Config),
+		span:         s,
+	}
+	makeSamplingDecision(data)
+}
+
 // makeSpanData produces a SpanData representing the current state of the span.
 // It requires that s.data is non-nil.
 func (s *span) makeSpanData() *SpanData {
@@ -231,29 +258,15 @@ func startSpanInternal(name string, parent core.SpanContext, remoteParent bool,
 		noParent = true
 	}
 	span.spanContext.SpanID = cfg.IDGenerator.NewSpanID()
-	sampler := cfg.DefaultSampler
+	data := samplingData{
-
+		noParent:     noParent,
-	// TODO: [rghetia] fix sampler
+		remoteParent: remoteParent,
-	//if !hasParent || remoteParent || o.Sampler != nil {
+		parent:       parent,
-	if noParent || remoteParent {
+		name:         name,
-		// If this span is the child of a local span and no Sampler is set in the
+		cfg:          cfg,
-		// options, keep the parent's TraceOptions.
+		span:         span,
-		//
-		// Otherwise, consult the Sampler in the options if it is non-nil, otherwise
-		// the default sampler.
-		//if o.Sampler != nil {
-		//	sampler = o.Sampler
-		//}
-		sampled := sampler(SamplingParameters{
-			ParentContext:   parent,
-			TraceID:         span.spanContext.TraceID,
-			SpanID:          span.spanContext.SpanID,
-			Name:            name,
-			HasRemoteParent: remoteParent}).Sample
-		if sampled {
-			span.spanContext.TraceOptions = core.TraceOptionSampled
-		}
 	}
+	makeSamplingDecision(data)
 
 	// TODO: [rghetia] restore when spanstore is added.
 	// if !internal.LocalSpanStoreEnabled && !span.spanContext.IsSampled() && !o.RecordEvent {
@@ -287,3 +300,39 @@ func startSpanInternal(name string, parent core.SpanContext, remoteParent bool,
 
 	return span
 }
+
+type samplingData struct {
+	noParent     bool
+	remoteParent bool
+	parent       core.SpanContext
+	name         string
+	cfg          *Config
+	span         *span
+}
+
+func makeSamplingDecision(data samplingData) {
+	if data.noParent || data.remoteParent {
+		// If this span is the child of a local span and no
+		// Sampler is set in the options, keep the parent's
+		// TraceOptions.
+		//
+		// Otherwise, consult the Sampler in the options if it
+		// is non-nil, otherwise the default sampler.
+		sampler := data.cfg.DefaultSampler
+		//if o.Sampler != nil {
+		//	sampler = o.Sampler
+		//}
+		spanContext := &data.span.spanContext
+		sampled := sampler(SamplingParameters{
+			ParentContext:   data.parent,
+			TraceID:         spanContext.TraceID,
+			SpanID:          spanContext.SpanID,
+			Name:            data.name,
+			HasRemoteParent: data.remoteParent}).Sample
+		if sampled {
+			spanContext.TraceOptions |= core.TraceOptionSampled
+		} else {
+			spanContext.TraceOptions &^= core.TraceOptionSampled
+		}
+	}
+}

sdk/trace/trace_test.go

@@ -17,6 +17,7 @@ package trace
 import (
 	"context"
 	"fmt"
+	"strings"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -36,6 +37,10 @@ var (
 
 func init() {
 	Register()
+	setupDefaultSamplerConfig()
+}
+
+func setupDefaultSamplerConfig() {
 	// no random sampling, but sample children of sampled spans.
 	ApplyConfig(Config{DefaultSampler: ProbabilitySampler(0)})
 }
@@ -56,6 +61,67 @@ func TestStartSpan(t *testing.T) {
 	}
 }
 
+func TestSetName(t *testing.T) {
+	samplerIsCalled := false
+	fooSampler := Sampler(func(p SamplingParameters) SamplingDecision {
+		samplerIsCalled = true
+		t.Logf("called sampler for name %q", p.Name)
+		return SamplingDecision{Sample: strings.HasPrefix(p.Name, "foo")}
+	})
+	ApplyConfig(Config{DefaultSampler: fooSampler})
+	defer setupDefaultSamplerConfig()
+	type testCase struct {
+		name          string
+		newName       string
+		sampledBefore bool
+		sampledAfter  bool
+	}
+	for idx, tt := range []testCase{
+		{ // 0
+			name:          "foobar",
+			newName:       "foobaz",
+			sampledBefore: true,
+			sampledAfter:  true,
+		},
+		{ // 1
+			name:          "foobar",
+			newName:       "barbaz",
+			sampledBefore: true,
+			sampledAfter:  false,
+		},
+		{ // 2
+			name:          "barbar",
+			newName:       "barbaz",
+			sampledBefore: false,
+			sampledAfter:  false,
+		},
+		{ // 3
+			name:          "barbar",
+			newName:       "foobar",
+			sampledBefore: false,
+			sampledAfter:  true,
+		},
+	} {
+		span := startNamedSpan(tt.name)
+		if !samplerIsCalled {
+			t.Errorf("%d: the sampler was not even called during span creation", idx)
+		}
+		samplerIsCalled = false
+		if gotSampledBefore := span.SpanContext().IsSampled(); tt.sampledBefore != gotSampledBefore {
+			t.Errorf("%d: invalid sampling decision before rename, expected %v, got %v", idx, tt.sampledBefore, gotSampledBefore)
+		}
+		span.SetName(tt.newName)
+		if !samplerIsCalled {
+			t.Errorf("%d: the sampler was not even called during span rename", idx)
+		}
+		samplerIsCalled = false
+		if gotSampledAfter := span.SpanContext().IsSampled(); tt.sampledAfter != gotSampledAfter {
+			t.Errorf("%d: invalid sampling decision after rename, expected %v, got %v", idx, tt.sampledAfter, gotSampledAfter)
+		}
+		span.Finish()
+	}
+}
+
 func TestRecordingIsOff(t *testing.T) {
 	_, span := apitrace.GlobalTracer().Start(context.Background(), "StartSpan")
 	defer span.Finish()
@@ -335,13 +401,20 @@ func checkChild(p core.SpanContext, apiSpan apitrace.Span) error {
 	return nil
 }
 
-// startSpan is a test utility func that starts a span with ChildOf option.
+// startSpan starts a span with a name "span0". See startNamedSpan for
-// remote span context contains traceoption with sampled bit set. This allows
+// details.
-// the span to be automatically sampled.
 func startSpan() apitrace.Span {
+	return startNamedSpan("span0")
+}
+
+// startNamed Span is a test utility func that starts a span with a
+// passed name and with ChildOf option.  remote span context contains
+// traceoption with sampled bit set. This allows the span to be
+// automatically sampled.
+func startNamedSpan(name string) apitrace.Span {
 	_, span := apitrace.GlobalTracer().Start(
 		context.Background(),
-		"span0",
+		name,
 		apitrace.ChildOf(remoteSpanContext()),
 		apitrace.WithRecordEvents(),
 	)