From a2c75c6d783399049e2368ab6acef2d8dcdb3e77 Mon Sep 17 00:00:00 2001 From: JBD Date: Tue, 8 Sep 2020 17:43:35 -0700 Subject: [PATCH] Unexport NoopXXX trace types (#1134) * Unexport NoopXXX trace types The change unexports the noop implementations and provide the NoopProvider function for user to construct noop providers. Users can access noop tracer and noop spans by using the provider. This change removes the types users should never be directly using from the package. It improves the usability of the API by reducing the API surface to half and helping the user to focus on the canonical APIs. Fixes #1133 * Provide noop tracer and span for internal use * Remove obsolete doc * Use noop span instead of nil * Fix the broken build --- api/global/internal/trace.go | 3 ++- api/global/trace_test.go | 9 ++++--- api/trace/context.go | 2 +- api/trace/context_test.go | 10 ++++---- api/trace/noop_span.go | 30 +++++++++++------------ api/trace/noop_trace.go | 8 +++--- api/trace/noop_trace_provider.go | 15 +++++++++--- bridge/opentracing/bridge.go | 9 ++++--- exporters/trace/jaeger/jaeger.go | 2 +- exporters/trace/jaeger/jaeger_test.go | 6 ++--- internal/trace/noop/noop.go | 35 +++++++++++++++++++++++++++ propagators/b3_integration_test.go | 8 ++++-- 12 files changed, 93 insertions(+), 44 deletions(-) create mode 100644 internal/trace/noop/noop.go diff --git a/api/global/internal/trace.go b/api/global/internal/trace.go index 9c97fc735..053c31c1e 100644 --- a/api/global/internal/trace.go +++ b/api/global/internal/trace.go @@ -36,6 +36,7 @@ import ( "sync" "go.opentelemetry.io/otel/api/trace" + "go.opentelemetry.io/otel/internal/trace/noop" ) // tracerProvider is a placeholder for a configured SDK Provider. @@ -120,5 +121,5 @@ func (t *tracer) Start(ctx context.Context, name string, opts ...trace.SpanOptio if t.delegate != nil { return t.delegate.Start(ctx, name, opts...) } - return trace.NoopTracer{}.Start(ctx, name, opts...) + return noop.Tracer.Start(ctx, name, opts...) } diff --git a/api/global/trace_test.go b/api/global/trace_test.go index 679bc9b9f..b66d172d4 100644 --- a/api/global/trace_test.go +++ b/api/global/trace_test.go @@ -19,6 +19,7 @@ import ( "go.opentelemetry.io/otel/api/global" "go.opentelemetry.io/otel/api/trace" + "go.opentelemetry.io/otel/internal/trace/noop" ) type testTracerProvider struct{} @@ -26,17 +27,17 @@ type testTracerProvider struct{} var _ trace.Provider = &testTracerProvider{} func (*testTracerProvider) Tracer(_ string, _ ...trace.TracerOption) trace.Tracer { - return &trace.NoopTracer{} + return noop.Tracer } func TestMultipleGlobalTracerProvider(t *testing.T) { p1 := testTracerProvider{} - p2 := trace.NoopProvider{} + p2 := trace.NoopProvider() global.SetTracerProvider(&p1) - global.SetTracerProvider(&p2) + global.SetTracerProvider(p2) got := global.TracerProvider() - want := &p2 + want := p2 if got != want { t.Fatalf("Provider: got %p, want %p\n", got, want) } diff --git a/api/trace/context.go b/api/trace/context.go index 489e3c120..0f330e3a0 100644 --- a/api/trace/context.go +++ b/api/trace/context.go @@ -36,7 +36,7 @@ func SpanFromContext(ctx context.Context) Span { if span, has := ctx.Value(currentSpanKey).(Span); has { return span } - return NoopSpan{} + return noopSpan{} } // ContextWithRemoteSpanContext creates a new context with a remote diff --git a/api/trace/context_test.go b/api/trace/context_test.go index 4af892926..cd9c108fe 100644 --- a/api/trace/context_test.go +++ b/api/trace/context_test.go @@ -21,14 +21,14 @@ import ( "go.opentelemetry.io/otel/api/trace" "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/internal/trace/noop" "go.opentelemetry.io/otel/label" ) func TestSetCurrentSpanOverridesPreviouslySetSpan(t *testing.T) { - originalSpan := trace.NoopSpan{} - expectedSpan := mockSpan{} - ctx := context.Background() + originalSpan := noop.Span + expectedSpan := mockSpan{} ctx = trace.ContextWithSpan(ctx, originalSpan) ctx = trace.ContextWithSpan(ctx, expectedSpan) @@ -47,7 +47,7 @@ func TestCurrentSpan(t *testing.T) { { name: "CurrentSpan() returns a NoopSpan{} from an empty context", ctx: context.Background(), - want: trace.NoopSpan{}, + want: noop.Span, }, { name: "CurrentSpan() returns current span if set", @@ -110,7 +110,7 @@ func (mockSpan) RecordError(ctx context.Context, err error, opts ...trace.ErrorO // Tracer returns noop implementation of Tracer. func (mockSpan) Tracer() trace.Tracer { - return trace.NoopTracer{} + return noop.Tracer } // Event does nothing. diff --git a/api/trace/noop_span.go b/api/trace/noop_span.go index d205daf88..510a0f0c3 100644 --- a/api/trace/noop_span.go +++ b/api/trace/noop_span.go @@ -22,58 +22,58 @@ import ( "go.opentelemetry.io/otel/label" ) -type NoopSpan struct { +type noopSpan struct { } -var _ Span = (*NoopSpan)(nil) +var _ Span = noopSpan{} // SpanContext returns an invalid span context. -func (NoopSpan) SpanContext() SpanContext { +func (noopSpan) SpanContext() SpanContext { return EmptySpanContext() } // IsRecording always returns false for NoopSpan. -func (NoopSpan) IsRecording() bool { +func (noopSpan) IsRecording() bool { return false } // SetStatus does nothing. -func (NoopSpan) SetStatus(status codes.Code, msg string) { +func (noopSpan) SetStatus(status codes.Code, msg string) { } // SetError does nothing. -func (NoopSpan) SetError(v bool) { +func (noopSpan) SetError(v bool) { } // SetAttributes does nothing. -func (NoopSpan) SetAttributes(attributes ...label.KeyValue) { +func (noopSpan) SetAttributes(attributes ...label.KeyValue) { } // SetAttribute does nothing. -func (NoopSpan) SetAttribute(k string, v interface{}) { +func (noopSpan) SetAttribute(k string, v interface{}) { } // End does nothing. -func (NoopSpan) End(options ...SpanOption) { +func (noopSpan) End(options ...SpanOption) { } // RecordError does nothing. -func (NoopSpan) RecordError(ctx context.Context, err error, opts ...ErrorOption) { +func (noopSpan) RecordError(ctx context.Context, err error, opts ...ErrorOption) { } // Tracer returns noop implementation of Tracer. -func (NoopSpan) Tracer() Tracer { - return NoopTracer{} +func (noopSpan) Tracer() Tracer { + return noopTracer{} } // AddEvent does nothing. -func (NoopSpan) AddEvent(ctx context.Context, name string, attrs ...label.KeyValue) { +func (noopSpan) AddEvent(ctx context.Context, name string, attrs ...label.KeyValue) { } // AddEventWithTimestamp does nothing. -func (NoopSpan) AddEventWithTimestamp(ctx context.Context, timestamp time.Time, name string, attrs ...label.KeyValue) { +func (noopSpan) AddEventWithTimestamp(ctx context.Context, timestamp time.Time, name string, attrs ...label.KeyValue) { } // SetName does nothing. -func (NoopSpan) SetName(name string) { +func (noopSpan) SetName(name string) { } diff --git a/api/trace/noop_trace.go b/api/trace/noop_trace.go index 9b67ea3d8..954f9e813 100644 --- a/api/trace/noop_trace.go +++ b/api/trace/noop_trace.go @@ -18,12 +18,12 @@ import ( "context" ) -type NoopTracer struct{} +type noopTracer struct{} -var _ Tracer = NoopTracer{} +var _ Tracer = noopTracer{} // Start starts a noop span. -func (NoopTracer) Start(ctx context.Context, name string, opts ...SpanOption) (context.Context, Span) { - span := NoopSpan{} +func (noopTracer) Start(ctx context.Context, name string, opts ...SpanOption) (context.Context, Span) { + span := noopSpan{} return ContextWithSpan(ctx, span), span } diff --git a/api/trace/noop_trace_provider.go b/api/trace/noop_trace_provider.go index 6a5f1a285..fc49da58a 100644 --- a/api/trace/noop_trace_provider.go +++ b/api/trace/noop_trace_provider.go @@ -14,11 +14,18 @@ package trace -type NoopProvider struct{} +type noopProvider struct{} -var _ Provider = NoopProvider{} +var _ Provider = noopProvider{} // Tracer returns noop implementation of Tracer. -func (p NoopProvider) Tracer(_ string, _ ...TracerOption) Tracer { - return NoopTracer{} +func (p noopProvider) Tracer(_ string, _ ...TracerOption) Tracer { + return noopTracer{} +} + +// NoopProvider returns a noop implementation of Provider. +// The Tracer and Spans created from the noop provider will +// also be noop. +func NoopProvider() Provider { + return noopProvider{} } diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 6fbe9bbac..267af4c54 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -30,6 +30,7 @@ import ( otelpropagation "go.opentelemetry.io/otel/api/propagation" oteltrace "go.opentelemetry.io/otel/api/trace" "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/internal/trace/noop" otelparent "go.opentelemetry.io/otel/internal/trace/parent" "go.opentelemetry.io/otel/label" @@ -300,7 +301,7 @@ var _ ot.TracerContextWithSpanExtension = &BridgeTracer{} func NewBridgeTracer() *BridgeTracer { return &BridgeTracer{ setTracer: bridgeSetTracer{ - otelTracer: oteltrace.NoopTracer{}, + otelTracer: noop.Tracer, }, warningHandler: func(msg string) {}, propagators: nil, @@ -579,8 +580,7 @@ func otSpanReferenceTypeToString(refType ot.SpanReferenceType) string { // fakeSpan is just a holder of span context, nothing more. It's for // propagators, so they can get the span context from Go context. type fakeSpan struct { - oteltrace.NoopSpan - + oteltrace.Span sc oteltrace.SpanContext } @@ -609,7 +609,8 @@ func (t *BridgeTracer) Inject(sm ot.SpanContext, format interface{}, carrier int } header := http.Header(hhcarrier) fs := fakeSpan{ - sc: bridgeSC.otelSpanContext, + Span: noop.Span, + sc: bridgeSC.otelSpanContext, } ctx := oteltrace.ContextWithSpan(context.Background(), fs) ctx = otelcorrelation.ContextWithMap(ctx, bridgeSC.baggageItems) diff --git a/exporters/trace/jaeger/jaeger.go b/exporters/trace/jaeger/jaeger.go index 001c269ae..2ea474553 100644 --- a/exporters/trace/jaeger/jaeger.go +++ b/exporters/trace/jaeger/jaeger.go @@ -156,7 +156,7 @@ func NewExportPipeline(endpointOption EndpointOption, opts ...Option) (apitrace. opt(&o) } if o.Disabled { - return &apitrace.NoopProvider{}, func() {}, nil + return apitrace.NoopProvider(), func() {}, nil } exporter, err := NewRawExporter(endpointOption, opts...) diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go index a25e08c97..e25bd2630 100644 --- a/exporters/trace/jaeger/jaeger_test.go +++ b/exporters/trace/jaeger/jaeger_test.go @@ -69,7 +69,7 @@ func TestInstallNewPipeline(t *testing.T) { options: []Option{ WithDisabled(true), }, - expectedProvider: &apitrace.NoopProvider{}, + expectedProvider: apitrace.NoopProvider(), }, } @@ -108,7 +108,7 @@ func TestNewExportPipeline(t *testing.T) { options: []Option{ WithDisabled(true), }, - expectedProviderType: &apitrace.NoopProvider{}, + expectedProviderType: apitrace.NoopProvider(), }, { name: "always on", @@ -173,7 +173,7 @@ func TestNewExportPipelineWithDisabledFromEnv(t *testing.T) { ) defer fn() assert.NoError(t, err) - assert.IsType(t, &apitrace.NoopProvider{}, tp) + assert.IsType(t, apitrace.NoopProvider(), tp) } func TestNewRawExporter(t *testing.T) { diff --git a/internal/trace/noop/noop.go b/internal/trace/noop/noop.go new file mode 100644 index 000000000..bc02db11a --- /dev/null +++ b/internal/trace/noop/noop.go @@ -0,0 +1,35 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package noop provides noop tracing implementations for tracer and span. +package noop + +import ( + "context" + + "go.opentelemetry.io/otel/api/trace" +) + +var ( + // Tracer is a noop tracer that starts noop spans. + Tracer trace.Tracer + + // Span is a noop Span. + Span trace.Span +) + +func init() { + Tracer = trace.NoopProvider().Tracer("") + _, Span = Tracer.Start(context.Background(), "") +} diff --git a/propagators/b3_integration_test.go b/propagators/b3_integration_test.go index 5846ce382..9675ab482 100644 --- a/propagators/b3_integration_test.go +++ b/propagators/b3_integration_test.go @@ -23,6 +23,7 @@ import ( "go.opentelemetry.io/otel/api/propagation" "go.opentelemetry.io/otel/api/trace" + "go.opentelemetry.io/otel/internal/trace/noop" "go.opentelemetry.io/otel/propagators" ) @@ -64,7 +65,7 @@ func TestExtractB3(t *testing.T) { } type testSpan struct { - trace.NoopSpan + trace.Span sc trace.SpanContext } @@ -94,7 +95,10 @@ func TestInjectB3(t *testing.T) { req, _ := http.NewRequest("GET", "http://example.com", nil) ctx := trace.ContextWithSpan( context.Background(), - testSpan{sc: tt.sc}, + testSpan{ + Span: noop.Span, + sc: tt.sc, + }, ) propagator.Inject(ctx, req.Header)