From 3fc6025071d5a6590501939dd3271b700ba01c14 Mon Sep 17 00:00:00 2001 From: Krzesimir Nowak Date: Mon, 26 Aug 2019 20:53:12 +0200 Subject: [PATCH] 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 | 3 + api/trace/current_test.go | 4 + api/trace/noop_span.go | 4 + .../exporter/observer/eventtype_string.go | 5 +- .../streaming/exporter/observer/observer.go | 3 +- .../exporter/reader/format/format.go | 4 + .../streaming/exporter/reader/reader.go | 5 + experimental/streaming/sdk/span.go | 7 ++ sdk/trace/span.go | 93 ++++++++++++++----- sdk/trace/trace_test.go | 81 +++++++++++++++- 10 files changed, 180 insertions(+), 29 deletions(-) diff --git a/api/trace/api.go b/api/trace/api.go index f9bfea666..f6edb5823 100644 --- a/api/trace/api.go +++ b/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) diff --git a/api/trace/current_test.go b/api/trace/current_test.go index 8d225e6e8..c8937e858 100644 --- a/api/trace/current_test.go +++ b/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) { } diff --git a/api/trace/noop_span.go b/api/trace/noop_span.go index f15fe2726..233ca6006 100644 --- a/api/trace/noop_span.go +++ b/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) { +} diff --git a/experimental/streaming/exporter/observer/eventtype_string.go b/experimental/streaming/exporter/observer/eventtype_string.go index 35d00d64c..8ccd16ccc 100644 --- a/experimental/streaming/exporter/observer/eventtype_string.go +++ b/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) { diff --git a/experimental/streaming/exporter/observer/observer.go b/experimental/streaming/exporter/observer/observer.go index 26248656e..e56fc935d 100644 --- a/experimental/streaming/exporter/observer/observer.go +++ b/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 ( diff --git a/experimental/streaming/exporter/reader/format/format.go b/experimental/streaming/exporter/reader/format/format.go index c73ebf5cd..97d4aa337 100644 --- a/experimental/streaming/exporter/reader/format/format.go +++ b/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)) } diff --git a/experimental/streaming/exporter/reader/reader.go b/experimental/streaming/exporter/reader/reader.go index c353e9eca..03732e6bc 100644 --- a/experimental/streaming/exporter/reader/reader.go +++ b/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)) } diff --git a/experimental/streaming/sdk/span.go b/experimental/streaming/sdk/span.go index 9dc155115..89c91f7e3 100644 --- a/experimental/streaming/sdk/span.go +++ b/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, + }) +} diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 5ad85081c..f3ae8d621 100644 --- a/sdk/trace/span.go +++ b/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 - - // TODO: [rghetia] fix sampler - //if !hasParent || remoteParent || o.Sampler != nil { - if noParent || 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. - //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 - } + data := samplingData{ + noParent: noParent, + remoteParent: remoteParent, + parent: parent, + name: name, + cfg: cfg, + span: span, } + 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 + } + } +} diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index ce6997001..e06a57033 100644 --- a/sdk/trace/trace_test.go +++ b/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. -// remote span context contains traceoption with sampled bit set. This allows -// the span to be automatically sampled. +// startSpan starts a span with a name "span0". See startNamedSpan for +// details. 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(), )