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
open-telemetry · Sep 9, 2020 · a2c75c6 · a2c75c6
1 parent f1dad21
commit a2c75c6
Show file tree

Hide file tree

Showing 12 changed files with 93 additions and 44 deletions.
diff --git 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
@@ -19,24 +19,25 @@ import (
 
 	"go.opentelemetry.io/otel/api/global"
 	"go.opentelemetry.io/otel/api/trace"
+	"go.opentelemetry.io/otel/internal/trace/noop"
 )
 
 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)