diff --git a/CHANGELOG.md b/CHANGELOG.md index 226f05bf600..1dc67c69105 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The storage of a local or remote Span in a `context.Context` using its SpanContext is unified to store just the current Span. The Span's SpanContext can now self-identify as being remote or not. This means that `"go.opentelemetry.io/otel/trace".ContextWithRemoteSpanContext` will now overwrite any existing current Span, not just existing remote Spans, and make it the current Span in a `context.Context`. (#1731) +- The `ParentContext` field of the `"go.opentelemetry.io/otel/sdk/trace".SamplingParameters` is updated to hold a `context.Context` containing the parent span. + This changes it to make `SamplingParameters` conform with the OpenTelemetry specification. (TBD) ### Removed @@ -27,6 +29,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm Because of this `"go.opentelemetry.io/otel/trace".RemoteSpanContextFromContext` is removed as it is no longer needed. Instead, `"go.opentelemetry.io/otel/trace".SpanContextFromContex` can be used to return the current Span. If needed, that Span's `SpanContext.IsRemote()` can then be used to determine if it is remote or not. (#1731) +- The `HasRemoteParent` field of the `"go.opentelemetry.io/otel/sdk/trace".SamplingParameters` is removed. + This field is redundant to the information returned from the `Remote` method of the `SpanContext` held in the `ParentContext` field. (TBD) ## [0.19.0] - 2021-03-18 diff --git a/sdk/trace/sampling.go b/sdk/trace/sampling.go index 30063886964..86fc3108dc3 100644 --- a/sdk/trace/sampling.go +++ b/sdk/trace/sampling.go @@ -15,6 +15,7 @@ package trace // import "go.opentelemetry.io/otel/sdk/trace" import ( + "context" "encoding/binary" "fmt" @@ -30,13 +31,12 @@ type Sampler interface { // SamplingParameters contains the values passed to a Sampler. type SamplingParameters struct { - ParentContext trace.SpanContext - TraceID trace.TraceID - Name string - HasRemoteParent bool - Kind trace.SpanKind - Attributes []attribute.KeyValue - Links []trace.Link + ParentContext context.Context + TraceID trace.TraceID + Name string + Kind trace.SpanKind + Attributes []attribute.KeyValue + Links []trace.Link } // SamplingDecision indicates whether a span is dropped, recorded and/or sampled. @@ -69,16 +69,17 @@ type traceIDRatioSampler struct { } func (ts traceIDRatioSampler) ShouldSample(p SamplingParameters) SamplingResult { + psc := trace.SpanContextFromContext(p.ParentContext) x := binary.BigEndian.Uint64(p.TraceID[0:8]) >> 1 if x < ts.traceIDUpperBound { return SamplingResult{ Decision: RecordAndSample, - Tracestate: p.ParentContext.TraceState(), + Tracestate: psc.TraceState(), } } return SamplingResult{ Decision: Drop, - Tracestate: p.ParentContext.TraceState(), + Tracestate: psc.TraceState(), } } @@ -111,7 +112,7 @@ type alwaysOnSampler struct{} func (as alwaysOnSampler) ShouldSample(p SamplingParameters) SamplingResult { return SamplingResult{ Decision: RecordAndSample, - Tracestate: p.ParentContext.TraceState(), + Tracestate: trace.SpanContextFromContext(p.ParentContext).TraceState(), } } @@ -132,7 +133,7 @@ type alwaysOffSampler struct{} func (as alwaysOffSampler) ShouldSample(p SamplingParameters) SamplingResult { return SamplingResult{ Decision: Drop, - Tracestate: p.ParentContext.TraceState(), + Tracestate: trace.SpanContextFromContext(p.ParentContext).TraceState(), } } @@ -260,15 +261,16 @@ func (o localParentNotSampledOption) Apply(config *config) { func (localParentNotSampledOption) private() {} func (pb parentBased) ShouldSample(p SamplingParameters) SamplingResult { - if p.ParentContext.IsValid() { - if p.HasRemoteParent { - if p.ParentContext.IsSampled() { + psc := trace.SpanContextFromContext(p.ParentContext) + if psc.IsValid() { + if psc.IsRemote() { + if psc.IsSampled() { return pb.config.remoteParentSampled.ShouldSample(p) } return pb.config.remoteParentNotSampled.ShouldSample(p) } - if p.ParentContext.IsSampled() { + if psc.IsSampled() { return pb.config.localParentSampled.ShouldSample(p) } return pb.config.localParentNotSampled.ShouldSample(p) diff --git a/sdk/trace/sampling_test.go b/sdk/trace/sampling_test.go index 61b38eb78f3..98bb4f42150 100644 --- a/sdk/trace/sampling_test.go +++ b/sdk/trace/sampling_test.go @@ -20,6 +20,7 @@ import ( "math/rand" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/attribute" @@ -30,11 +31,14 @@ func TestParentBasedDefaultLocalParentSampled(t *testing.T) { sampler := ParentBased(AlwaysSample()) traceID, _ := trace.TraceIDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7") - parentCtx := trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: traceID, - SpanID: spanID, - TraceFlags: trace.FlagsSampled, - }) + parentCtx := trace.ContextWithSpanContext( + context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceID, + SpanID: spanID, + TraceFlags: trace.FlagsSampled, + }), + ) if sampler.ShouldSample(SamplingParameters{ParentContext: parentCtx}).Decision != RecordAndSample { t.Error("Sampling decision should be RecordAndSample") } @@ -44,10 +48,13 @@ func TestParentBasedDefaultLocalParentNotSampled(t *testing.T) { sampler := ParentBased(AlwaysSample()) traceID, _ := trace.TraceIDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7") - parentCtx := trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: traceID, - SpanID: spanID, - }) + parentCtx := trace.ContextWithSpanContext( + context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceID, + SpanID: spanID, + }), + ) if sampler.ShouldSample(SamplingParameters{ParentContext: parentCtx}).Decision != Drop { t.Error("Sampling decision should be Drop") } @@ -108,18 +115,20 @@ func TestParentBasedWithSamplerOptions(t *testing.T) { t.Run(tc.name, func(t *testing.T) { traceID, _ := trace.TraceIDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7") - parentCtx := trace.NewSpanContext(trace.SpanContextConfig{ + pscc := trace.SpanContextConfig{ TraceID: traceID, SpanID: spanID, - }) - + Remote: tc.isParentRemote, + } if tc.isParentSampled { - parentCtx = parentCtx.WithTraceFlags(trace.FlagsSampled) + pscc.TraceFlags = trace.FlagsSampled } - params := SamplingParameters{ParentContext: parentCtx} - if tc.isParentRemote { - params.HasRemoteParent = true + params := SamplingParameters{ + ParentContext: trace.ContextWithSpanContext( + context.Background(), + trace.NewSpanContext(pscc), + ), } sampler := ParentBased( @@ -127,16 +136,27 @@ func TestParentBasedWithSamplerOptions(t *testing.T) { tc.samplerOption, ) + var wantStr, gotStr string switch tc.expectedDecision { case RecordAndSample: - if sampler.ShouldSample(params).Decision != tc.expectedDecision { - t.Error("Sampling decision should be RecordAndSample") - } + wantStr = "RecordAndSample" + case Drop: + wantStr = "Drop" + default: + wantStr = "unknown" + } + + actualDecision := sampler.ShouldSample(params).Decision + switch actualDecision { + case RecordAndSample: + gotStr = "RecordAndSample" case Drop: - if sampler.ShouldSample(params).Decision != tc.expectedDecision { - t.Error("Sampling decision should be Drop") - } + gotStr = "Drop" + default: + gotStr = "unknown" } + + assert.Equalf(t, tc.expectedDecision, actualDecision, "want %s, got %s", wantStr, gotStr) }) } } @@ -225,10 +245,14 @@ func TestTracestateIsPassed(t *testing.T) { t.Error(err) } - parentCtx := trace.NewSpanContext(trace.SpanContextConfig{ - TraceState: traceState, - }) - params := SamplingParameters{ParentContext: parentCtx} + params := SamplingParameters{ + ParentContext: trace.ContextWithSpanContext( + context.Background(), + trace.NewSpanContext(trace.SpanContextConfig{ + TraceState: traceState, + }), + ), + } require.Equal(t, traceState, tc.sampler.ShouldSample(params).Tracestate, "TraceState is not equal") }) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index cbf37769b39..f9d9f4bbd44 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -544,13 +544,12 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, o *trace.Sp span.spanLimits = spanLimits samplingResult := provider.sampler.ShouldSample(SamplingParameters{ - ParentContext: psc, - TraceID: tid, - Name: name, - HasRemoteParent: psc.IsRemote(), - Kind: o.SpanKind, - Attributes: o.Attributes, - Links: o.Links, + ParentContext: ctx, + TraceID: tid, + Name: name, + Kind: o.SpanKind, + Attributes: o.Attributes, + Links: o.Links, }) scc := trace.SpanContextConfig{ diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 1bb05b417f7..e5eb7906099 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -1610,7 +1610,8 @@ func (s *stateSampler) ShouldSample(p SamplingParameters) SamplingResult { if strings.HasPrefix(p.Name, s.prefix) { decision = RecordAndSample } - return SamplingResult{Decision: decision, Tracestate: s.f(p.ParentContext.TraceState())} + ts := s.f(trace.SpanContextFromContext(p.ParentContext).TraceState()) + return SamplingResult{Decision: decision, Tracestate: ts} } func (s stateSampler) Description() string { diff --git a/trace/context.go b/trace/context.go index 0fd11d2e942..76f9a083c40 100644 --- a/trace/context.go +++ b/trace/context.go @@ -25,12 +25,20 @@ func ContextWithSpan(parent context.Context, span Span) context.Context { return context.WithValue(parent, currentSpanKey, span) } +// ContextWithSpanContext returns a copy of parent with sc as the current +// Span. The Span implementation that wraps sc is non-recording and performs +// no operations other than to return sc as the SpanContext from the +// SpanContext method. +func ContextWithSpanContext(parent context.Context, sc SpanContext) context.Context { + return ContextWithSpan(parent, nonRecordingSpan{sc: sc}) +} + // ContextWithRemoteSpanContext returns a copy of parent with rsc set explicly // as a remote SpanContext and as the current Span. The Span implementation // that wraps rsc is non-recording and performs no operations other than to // return rsc as the SpanContext from the SpanContext method. func ContextWithRemoteSpanContext(parent context.Context, rsc SpanContext) context.Context { - return ContextWithSpan(parent, nonRecordingSpan{sc: rsc.WithRemote(true)}) + return ContextWithSpanContext(parent, rsc.WithRemote(true)) } // SpanFromContext returns the current Span from ctx. @@ -38,6 +46,9 @@ func ContextWithRemoteSpanContext(parent context.Context, rsc SpanContext) conte // If no Span is currently set in ctx an implementation of a Span that // performs no operations is returned. func SpanFromContext(ctx context.Context) Span { + if ctx == nil { + return noopSpan{} + } if span, ok := ctx.Value(currentSpanKey).(Span); ok { return span } diff --git a/trace/context_test.go b/trace/context_test.go index d41cddefb2d..c7529022991 100644 --- a/trace/context_test.go +++ b/trace/context_test.go @@ -51,6 +51,11 @@ func TestSpanFromContext(t *testing.T) { }{ { name: "empty context", + context: nil, + expectedSpan: emptySpan, + }, + { + name: "background context", context: context.Background(), expectedSpan: emptySpan, },