From d75e268053a7a3fc19394cb48a98d22fd15eb36a Mon Sep 17 00:00:00 2001 From: Injun Song Date: Tue, 9 Mar 2021 06:43:11 +0900 Subject: [PATCH] Avoid overriding configuration of tracer provider (#1633) * sdk/trace: add missing options to tracer provider This change adds `WithDefaultSampler` and `WithSpanLimits` to the tracer provider and removed `WithConfig` from it. Before this change, `WithConfig` is the only way to set sampler or limits of a span. However, it is prone to misuse, since `WithConfig` can override tracing configurations that are configured by `WithResource` or `WithIDGenerator`. Thus to fix this, it adds new functional options - `WithDefaultSampler` and `WithSpanLimits` and removes `WithConfig`. Resolves #1631. * Update sdk/trace/provider.go Co-authored-by: Tyler Yahn * Update sdk/trace/provider.go Co-authored-by: Tyler Yahn * rebase and remove WithConfig Co-authored-by: Tyler Yahn --- CHANGELOG.md | 2 + example/namedtracer/main.go | 6 +-- example/otel-collector/main.go | 2 +- exporters/otlp/internal/otlptest/otlptest.go | 2 +- exporters/otlp/otlpgrpc/example_test.go | 6 +-- .../otlp/otlpgrpc/otlp_integration_test.go | 2 +- exporters/trace/jaeger/jaeger.go | 7 ++- exporters/trace/jaeger/jaeger_test.go | 2 +- sdk/trace/benchmark_test.go | 2 +- sdk/trace/provider.go | 21 +++++--- sdk/trace/trace_test.go | 49 ++++++++++--------- sdk/trace/util_test.go | 4 +- 12 files changed, 58 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53a457352d4..4a047f645ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | Windows | 1.14 | amd64 | | Windows | 1.15 | 386 | | Windows | 1.14 | 386 | +- Added `WithDefaultSampler` and `WithSpanLimits` to tracer provider. (#1633) ### Changed @@ -89,6 +90,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `test-benchmark` is no longer a dependency of the `precommit` make target. (#1567) - Removed the `test-386` make target. This was replaced with a full compatibility testing suite (i.e. multi OS/arch) in the CI system. (#1567) +- Removed `WithConfig` from tracer provider to avoid overriding configuration. (#1633) ### Fixed diff --git a/example/namedtracer/main.go b/example/namedtracer/main.go index 9356309504f..214d6518e5b 100644 --- a/example/namedtracer/main.go +++ b/example/namedtracer/main.go @@ -45,11 +45,7 @@ func initTracer() { } bsp := sdktrace.NewBatchSpanProcessor(exp) tp = sdktrace.NewTracerProvider( - sdktrace.WithConfig( - sdktrace.Config{ - DefaultSampler: sdktrace.AlwaysSample(), - }, - ), + sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()), sdktrace.WithSpanProcessor(bsp), ) otel.SetTracerProvider(tp) diff --git a/example/otel-collector/main.go b/example/otel-collector/main.go index df1829bc502..8ff81960b4f 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.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}), + sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()), sdktrace.WithResource(res), sdktrace.WithSpanProcessor(bsp), ) diff --git a/exporters/otlp/internal/otlptest/otlptest.go b/exporters/otlp/internal/otlptest/otlptest.go index 56643ec2555..5096064def4 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.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}), + sdktrace.WithDefaultSampler(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 8e666235be6..4160f7249e4 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.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}), + sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()), sdktrace.WithBatcher( exp, // add following two options to ensure flush @@ -102,7 +102,7 @@ func Example_withTLS() { }() tp := sdktrace.NewTracerProvider( - sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}), + sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()), sdktrace.WithBatcher( exp, // add following two options to ensure flush @@ -163,7 +163,7 @@ func Example_withDifferentSignalCollectors() { }() tp := sdktrace.NewTracerProvider( - sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}), + sdktrace.WithDefaultSampler(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 2004f691abf..2c1ca062cde 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.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}), + sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()), sdktrace.WithBatcher( exp, // add following two options to ensure flush diff --git a/exporters/trace/jaeger/jaeger.go b/exporters/trace/jaeger/jaeger.go index 3203258744d..76932edadf1 100644 --- a/exporters/trace/jaeger/jaeger.go +++ b/exporters/trace/jaeger/jaeger.go @@ -169,7 +169,12 @@ func NewExportPipeline(endpointOption EndpointOption, opts ...Option) (trace.Tra pOpts := []sdktrace.TracerProviderOption{sdktrace.WithSyncer(exporter)} if exporter.o.Config != nil { - pOpts = append(pOpts, sdktrace.WithConfig(*exporter.o.Config)) + pOpts = append(pOpts, + sdktrace.WithDefaultSampler(exporter.o.Config.DefaultSampler), + sdktrace.WithIDGenerator(exporter.o.Config.IDGenerator), + sdktrace.WithSpanLimits(exporter.o.Config.SpanLimits), + sdktrace.WithResource(exporter.o.Config.Resource), + ) } tp := sdktrace.NewTracerProvider(pOpts...) return tp, exporter.Flush, nil diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go index 6a52f973886..fed27d44d8e 100644 --- a/exporters/trace/jaeger/jaeger_test.go +++ b/exporters/trace/jaeger/jaeger_test.go @@ -338,7 +338,7 @@ func TestExporter_ExportSpan(t *testing.T) { assert.NoError(t, err) tp := sdktrace.NewTracerProvider( - sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()}), + sdktrace.WithDefaultSampler(sdktrace.AlwaysSample()), sdktrace.WithSyncer(exp), ) otel.SetTracerProvider(tp) diff --git a/sdk/trace/benchmark_test.go b/sdk/trace/benchmark_test.go index 183fbd3f29d..f6d5357d92f 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.WithConfig(sdktrace.Config{DefaultSampler: sampler})) + tp := sdktrace.NewTracerProvider(sdktrace.WithDefaultSampler(sampler)) return tp.Tracer(name) } diff --git a/sdk/trace/provider.go b/sdk/trace/provider.go index e0c9814f049..5ecf706a955 100644 --- a/sdk/trace/provider.go +++ b/sdk/trace/provider.go @@ -274,13 +274,6 @@ func WithSpanProcessor(sp SpanProcessor) TracerProviderOption { } } -// WithConfig option sets the configuration to provider. -func WithConfig(config Config) TracerProviderOption { - return func(opts *TracerProviderConfig) { - opts.config = config - } -} - // WithResource option attaches a resource to the provider. // The resource is added to the span when it is started. func WithResource(r *resource.Resource) TracerProviderOption { @@ -295,3 +288,17 @@ func WithIDGenerator(g IDGenerator) TracerProviderOption { opts.config.IDGenerator = g } } + +// WithDefaultSampler option registers a DefaultSampler with the the TracerProvider. +func WithDefaultSampler(s Sampler) TracerProviderOption { + return func(opts *TracerProviderConfig) { + opts.config.DefaultSampler = s + } +} + +// WithSpanLimits option registers a SpanLimits with the the TracerProvider. +func WithSpanLimits(sl SpanLimits) TracerProviderOption { + return func(opts *TracerProviderConfig) { + opts.config.SpanLimits = sl + } +} diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 0d7b7a3b095..fb46f393efc 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -36,6 +36,7 @@ import ( "github.com/stretchr/testify/require" ottest "go.opentelemetry.io/otel/internal/internaltest" + export "go.opentelemetry.io/otel/sdk/export/trace" "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/resource" @@ -81,10 +82,10 @@ func TestTracerFollowsExpectedAPIBehaviour(t *testing.T) { harness := oteltest.NewHarness(t) harness.TestTracerProvider(func() trace.TracerProvider { - return NewTracerProvider(WithConfig(Config{DefaultSampler: TraceIDRatioBased(0)})) + return NewTracerProvider(WithDefaultSampler(TraceIDRatioBased(0))) }) - tp := NewTracerProvider(WithConfig(Config{DefaultSampler: TraceIDRatioBased(0)})) + tp := NewTracerProvider(WithDefaultSampler(TraceIDRatioBased(0))) harness.TestTracer(func() trace.Tracer { return tp.Tracer("") }) @@ -269,7 +270,7 @@ func TestSampling(t *testing.T) { tc := tc t.Run(name, func(t *testing.T) { t.Parallel() - p := NewTracerProvider(WithConfig(Config{DefaultSampler: tc.sampler})) + p := NewTracerProvider(WithDefaultSampler(tc.sampler)) tr := p.Tracer("test") var sampled int for i := 0; i < total; i++ { @@ -421,7 +422,7 @@ func TestSetSpanAttributes(t *testing.T) { func TestSamplerAttributesLocalChildSpan(t *testing.T) { sampler := &testSampler{prefix: "span", t: t} te := NewTestExporter() - tp := NewTracerProvider(WithConfig(Config{DefaultSampler: sampler}), WithSyncer(te), WithResource(resource.Empty())) + tp := NewTracerProvider(WithDefaultSampler(sampler), WithSyncer(te), WithResource(resource.Empty())) ctx := context.Background() ctx, span := startLocalSpan(tp, ctx, "SpanOne", "span0") @@ -484,8 +485,7 @@ func TestSamplerAttributesLocalChildSpan(t *testing.T) { func TestSetSpanAttributesOverLimit(t *testing.T) { te := NewTestExporter() - cfg := Config{SpanLimits: SpanLimits{AttributeCountLimit: 2}} - tp := NewTracerProvider(WithConfig(cfg), WithSyncer(te), WithResource(resource.Empty())) + tp := NewTracerProvider(WithSpanLimits(SpanLimits{AttributeCountLimit: 2}), WithSyncer(te), WithResource(resource.Empty())) span := startSpan(tp, "SpanAttributesOverLimit") span.SetAttributes( @@ -522,8 +522,7 @@ func TestSetSpanAttributesOverLimit(t *testing.T) { func TestSetSpanAttributesWithInvalidKey(t *testing.T) { te := NewTestExporter() - cfg := Config{SpanLimits: SpanLimits{}} - tp := NewTracerProvider(WithConfig(cfg), WithSyncer(te), WithResource(resource.Empty())) + tp := NewTracerProvider(WithSpanLimits(SpanLimits{}), WithSyncer(te), WithResource(resource.Empty())) span := startSpan(tp, "SpanToSetInvalidKeyOrValue") span.SetAttributes( @@ -602,8 +601,7 @@ func TestEvents(t *testing.T) { func TestEventsOverLimit(t *testing.T) { te := NewTestExporter() - cfg := Config{SpanLimits: SpanLimits{EventCountLimit: 2}} - tp := NewTracerProvider(WithConfig(cfg), WithSyncer(te), WithResource(resource.Empty())) + tp := NewTracerProvider(WithSpanLimits(SpanLimits{EventCountLimit: 2}), WithSyncer(te), WithResource(resource.Empty())) span := startSpan(tp, "EventsOverLimit") k1v1 := attribute.String("key1", "value1") @@ -693,13 +691,12 @@ func TestLinks(t *testing.T) { func TestLinksOverLimit(t *testing.T) { te := NewTestExporter() - cfg := Config{SpanLimits: SpanLimits{LinkCountLimit: 2}} sc1 := trace.SpanContext{TraceID: trace.TraceID([16]byte{1, 1}), SpanID: trace.SpanID{3}} sc2 := trace.SpanContext{TraceID: trace.TraceID([16]byte{1, 1}), SpanID: trace.SpanID{3}} sc3 := trace.SpanContext{TraceID: trace.TraceID([16]byte{1, 1}), SpanID: trace.SpanID{3}} - tp := NewTracerProvider(WithConfig(cfg), WithSyncer(te), WithResource(resource.Empty())) + tp := NewTracerProvider(WithSpanLimits(SpanLimits{LinkCountLimit: 2}), WithSyncer(te), WithResource(resource.Empty())) span := startSpan(tp, "LinksOverLimit", trace.WithLinks( @@ -958,7 +955,7 @@ func TestEndSpanTwice(t *testing.T) { func TestStartSpanAfterEnd(t *testing.T) { te := NewTestExporter() - tp := NewTracerProvider(WithConfig(Config{DefaultSampler: AlwaysSample()}), WithSyncer(te)) + tp := NewTracerProvider(WithDefaultSampler(AlwaysSample()), WithSyncer(te)) ctx := context.Background() tr := tp.Tracer("SpanAfterEnd") @@ -1003,7 +1000,7 @@ func TestStartSpanAfterEnd(t *testing.T) { func TestChildSpanCount(t *testing.T) { te := NewTestExporter() - tp := NewTracerProvider(WithConfig(Config{DefaultSampler: AlwaysSample()}), WithSyncer(te)) + tp := NewTracerProvider(WithDefaultSampler(AlwaysSample()), WithSyncer(te)) tr := tp.Tracer("ChidSpanCount") ctx, span0 := tr.Start(context.Background(), "parent") @@ -1057,7 +1054,7 @@ func TestNilSpanEnd(t *testing.T) { func TestExecutionTracerTaskEnd(t *testing.T) { var n uint64 - tp := NewTracerProvider(WithConfig(Config{DefaultSampler: NeverSample()})) + tp := NewTracerProvider(WithDefaultSampler(NeverSample())) tr := tp.Tracer("Execution Tracer Task End") executionTracerTaskEnd := func() { @@ -1106,7 +1103,7 @@ func TestExecutionTracerTaskEnd(t *testing.T) { func TestCustomStartEndTime(t *testing.T) { te := NewTestExporter() - tp := NewTracerProvider(WithSyncer(te), WithConfig(Config{DefaultSampler: AlwaysSample()})) + tp := NewTracerProvider(WithSyncer(te), WithDefaultSampler(AlwaysSample())) startTime := time.Date(2019, time.August, 27, 14, 42, 0, 0, time.UTC) endTime := startTime.Add(time.Second * 20) @@ -1220,7 +1217,7 @@ func TestRecordErrorNil(t *testing.T) { func TestWithSpanKind(t *testing.T) { te := NewTestExporter() - tp := NewTracerProvider(WithSyncer(te), WithConfig(Config{DefaultSampler: AlwaysSample()}), WithResource(resource.Empty())) + tp := NewTracerProvider(WithSyncer(te), WithDefaultSampler(AlwaysSample()), WithResource(resource.Empty())) tr := tp.Tracer("withSpanKind") _, span := tr.Start(context.Background(), "WithoutSpanKind") @@ -1290,7 +1287,7 @@ func TestWithResource(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { te := NewTestExporter() - defaultOptions := []TracerProviderOption{WithSyncer(te), WithConfig(Config{DefaultSampler: AlwaysSample()})} + defaultOptions := []TracerProviderOption{WithSyncer(te), WithDefaultSampler(AlwaysSample())} tp := NewTracerProvider(append(defaultOptions, tc.options...)...) span := startSpan(tp, "WithResource") span.SetAttributes(attribute.String("key1", "value1")) @@ -1497,8 +1494,11 @@ func TestReadWriteSpan(t *testing.T) { func TestAddEventsWithMoreAttributesThanLimit(t *testing.T) { te := NewTestExporter() - cfg := Config{SpanLimits: SpanLimits{AttributePerEventCountLimit: 2}} - tp := NewTracerProvider(WithConfig(cfg), WithSyncer(te), WithResource(resource.Empty())) + tp := NewTracerProvider( + WithSpanLimits(SpanLimits{AttributePerEventCountLimit: 2}), + WithSyncer(te), + WithResource(resource.Empty()), + ) span := startSpan(tp, "AddSpanEventWithOverLimitedAttributes") span.AddEvent("test1", trace.WithAttributes( @@ -1559,8 +1559,11 @@ func TestAddEventsWithMoreAttributesThanLimit(t *testing.T) { func TestAddLinksWithMoreAttributesThanLimit(t *testing.T) { te := NewTestExporter() - cfg := Config{SpanLimits: SpanLimits{AttributePerLinkCountLimit: 1}} - tp := NewTracerProvider(WithConfig(cfg), WithSyncer(te), WithResource(resource.Empty())) + tp := NewTracerProvider( + WithSpanLimits(SpanLimits{AttributePerLinkCountLimit: 1}), + WithSyncer(te), + WithResource(resource.Empty()), + ) k1v1 := attribute.String("key1", "value1") k2v2 := attribute.String("key2", "value2") @@ -1708,7 +1711,7 @@ func TestSamplerTraceState(t *testing.T) { ts := ts t.Run(ts.name, func(t *testing.T) { te := NewTestExporter() - tp := NewTracerProvider(WithConfig(Config{DefaultSampler: ts.sampler}), WithSyncer(te), WithResource(resource.Empty())) + tp := NewTracerProvider(WithDefaultSampler(ts.sampler), WithSyncer(te), WithResource(resource.Empty())) tr := tp.Tracer("TraceState") sc1 := trace.SpanContext{ diff --git a/sdk/trace/util_test.go b/sdk/trace/util_test.go index 4354199f5d2..d6aca0f5cee 100644 --- a/sdk/trace/util_test.go +++ b/sdk/trace/util_test.go @@ -20,9 +20,7 @@ import ( sdktrace "go.opentelemetry.io/otel/sdk/trace" ) -var testConfig = sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()} - func basicTracerProvider(t *testing.T) *sdktrace.TracerProvider { - tp := sdktrace.NewTracerProvider(sdktrace.WithConfig(testConfig)) + tp := sdktrace.NewTracerProvider(sdktrace.WithDefaultSampler(sdktrace.AlwaysSample())) return tp }