Update SamplingParameters (#1749)

* Update SamplingParameters

Remove HasRemoteParent fields from SamplingParameters. The
HasRemoteParent field is a duplicate of the Remote field of the parent
span context contained in the ParentContext.

Change the `ParentContext` field from storing a `SpanContext` to a
`context.Context` that holds the parent span. This is to conform with
the OpenTelemetry specification and resolve #1727.

* Update PR number

CHANGELOG.md

@@ -18,6 +18,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   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)
 - Information about a parent span context in a `"go.opentelemetry.io/otel/export/trace".SpanSnapshot` is unified in a new `Parent` field.
   The existing `ParentSpanID` and `HasRemoteParent` fields are removed in favor of this. (#1748)
+- 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. (#1749)
 
 ### Removed
 
@@ -29,6 +31,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. (#1749)
 
 ## [0.19.0] - 2021-03-18
 

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)

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,35 +115,48 @@ 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(
 				nil,
 				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")
 		})

sdk/trace/span.go

@@ -543,13 +543,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{

sdk/trace/trace_test.go

@@ -1532,7 +1532,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 {

trace/context.go

@@ -25,19 +25,30 @@ 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.
 //
 // 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
 	}

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,
 		},