Rename ParentOrElse sampler to ParentBased and enhance it according t…

…o the spec (#1153)

* Rename ParentOrElse to ParentBased and enhance it according to the spec

- Renaming of type and sampler function
- Enhancing ParentBased with sampler options
- Set default samplers for each applicable parent-based case
- Adjust ShouldSample(...) func accordingly

* Adjust existing tests for ParentBased and add new ones

- add tests for ParentBased sampler options and description
- renaming in trace_test.go

* Update CHANGELOG.md

* PR feedback

- More clearer naming of structs; add comments where missing
- Adhere to the configuration style guide

* PR feedback - punctuation

CHANGELOG.md

@@ -32,6 +32,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Move `tools` package under `internal`. (#1141)
 - Move `go.opentelemetry.io/otel/api/correlation` package to `go.opentelemetry.io/otel/api/baggage`. (#1142)
    The `correlation.CorrelationContext` propagator has been renamed `baggage.Baggage`.  Other exported functions and types are unchanged.
+- Rename `ParentOrElse` sampler to `ParentBased` and allow setting samplers depending on parent span. (#1153)
 - In the `go.opentelemetry.io/otel/api/trace` package, `SpanConfigure` was renamed to `NewSpanConfig`. (#1155)
 - Change `dependabot.yml` to add a `Skip Changelog` label to dependabot-sourced PRs. (#1161)
 

sdk/trace/provider.go

@@ -64,7 +64,7 @@ func NewProvider(opts ...ProviderOption) *Provider {
 		namedTracer: make(map[instrumentation.Library]*tracer),
 	}
 	tp.config.Store(&Config{
-		DefaultSampler:       ParentSample(AlwaysSample()),
+		DefaultSampler:       ParentBased(AlwaysSample()),
 		IDGenerator:          defIDGenerator(),
 		MaxAttributesPerSpan: DefaultMaxAttributesPerSpan,
 		MaxEventsPerSpan:     DefaultMaxEventsPerSpan,

sdk/trace/sampling.go

@@ -125,29 +125,131 @@ func NeverSample() Sampler {
 	return alwaysOffSampler{}
 }
 
-// ParentSample returns a Sampler that samples a trace only
-// if the the span has a parent span and it is sampled. If the span has
-// parent span but it is not sampled, neither will this span. If the span
-// does not have a parent the fallback Sampler is used to determine if the
-// span should be sampled.
-func ParentSample(fallback Sampler) Sampler {
-	return parentSampler{fallback}
+// ParentBased returns a composite sampler which behaves differently,
+// based on the parent of the span. If the span has no parent,
+// the root(Sampler) is used to make sampling decision. If the span has
+// a parent, depending on whether the parent is remote and whether it
+// is sampled, one of the following samplers will apply:
+// - remoteParentSampled(Sampler) (default: AlwaysOn)
+// - remoteParentNotSampled(Sampler) (default: AlwaysOff)
+// - localParentSampled(Sampler) (default: AlwaysOn)
+// - localParentNotSampled(Sampler) (default: AlwaysOff)
+func ParentBased(root Sampler, samplers ...ParentBasedSamplerOption) Sampler {
+	return parentBased{
+		root:   root,
+		config: configureSamplersForParentBased(samplers),
+	}
+}
+
+type parentBased struct {
+	root   Sampler
+	config config
+}
+
+func configureSamplersForParentBased(samplers []ParentBasedSamplerOption) config {
+	c := config{
+		remoteParentSampled:    AlwaysSample(),
+		remoteParentNotSampled: NeverSample(),
+		localParentSampled:     AlwaysSample(),
+		localParentNotSampled:  NeverSample(),
+	}
+
+	for _, so := range samplers {
+		so.Apply(&c)
+	}
+
+	return c
+}
+
+// config is a group of options for parentBased sampler.
+type config struct {
+	remoteParentSampled, remoteParentNotSampled Sampler
+	localParentSampled, localParentNotSampled   Sampler
+}
+
+// ParentBasedSamplerOption configures the sampler for a particular sampling case.
+type ParentBasedSamplerOption interface {
+	Apply(*config)
+}
+
+// WithRemoteParentSampled sets the sampler for the case of sampled remote parent.
+func WithRemoteParentSampled(s Sampler) ParentBasedSamplerOption {
+	return remoteParentSampledOption{s}
 }
 
-type parentSampler struct {
-	fallback Sampler
+type remoteParentSampledOption struct {
+	s Sampler
 }
 
-func (ps parentSampler) ShouldSample(p SamplingParameters) SamplingResult {
+func (o remoteParentSampledOption) Apply(config *config) {
+	config.remoteParentSampled = o.s
+}
+
+// WithRemoteParentNotSampled sets the sampler for the case of remote parent
+// which is not sampled.
+func WithRemoteParentNotSampled(s Sampler) ParentBasedSamplerOption {
+	return remoteParentNotSampledOption{s}
+}
+
+type remoteParentNotSampledOption struct {
+	s Sampler
+}
+
+func (o remoteParentNotSampledOption) Apply(config *config) {
+	config.remoteParentNotSampled = o.s
+}
+
+// WithLocalParentSampled sets the sampler for the case of sampled local parent.
+func WithLocalParentSampled(s Sampler) ParentBasedSamplerOption {
+	return localParentSampledOption{s}
+}
+
+type localParentSampledOption struct {
+	s Sampler
+}
+
+func (o localParentSampledOption) Apply(config *config) {
+	config.localParentSampled = o.s
+}
+
+// WithLocalParentNotSampled sets the sampler for the case of local parent
+// which is not sampled.
+func WithLocalParentNotSampled(s Sampler) ParentBasedSamplerOption {
+	return localParentNotSampledOption{s}
+}
+
+type localParentNotSampledOption struct {
+	s Sampler
+}
+
+func (o localParentNotSampledOption) Apply(config *config) {
+	config.localParentNotSampled = o.s
+}
+
+func (pb parentBased) ShouldSample(p SamplingParameters) SamplingResult {
 	if p.ParentContext.IsValid() {
+		if p.HasRemoteParent {
+			if p.ParentContext.IsSampled() {
+				return pb.config.remoteParentSampled.ShouldSample(p)
+			}
+			return pb.config.remoteParentNotSampled.ShouldSample(p)
+		}
+
 		if p.ParentContext.IsSampled() {
-			return SamplingResult{Decision: RecordAndSampled}
+			return pb.config.localParentSampled.ShouldSample(p)
 		}
-		return SamplingResult{Decision: NotRecord}
+		return pb.config.localParentNotSampled.ShouldSample(p)
 	}
-	return ps.fallback.ShouldSample(p)
-}
-
-func (ps parentSampler) Description() string {
-	return fmt.Sprintf("ParentOrElse{%s}", ps.fallback.Description())
+	return pb.root.ShouldSample(p)
+}
+
+func (pb parentBased) Description() string {
+	return fmt.Sprintf("ParentBased{root:%s,remoteParentSampled:%s,"+
+		"remoteParentNotSampled:%s,localParentSampled:%s,localParentNotSampled:%s}",
+		pb.root.Description(),
+		pb.config.remoteParentSampled.Description(),
+		pb.config.remoteParentNotSampled.Description(),
+		pb.config.localParentSampled.Description(),
+		pb.config.localParentNotSampled.Description(),
+	)
 }

sdk/trace/sampling_test.go

@@ -15,6 +15,7 @@
 package trace
 
 import (
+	"fmt"
 	"math/rand"
 	"testing"
 
@@ -23,8 +24,8 @@ import (
 	api "go.opentelemetry.io/otel/api/trace"
 )
 
-func TestAlwaysParentSampleWithParentSampled(t *testing.T) {
-	sampler := ParentSample(AlwaysSample())
+func TestParentBasedDefaultLocalParentSampled(t *testing.T) {
+	sampler := ParentBased(AlwaysSample())
 	traceID, _ := api.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736")
 	spanID, _ := api.SpanIDFromHex("00f067aa0ba902b7")
 	parentCtx := api.SpanContext{
@@ -37,22 +38,8 @@ func TestAlwaysParentSampleWithParentSampled(t *testing.T) {
 	}
 }
 
-func TestNeverParentSampleWithParentSampled(t *testing.T) {
-	sampler := ParentSample(NeverSample())
-	traceID, _ := api.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736")
-	spanID, _ := api.SpanIDFromHex("00f067aa0ba902b7")
-	parentCtx := api.SpanContext{
-		TraceID:    traceID,
-		SpanID:     spanID,
-		TraceFlags: api.FlagsSampled,
-	}
-	if sampler.ShouldSample(SamplingParameters{ParentContext: parentCtx}).Decision != RecordAndSampled {
-		t.Error("Sampling decision should be RecordAndSampled")
-	}
-}
-
-func TestAlwaysParentSampleWithParentNotSampled(t *testing.T) {
-	sampler := ParentSample(AlwaysSample())
+func TestParentBasedDefaultLocalParentNotSampled(t *testing.T) {
+	sampler := ParentBased(AlwaysSample())
 	traceID, _ := api.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736")
 	spanID, _ := api.SpanIDFromHex("00f067aa0ba902b7")
 	parentCtx := api.SpanContext{
@@ -64,20 +51,114 @@ func TestAlwaysParentSampleWithParentNotSampled(t *testing.T) {
 	}
 }
 
-func TestParentSampleWithNoParent(t *testing.T) {
+func TestParentBasedWithNoParent(t *testing.T) {
 	params := SamplingParameters{}
 
-	sampler := ParentSample(AlwaysSample())
+	sampler := ParentBased(AlwaysSample())
 	if sampler.ShouldSample(params).Decision != RecordAndSampled {
 		t.Error("Sampling decision should be RecordAndSampled")
 	}
 
-	sampler = ParentSample(NeverSample())
+	sampler = ParentBased(NeverSample())
 	if sampler.ShouldSample(params).Decision != NotRecord {
 		t.Error("Sampling decision should be NotRecord")
 	}
 }
 
+func TestParentBasedWithSamplerOptions(t *testing.T) {
+	testCases := []struct {
+		name                            string
+		samplerOption                   ParentBasedSamplerOption
+		isParentRemote, isParentSampled bool
+		expectedDecision                SamplingDecision
+	}{
+		{
+			"localParentSampled",
+			WithLocalParentSampled(NeverSample()),
+			false,
+			true,
+			NotRecord,
+		},
+		{
+			"localParentNotSampled",
+			WithLocalParentNotSampled(AlwaysSample()),
+			false,
+			false,
+			RecordAndSampled,
+		},
+		{
+			"remoteParentSampled",
+			WithRemoteParentSampled(NeverSample()),
+			true,
+			true,
+			NotRecord,
+		},
+		{
+			"remoteParentNotSampled",
+			WithRemoteParentNotSampled(AlwaysSample()),
+			true,
+			false,
+			RecordAndSampled,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			traceID, _ := api.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736")
+			spanID, _ := api.SpanIDFromHex("00f067aa0ba902b7")
+			parentCtx := api.SpanContext{
+				TraceID: traceID,
+				SpanID:  spanID,
+			}
+
+			if tc.isParentSampled {
+				parentCtx.TraceFlags = api.FlagsSampled
+			}
+
+			params := SamplingParameters{ParentContext: parentCtx}
+			if tc.isParentRemote {
+				params.HasRemoteParent = true
+			}
+
+			sampler := ParentBased(
+				nil,
+				tc.samplerOption,
+			)
+
+			switch tc.expectedDecision {
+			case RecordAndSampled:
+				if sampler.ShouldSample(params).Decision != tc.expectedDecision {
+					t.Error("Sampling decision should be RecordAndSampled")
+				}
+			case NotRecord:
+				if sampler.ShouldSample(params).Decision != tc.expectedDecision {
+					t.Error("Sampling decision should be NotRecord")
+				}
+			}
+		})
+	}
+}
+
+func TestParentBasedDefaultDescription(t *testing.T) {
+	sampler := ParentBased(AlwaysSample())
+
+	expectedDescription := fmt.Sprintf("ParentBased{root:%s,remoteParentSampled:%s,"+
+		"remoteParentNotSampled:%s,localParentSampled:%s,localParentNotSampled:%s}",
+		AlwaysSample().Description(),
+		AlwaysSample().Description(),
+		NeverSample().Description(),
+		AlwaysSample().Description(),
+		NeverSample().Description())
+
+	if sampler.Description() != expectedDescription {
+		t.Error(fmt.Sprintf("Sampler description should be %s, got '%s' instead",
+			expectedDescription,
+			sampler.Description(),
+		))
+	}
+
+}
+
 // TraceIDRatioBased sampler requirements state
 //  "A TraceIDRatioBased sampler with a given sampling rate MUST also sample
 //   all traces that any TraceIDRatioBased sampler with a lower sampling rate

sdk/trace/trace_test.go

@@ -232,10 +232,10 @@ func TestSampling(t *testing.T) {
 		"TraceIdRatioBased_.75": {sampler: TraceIDRatioBased(0.75), expect: .75},
 		"TraceIdRatioBased_2.0": {sampler: TraceIDRatioBased(2.0), expect: 1},
 
-		// Spans w/o a parent and using ParentSample(DelegateSampler()) Sampler, receive DelegateSampler's sampling decision
-		"ParentNeverSample":           {sampler: ParentSample(NeverSample()), expect: 0},
-		"ParentAlwaysSample":          {sampler: ParentSample(AlwaysSample()), expect: 1},
-		"ParentTraceIdRatioBased_.50": {sampler: ParentSample(TraceIDRatioBased(0.50)), expect: .5},
+		// Spans w/o a parent and using ParentBased(DelegateSampler()) Sampler, receive DelegateSampler's sampling decision
+		"ParentNeverSample":           {sampler: ParentBased(NeverSample()), expect: 0},
+		"ParentAlwaysSample":          {sampler: ParentBased(AlwaysSample()), expect: 1},
+		"ParentTraceIdRatioBased_.50": {sampler: ParentBased(TraceIDRatioBased(0.50)), expect: .5},
 
 		// An unadorned TraceIDRatioBased sampler ignores parent spans
 		"UnsampledParentSpanWithTraceIdRatioBased_.25": {sampler: TraceIDRatioBased(0.25), expect: .25, parent: true},
@@ -248,13 +248,13 @@ func TestSampling(t *testing.T) {
 		// Spans with a sampled parent but using NeverSample Sampler, are not sampled
 		"SampledParentSpanWithNeverSample": {sampler: NeverSample(), expect: 0, parent: true, sampledParent: true},
 
-		// Spans with a sampled parent and using ParentSample(DelegateSampler()) Sampler, inherit the parent span's sampling status
-		"SampledParentSpanWithParentNeverSample":             {sampler: ParentSample(NeverSample()), expect: 1, parent: true, sampledParent: true},
-		"UnsampledParentSpanWithParentNeverSampler":          {sampler: ParentSample(NeverSample()), expect: 0, parent: true, sampledParent: false},
-		"SampledParentSpanWithParentAlwaysSampler":           {sampler: ParentSample(AlwaysSample()), expect: 1, parent: true, sampledParent: true},
-		"UnsampledParentSpanWithParentAlwaysSampler":         {sampler: ParentSample(AlwaysSample()), expect: 0, parent: true, sampledParent: false},
-		"SampledParentSpanWithParentTraceIdRatioBased_.50":   {sampler: ParentSample(TraceIDRatioBased(0.50)), expect: 1, parent: true, sampledParent: true},
-		"UnsampledParentSpanWithParentTraceIdRatioBased_.50": {sampler: ParentSample(TraceIDRatioBased(0.50)), expect: 0, parent: true, sampledParent: false},
+		// Spans with a sampled parent and using ParentBased(DelegateSampler()) Sampler, inherit the parent span's sampling status
+		"SampledParentSpanWithParentNeverSample":             {sampler: ParentBased(NeverSample()), expect: 1, parent: true, sampledParent: true},
+		"UnsampledParentSpanWithParentNeverSampler":          {sampler: ParentBased(NeverSample()), expect: 0, parent: true, sampledParent: false},
+		"SampledParentSpanWithParentAlwaysSampler":           {sampler: ParentBased(AlwaysSample()), expect: 1, parent: true, sampledParent: true},
+		"UnsampledParentSpanWithParentAlwaysSampler":         {sampler: ParentBased(AlwaysSample()), expect: 0, parent: true, sampledParent: false},
+		"SampledParentSpanWithParentTraceIdRatioBased_.50":   {sampler: ParentBased(TraceIDRatioBased(0.50)), expect: 1, parent: true, sampledParent: true},
+		"UnsampledParentSpanWithParentTraceIdRatioBased_.50": {sampler: ParentBased(TraceIDRatioBased(0.50)), expect: 0, parent: true, sampledParent: false},
 	} {
 		tc := tc
 		t.Run(name, func(t *testing.T) {