Repository: open-telemetry/opentelemetry-go
Pull request #1702 (2021-03-16 18:15:22)

Rename WithDefaultSampler TracerProvider option to WithSampler and update docs
The term "DefaultSampler" comes from early ideas of this project where there would be overriding samplers lower in the trace SDK. This overriding does not exist and if it is going to be introduced in the future the sampler associated with the TracerProvider is already scoped based on that association (no need to scope with a name). This renames the TracerProvider option to not include this anachronism.

This does not rename the `TracerProvider` config which is being removed in https://github.com/open-telemetry/opentelemetry-go/pull/1693

seh: If one does not pass the `WithSampler` option, what is the default sampling behavior?
MrAlias: @seh `ParentBased(AlwaysSample())`:

https://github.com/open-telemetry/opentelemetry-go/blob/345f264a137ed7162c30d14dd4739b5b72f76537/sdk/trace/provider.go#L69
seh: Thanks for the reference. What do you think about mentioning that in the documentation for `WithSampler`? In other words, answer the "But what am I replacing here?" question at that point. Either that, or maybe cross-reference some other function where we document that default sampling behavior.
MrAlias: > What do you think about mentioning that in the documentation for `WithSampler`?

SGTM


codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702?src=pr&el=h1) Report
> Merging [#1702](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702?src=pr&el=desc) (3896e56) into [main](https://codecov.io/gh/open-telemetry/opentelemetry-go/commit/345f264a137ed7162c30d14dd4739b5b72f76537?el=desc) (345f264) will **increase** coverage by `0.0%`.
> The diff coverage is `100.0%`.

[![Impacted file tree graph](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702/graphs/tree.svg?width=650&height=150&src=pr&token=8efTmh4kvf)](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702?src=pr&el=tree)

```diff
@@          Coverage Diff          @@
##            main   #1702   +/-   ##
=====================================
  Coverage   77.8%   77.8%           
=====================================
  Files        130     130           
  Lines       6988    6988           
=====================================
+ Hits        5438    5440    +2     
+ Misses      1299    1297    -2     
  Partials     251     251           
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [sdk/trace/provider.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702/diff?src=pr&el=tree#diff-c2RrL3RyYWNlL3Byb3ZpZGVyLmdv) | `83.0% <ø> (ø)` | |
| [exporters/otlp/internal/otlptest/otlptest.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702/diff?src=pr&el=tree#diff-ZXhwb3J0ZXJzL290bHAvaW50ZXJuYWwvb3RscHRlc3Qvb3RscHRlc3QuZ28=) | `74.5% <100.0%> (ø)` | |
| [exporters/trace/jaeger/jaeger.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702/diff?src=pr&el=tree#diff-ZXhwb3J0ZXJzL3RyYWNlL2phZWdlci9qYWVnZXIuZ28=) | `93.7% <100.0%> (ø)` | |
| [exporters/otlp/otlpgrpc/connection.go](https://codecov.io/gh/open-telemetry/opentelemetry-go/pull/1702/diff?src=pr&el=tree#diff-ZXhwb3J0ZXJzL290bHAvb3RscGdycGMvY29ubmVjdGlvbi5nbw==) | `88.7% <0.0%> (+1.8%)` | :arrow_up: |

MrAlias: @seh PTAL: https://github.com/open-telemetry/opentelemetry-go/pull/1702/commits/5228b6c1ac20fdc5d548a04dc1e0ba2e2a7caabf

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8ac5b86c..b5fd504e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,7 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - Added `Marshaler` config option to `otlphttp` to enable otlp over json or protobufs. (#1586)
 - A `ForceFlush` method to the `"go.opentelemetry.io/otel/sdk/trace".TracerProvider` to flush all registered `SpanProcessor`s. (#1608)
-- Added `WithDefaultSampler` and `WithSpanLimits` to tracer provider. (#1633)
+- Added `WithSampler` and `WithSpanLimits` to tracer provider. (#1633, #1702)
 - Jaeger exporter falls back to `resource.Default`'s `service.name` if the exported Span does not have one. (#1673)
 - `"go.opentelemetry.io/otel/trace".SpanContext` now has a `remote` property, and `IsRemote()` predicate, that is true when the `SpanContext` has been extracted from remote context data. (#1701)
 - A `Valid` method to the `"go.opentelemetry.io/otel/attribute".KeyValue` type. (#1703)
diff --git a/example/namedtracer/main.go b/example/namedtracer/main.go
index 214d6518..0ea4669a 100644
--- a/example/namedtracer/main.go
+++ b/example/namedtracer/main.go
@@ -45,7 +45,7 @@ func initTracer() {
 	}
 	bsp := sdktrace.NewBatchSpanProcessor(exp)
 	tp = sdktrace.NewTracerProvider(
-		sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
 		sdktrace.WithSpanProcessor(bsp),
 	)
 	otel.SetTracerProvider(tp)
diff --git a/example/otel-collector/main.go b/example/otel-collector/main.go
index cdaf3001..2560c04a 100644
--- a/example/otel-collector/main.go
+++ b/example/otel-collector/main.go
@@ -69,7 +69,7 @@ func initProvider() func() {
 
 	bsp := sdktrace.NewBatchSpanProcessor(exp)
 	tracerProvider := sdktrace.NewTracerProvider(
-		sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
 		sdktrace.WithResource(res),
 		sdktrace.WithSpanProcessor(bsp),
 	)
diff --git a/exporters/trace/jaeger/jaeger.go b/exporters/trace/jaeger/jaeger.go
index 2d192bdf..1409f363 100644
--- a/exporters/trace/jaeger/jaeger.go
+++ b/exporters/trace/jaeger/jaeger.go
@@ -160,7 +160,7 @@ func NewExportPipeline(endpointOption EndpointOption, opts ...Option) (trace.Tra
 	pOpts := []sdktrace.TracerProviderOption{sdktrace.WithSyncer(exporter)}
 	if exporter.o.Config != nil {
 		pOpts = append(pOpts,
-			sdktrace.WithDefaultSampler(exporter.o.Config.DefaultSampler),
+			sdktrace.WithSampler(exporter.o.Config.DefaultSampler),
 			sdktrace.WithIDGenerator(exporter.o.Config.IDGenerator),
 			sdktrace.WithSpanLimits(exporter.o.Config.SpanLimits),
 			sdktrace.WithResource(exporter.o.Config.Resource),
diff --git a/sdk/trace/provider.go b/sdk/trace/provider.go
index 5ecf706a..df1f4f29 100644
--- a/sdk/trace/provider.go
+++ b/sdk/trace/provider.go
@@ -52,9 +52,16 @@ type TracerProvider struct {
 
 var _ trace.TracerProvider = &TracerProvider{}
 
-// NewTracerProvider creates an instance of trace provider. Optional
-// parameter configures the provider with common options applicable
-// to all tracer instances that will be created by this provider.
+// NewTracerProvider returns a new and configured TracerProvider.
+//
+// By default the returned TracerProvider is configured with:
+//  - a ParentBased(AlwaysSample) Sampler
+//  - a random number IDGenerator
+//  - the resource.Default() Resource
+//  - the default SpanLimits.
+//
+// The passed opts are used to override these default values and configure the
+// returned TracerProvider appropriately.
 func NewTracerProvider(opts ...TracerProviderOption) *TracerProvider {
 	o := &TracerProviderConfig{}
 
@@ -274,29 +281,52 @@ func WithSpanProcessor(sp SpanProcessor) TracerProviderOption {
 	}
 }
 
-// WithResource option attaches a resource to the provider.
-// The resource is added to the span when it is started.
+// WithResource returns a TracerProviderOption that will configure the
+// Resource r as a TracerProvider's Resource. The configured Resource is
+// referenced by all the Tracers the TracerProvider creates. It represents the
+// entity producing telemetry.
+//
+// If this option is not used, the TracerProvider will use the
+// resource.Default() Resource by default.
 func WithResource(r *resource.Resource) TracerProviderOption {
 	return func(opts *TracerProviderConfig) {
 		opts.config.Resource = r
 	}
 }
 
-// WithIDGenerator option registers an IDGenerator with the TracerProvider.
+// WithIDGenerator returns a TracerProviderOption that will configure the
+// IDGenerator g as a TracerProvider's IDGenerator. The configured IDGenerator
+// is used by the Tracers the TracerProvider creates to generate new Span and
+// Trace IDs.
+//
+// If this option is not used, the TracerProvider will use a random number
+// IDGenerator by default.
 func WithIDGenerator(g IDGenerator) TracerProviderOption {
 	return func(opts *TracerProviderConfig) {
 		opts.config.IDGenerator = g
 	}
 }
 
-// WithDefaultSampler option registers a DefaultSampler with the the TracerProvider.
-func WithDefaultSampler(s Sampler) TracerProviderOption {
+// WithSampler returns a TracerProviderOption that will configure the Sampler
+// s as a TracerProvider's Sampler. The configured Sampler is used by the
+// Tracers the TracerProvider creates to make their sampling decisions for the
+// Spans they create.
+//
+// If this option is not used, the TracerProvider will use a
+// ParentBased(AlwaysSample) Sampler by default.
+func WithSampler(s Sampler) TracerProviderOption {
 	return func(opts *TracerProviderConfig) {
 		opts.config.DefaultSampler = s
 	}
 }
 
-// WithSpanLimits option registers a SpanLimits with the the TracerProvider.
+// WithSpanLimits returns a TracerProviderOption that will configure the
+// SpanLimits sl as a TracerProvider's SpanLimits. The configured SpanLimits
+// are used used by the Tracers the TracerProvider and the Spans they create
+// to limit tracing resources used.
+//
+// If this option is not used, the TracerProvider will use the default
+// SpanLimits.
 func WithSpanLimits(sl SpanLimits) TracerProviderOption {
 	return func(opts *TracerProviderConfig) {
 		opts.config.SpanLimits = sl


Test patch:
diff --git a/exporters/otlp/internal/otlptest/otlptest.go b/exporters/otlp/internal/otlptest/otlptest.go
index 5096064d..d2ffd202 100644
--- a/exporters/otlp/internal/otlptest/otlptest.go
+++ b/exporters/otlp/internal/otlptest/otlptest.go
@@ -40,7 +40,7 @@ import (
 // themselves.
 func RunEndToEndTest(ctx context.Context, t *testing.T, exp *otlp.Exporter, mcTraces, mcMetrics Collector) {
 	pOpts := []sdktrace.TracerProviderOption{
-		sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
 		sdktrace.WithBatcher(
 			exp,
 			// add following two options to ensure flush
diff --git a/exporters/otlp/otlpgrpc/example_test.go b/exporters/otlp/otlpgrpc/example_test.go
index 4160f724..4715e25b 100644
--- a/exporters/otlp/otlpgrpc/example_test.go
+++ b/exporters/otlp/otlpgrpc/example_test.go
@@ -49,7 +49,7 @@ func Example_insecure() {
 	}()
 
 	tp := sdktrace.NewTracerProvider(
-		sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
 		sdktrace.WithBatcher(
 			exp,
 			// add following two options to ensure flush
@@ -102,7 +102,7 @@ func Example_withTLS() {
 	}()
 
 	tp := sdktrace.NewTracerProvider(
-		sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
 		sdktrace.WithBatcher(
 			exp,
 			// add following two options to ensure flush
@@ -163,7 +163,7 @@ func Example_withDifferentSignalCollectors() {
 	}()
 
 	tp := sdktrace.NewTracerProvider(
-		sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
 		sdktrace.WithBatcher(
 			exp,
 			// add following two options to ensure flush
diff --git a/exporters/otlp/otlpgrpc/otlp_integration_test.go b/exporters/otlp/otlpgrpc/otlp_integration_test.go
index 2c1ca062..35e3e2fd 100644
--- a/exporters/otlp/otlpgrpc/otlp_integration_test.go
+++ b/exporters/otlp/otlpgrpc/otlp_integration_test.go
@@ -333,7 +333,7 @@ func TestNewExporter_withMultipleAttributeTypes(t *testing.T) {
 	}()
 
 	tp := sdktrace.NewTracerProvider(
-		sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
 		sdktrace.WithBatcher(
 			exp,
 			// add following two options to ensure flush
diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go
index f150dce4..036771c6 100644
--- a/exporters/trace/jaeger/jaeger_test.go
+++ b/exporters/trace/jaeger/jaeger_test.go
@@ -317,7 +317,7 @@ func TestExporter_ExportSpan(t *testing.T) {
 	assert.NoError(t, err)
 
 	tp := sdktrace.NewTracerProvider(
-		sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()),
+		sdktrace.WithSampler(sdktrace.AlwaysSample()),
 		sdktrace.WithSyncer(exp),
 	)
 	otel.SetTracerProvider(tp)
diff --git a/sdk/trace/benchmark_test.go b/sdk/trace/benchmark_test.go
index 3243f5a2..b5322e95 100644
--- a/sdk/trace/benchmark_test.go
+++ b/sdk/trace/benchmark_test.go
@@ -149,6 +149,6 @@ func traceBenchmark(b *testing.B, name string, fn func(*testing.B, trace.Tracer)
 }
 
 func tracer(b *testing.B, name string, sampler sdktrace.Sampler) trace.Tracer {
-	tp := sdktrace.NewTracerProvider(sdktrace.WithDefaultSampler(sampler))
+	tp := sdktrace.NewTracerProvider(sdktrace.WithSampler(sampler))
 	return tp.Tracer(name)
 }
diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go
index ee5858c0..ff4726bf 100644
--- a/sdk/trace/trace_test.go
+++ b/sdk/trace/trace_test.go
@@ -82,10 +82,10 @@ func TestTracerFollowsExpectedAPIBehaviour(t *testing.T) {
 	harness := oteltest.NewHarness(t)
 
 	harness.TestTracerProvider(func() trace.TracerProvider {
-		return NewTracerProvider(WithDefaultSampler(TraceIDRatioBased(0)))
+		return NewTracerProvider(WithSampler(TraceIDRatioBased(0)))
 	})
 
-	tp := NewTracerProvider(WithDefaultSampler(TraceIDRatioBased(0)))
+	tp := NewTracerProvider(WithSampler(TraceIDRatioBased(0)))
 	harness.TestTracer(func() trace.Tracer {
 		return tp.Tracer("")
 	})
@@ -270,7 +270,7 @@ func TestSampling(t *testing.T) {
 		tc := tc
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
-			p := NewTracerProvider(WithDefaultSampler(tc.sampler))
+			p := NewTracerProvider(WithSampler(tc.sampler))
 			tr := p.Tracer("test")
 			var sampled int
 			for i := 0; i < total; i++ {
@@ -422,7 +422,7 @@ func TestSetSpanAttributes(t *testing.T) {
 func TestSamplerAttributesLocalChildSpan(t *testing.T) {
 	sampler := &testSampler{prefix: "span", t: t}
 	te := NewTestExporter()
-	tp := NewTracerProvider(WithDefaultSampler(sampler), WithSyncer(te), WithResource(resource.Empty()))
+	tp := NewTracerProvider(WithSampler(sampler), WithSyncer(te), WithResource(resource.Empty()))
 
 	ctx := context.Background()
 	ctx, span := startLocalSpan(tp, ctx, "SpanOne", "span0")
@@ -953,7 +953,7 @@ func TestEndSpanTwice(t *testing.T) {
 
 func TestStartSpanAfterEnd(t *testing.T) {
 	te := NewTestExporter()
-	tp := NewTracerProvider(WithDefaultSampler(AlwaysSample()), WithSyncer(te))
+	tp := NewTracerProvider(WithSampler(AlwaysSample()), WithSyncer(te))
 	ctx := context.Background()
 
 	tr := tp.Tracer("SpanAfterEnd")
@@ -998,7 +998,7 @@ func TestStartSpanAfterEnd(t *testing.T) {
 
 func TestChildSpanCount(t *testing.T) {
 	te := NewTestExporter()
-	tp := NewTracerProvider(WithDefaultSampler(AlwaysSample()), WithSyncer(te))
+	tp := NewTracerProvider(WithSampler(AlwaysSample()), WithSyncer(te))
 
 	tr := tp.Tracer("ChidSpanCount")
 	ctx, span0 := tr.Start(context.Background(), "parent")
@@ -1052,7 +1052,7 @@ func TestNilSpanEnd(t *testing.T) {
 
 func TestExecutionTracerTaskEnd(t *testing.T) {
 	var n uint64
-	tp := NewTracerProvider(WithDefaultSampler(NeverSample()))
+	tp := NewTracerProvider(WithSampler(NeverSample()))
 	tr := tp.Tracer("Execution Tracer Task End")
 
 	executionTracerTaskEnd := func() {
@@ -1085,7 +1085,6 @@ func TestExecutionTracerTaskEnd(t *testing.T) {
 	s.executionTracerTaskEnd = executionTracerTaskEnd
 	spans = append(spans, s) // parent not sampled
 
-	// tp.ApplyConfig(Config{DefaultSampler: AlwaysSample()})
 	_, apiSpan = tr.Start(context.Background(), "foo")
 	s = apiSpan.(*span)
 	s.executionTracerTaskEnd = executionTracerTaskEnd
@@ -1101,7 +1100,7 @@ func TestExecutionTracerTaskEnd(t *testing.T) {
 
 func TestCustomStartEndTime(t *testing.T) {
 	te := NewTestExporter()
-	tp := NewTracerProvider(WithSyncer(te), WithDefaultSampler(AlwaysSample()))
+	tp := NewTracerProvider(WithSyncer(te), WithSampler(AlwaysSample()))
 
 	startTime := time.Date(2019, time.August, 27, 14, 42, 0, 0, time.UTC)
 	endTime := startTime.Add(time.Second * 20)
@@ -1215,7 +1214,7 @@ func TestRecordErrorNil(t *testing.T) {
 
 func TestWithSpanKind(t *testing.T) {
 	te := NewTestExporter()
-	tp := NewTracerProvider(WithSyncer(te), WithDefaultSampler(AlwaysSample()), WithResource(resource.Empty()))
+	tp := NewTracerProvider(WithSyncer(te), WithSampler(AlwaysSample()), WithResource(resource.Empty()))
 	tr := tp.Tracer("withSpanKind")
 
 	_, span := tr.Start(context.Background(), "WithoutSpanKind")
@@ -1285,7 +1284,7 @@ func TestWithResource(t *testing.T) {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			te := NewTestExporter()
-			defaultOptions := []TracerProviderOption{WithSyncer(te), WithDefaultSampler(AlwaysSample())}
+			defaultOptions := []TracerProviderOption{WithSyncer(te), WithSampler(AlwaysSample())}
 			tp := NewTracerProvider(append(defaultOptions, tc.options...)...)
 			span := startSpan(tp, "WithResource")
 			span.SetAttributes(attribute.String("key1", "value1"))
@@ -1709,7 +1708,7 @@ func TestSamplerTraceState(t *testing.T) {
 		ts := ts
 		t.Run(ts.name, func(t *testing.T) {
 			te := NewTestExporter()
-			tp := NewTracerProvider(WithDefaultSampler(ts.sampler), WithSyncer(te), WithResource(resource.Empty()))
+			tp := NewTracerProvider(WithSampler(ts.sampler), WithSyncer(te), WithResource(resource.Empty()))
 			tr := tp.Tracer("TraceState")
 
 			sc1 := trace.NewSpanContext(trace.SpanContextConfig{
diff --git a/sdk/trace/util_test.go b/sdk/trace/util_test.go
index d6aca0f5..d921ed69 100644
--- a/sdk/trace/util_test.go
+++ b/sdk/trace/util_test.go
@@ -21,6 +21,6 @@ import (
 )
 
 func basicTracerProvider(t *testing.T) *sdktrace.TracerProvider {
-	tp := sdktrace.NewTracerProvider(sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()))
+	tp := sdktrace.NewTracerProvider(sdktrace.WithSampler(sdktrace.AlwaysSample()))
 	return tp
 }