From 02cd1235426d16656120f78cf57c859c77072456 Mon Sep 17 00:00:00 2001 From: Paul Osman Date: Fri, 9 Oct 2020 17:48:34 -0500 Subject: [PATCH] Call sampler on local child spans. (#1233) * Call sampler on local child spans. * Update CHANGELOG.md * Adding a test for calling the sampler on local child spans * Add clarifying comment to test --- CHANGELOG.md | 1 + sdk/trace/span.go | 47 +++++++++--------------- sdk/trace/trace_test.go | 80 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fda30d896b..923d2908f9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `EmptySpanContext` is removed. - Move the `go.opentelemetry.io/otel/api/trace/tracetest` package into `go.opentelemetry.io/otel/oteltest`. (#1229) - OTLP Exporter supports OTLP v0.5.0. (#1230) +- The Sampler is now called on local child spans. (#1233) ## [0.13.0] - 2020-10-08 diff --git a/sdk/trace/span.go b/sdk/trace/span.go index ac8c9a413c1..a4748ebe0ad 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -397,36 +397,21 @@ type samplingData struct { } func makeSamplingDecision(data samplingData) SamplingResult { - 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 - // TraceFlags. - // - // 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.ShouldSample(SamplingParameters{ - ParentContext: data.parent, - TraceID: spanContext.TraceID, - Name: data.name, - HasRemoteParent: data.remoteParent, - Kind: data.kind, - Attributes: data.attributes, - Links: data.links, - }) - if sampled.Decision == RecordAndSample { - spanContext.TraceFlags |= otel.FlagsSampled - } else { - spanContext.TraceFlags &^= otel.FlagsSampled - } - return sampled - } - if data.parent.TraceFlags&otel.FlagsSampled != 0 { - return SamplingResult{Decision: RecordAndSample} + sampler := data.cfg.DefaultSampler + spanContext := &data.span.spanContext + sampled := sampler.ShouldSample(SamplingParameters{ + ParentContext: data.parent, + TraceID: spanContext.TraceID, + Name: data.name, + HasRemoteParent: data.remoteParent, + Kind: data.kind, + Attributes: data.attributes, + Links: data.links, + }) + if sampled.Decision == RecordAndSample { + spanContext.TraceFlags |= otel.FlagsSampled + } else { + spanContext.TraceFlags &^= otel.FlagsSampled } - return SamplingResult{Decision: Drop} + return sampled } diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index d4c4b09585e..f350c37df2b 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -399,6 +399,72 @@ func TestSetSpanAttributes(t *testing.T) { } } +// Test that the sampler is called for local child spans. This is verified by checking +// that the attributes set in the sampler are set on the child span. +func TestSamplerAttributesLocalChildSpan(t *testing.T) { + sampler := &testSampler{prefix: "span", t: t} + te := NewTestExporter() + tp := NewTracerProvider(WithConfig(Config{DefaultSampler: sampler}), WithSyncer(te)) + + ctx := context.Background() + ctx, span := startLocalSpan(tp, ctx, "SpanOne", "span0") + _, spanTwo := startLocalSpan(tp, ctx, "SpanTwo", "span1") + + spanTwo.End() + span.End() + + got := te.Spans() + + // endSpan expects only a single span in the test exporter, so manually clear the + // fields that can't be tested for easily (times, span and trace ids). + pid := got[0].SpanContext.SpanID + got[0].SpanContext.TraceID = tid + got[0].ParentSpanID = sid + + checkTime(&got[0].StartTime) + checkTime(&got[0].EndTime) + + got[1].SpanContext.SpanID = otel.SpanID{} + got[1].SpanContext.TraceID = tid + got[1].ParentSpanID = pid + got[0].SpanContext.SpanID = otel.SpanID{} + + checkTime(&got[1].StartTime) + checkTime(&got[1].EndTime) + + want := []*export.SpanData{ + { + SpanContext: otel.SpanContext{ + TraceID: tid, + TraceFlags: 0x1, + }, + ParentSpanID: sid, + Name: "span1", + Attributes: []label.KeyValue{label.Int("callCount", 2)}, + SpanKind: otel.SpanKindInternal, + HasRemoteParent: false, + InstrumentationLibrary: instrumentation.Library{Name: "SpanTwo"}, + }, + { + SpanContext: otel.SpanContext{ + TraceID: tid, + TraceFlags: 0x1, + }, + ParentSpanID: pid, + Name: "span0", + Attributes: []label.KeyValue{label.Int("callCount", 1)}, + SpanKind: otel.SpanKindInternal, + HasRemoteParent: false, + ChildSpanCount: 1, + InstrumentationLibrary: instrumentation.Library{Name: "SpanOne"}, + }, + } + + if diff := cmpDiff(got, want); diff != "" { + t.Errorf("SetSpanAttributesLocalChildSpan: -got +want %s", diff) + } +} + func TestSetSpanAttributesOverLimit(t *testing.T) { te := NewTestExporter() cfg := Config{MaxAttributesPerSpan: 2} @@ -730,6 +796,20 @@ func startNamedSpan(tp *TracerProvider, trName, name string, args ...otel.SpanOp return span } +// startLocalSpan is a test utility func that starts a span with a +// passed name and with the passed context. The context is returned +// along with the span so this parent can be used to create child +// spans. +func startLocalSpan(tp *TracerProvider, ctx context.Context, trName, name string, args ...otel.SpanOption) (context.Context, otel.Span) { + args = append(args, otel.WithRecord()) + ctx, span := tp.Tracer(trName).Start( + ctx, + name, + args..., + ) + return ctx, span +} + // endSpan is a test utility function that ends the span in the context and // returns the exported export.SpanData. // It requires that span be sampled using one of these methods