Update gopkg.in/macaron.v1/otelmacaron instrumentation to use TracerProvider (#374)

MrAlias · web-flow · commit 97f311484668 · 2020-10-06T08:57:24.000-07:00
* Update gopkg.in/macaron.v1/otelmacaron inst to use TracerProvider

* Add changes to changelog
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -21,6 +21,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [0.12.0] - 2020-09-25
 
+### Changed
+
+- Replace `WithTracer` with `WithTracerProvider` in the `go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron` instrumentation. (#374)
+
 ### Added
 
 - Benchmark tests for the gRPC instrumentation. (#296)
diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/config.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/config.go
@@ -15,34 +15,54 @@
 package otelmacaron
 
 import (
+	"go.opentelemetry.io/otel/api/global"
 	"go.opentelemetry.io/otel/api/propagation"
 	"go.opentelemetry.io/otel/api/trace"
 )
 
-// config is a helper struct to configure Macaron options
+// config is a group of options for this instrumentation.
 type config struct {
-	Tracer      trace.Tracer
-	Propagators propagation.Propagators
+	TracerProvider trace.TracerProvider
+	Propagators    propagation.Propagators
 }
 
-// Option specifies instrumentation configuration options.
-type Option func(*config)
+// Option applies an option value for a config.
+type Option interface {
+	Apply(*config)
+}
 
-// WithTracer specifies a tracer to use for creating spans. If none is
-// specified, a tracer named
-// "go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron"
-// from the global provider is used.
-func WithTracer(tracer trace.Tracer) Option {
-	return func(cfg *config) {
-		cfg.Tracer = tracer
+// newConfig returns a config configured with all the passed Options.
+func newConfig(opts []Option) *config {
+	c := &config{
+		Propagators:    global.Propagators(),
+		TracerProvider: global.TracerProvider(),
+	}
+	for _, o := range opts {
+		o.Apply(c)
 	}
+	return c
 }
 
-// WithPropagators specifies propagators to use for extracting
-// information from the HTTP requests. If none are specified, global
-// ones will be used.
-func WithPropagators(propagators propagation.Propagators) Option {
-	return func(cfg *config) {
-		cfg.Propagators = propagators
-	}
+type propagatorsOption struct{ p propagation.Propagators }
+
+func (o propagatorsOption) Apply(c *config) {
+	c.Propagators = o.p
+}
+
+// WithPropagators returns an Option to use the Propagators when extracting
+// and injecting trace context from HTTP requests.
+func WithPropagators(p propagation.Propagators) Option {
+	return propagatorsOption{p: p}
+}
+
+type tracerProviderOption struct{ tp trace.TracerProvider }
+
+func (o tracerProviderOption) Apply(c *config) {
+	c.TracerProvider = o.tp
+}
+
+// WithTracerProvider returns an Option to use the TracerProvider when
+// creating a Tracer.
+func WithTracerProvider(tp trace.TracerProvider) Option {
+	return tracerProviderOption{tp: tp}
 }
diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go
@@ -20,28 +20,23 @@ import (
 
 	"gopkg.in/macaron.v1"
 
-	otelglobal "go.opentelemetry.io/otel/api/global"
 	otelpropagation "go.opentelemetry.io/otel/api/propagation"
 	oteltrace "go.opentelemetry.io/otel/api/trace"
 	"go.opentelemetry.io/otel/semconv"
-)
 
-const (
-	tracerName = "go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron"
+	otelcontrib "go.opentelemetry.io/contrib"
 )
 
+// instrumentationName is the name of this instrumentation package.
+const instrumentationName = "go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron"
+
 // Middleware returns a macaron Handler to trace requests to the server.
 func Middleware(service string, opts ...Option) macaron.Handler {
-	cfg := config{}
-	for _, opt := range opts {
-		opt(&cfg)
-	}
-	if cfg.Tracer == nil {
-		cfg.Tracer = otelglobal.Tracer(tracerName)
-	}
-	if cfg.Propagators == nil {
-		cfg.Propagators = otelglobal.Propagators()
-	}
+	cfg := newConfig(opts)
+	tracer := cfg.TracerProvider.Tracer(
+		instrumentationName,
+		oteltrace.WithInstrumentationVersion(otelcontrib.SemVersion()),
+	)
 	return func(res http.ResponseWriter, req *http.Request, c *macaron.Context) {
 		savedCtx := c.Req.Request.Context()
 		defer func() {
@@ -60,7 +55,7 @@ func Middleware(service string, opts ...Option) macaron.Handler {
 		if spanName == "" {
 			spanName = fmt.Sprintf("HTTP %s route not found", c.Req.Method)
 		}
-		ctx, span := cfg.Tracer.Start(ctx, spanName, opts...)
+		ctx, span := tracer.Start(ctx, spanName, opts...)
 		defer span.End()
 
 		// pass the span through the request context
diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron_test.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron_test.go
@@ -24,49 +24,27 @@ import (
 	"github.com/stretchr/testify/require"
 	"gopkg.in/macaron.v1"
 
-	mocktrace "go.opentelemetry.io/contrib/internal/trace"
 	b3prop "go.opentelemetry.io/contrib/propagators/b3"
 	otelglobal "go.opentelemetry.io/otel/api/global"
 	otelpropagation "go.opentelemetry.io/otel/api/propagation"
 	oteltrace "go.opentelemetry.io/otel/api/trace"
+	"go.opentelemetry.io/otel/api/trace/tracetest"
 	"go.opentelemetry.io/otel/label"
 )
 
 func TestChildSpanFromGlobalTracer(t *testing.T) {
-	otelglobal.SetTracerProvider(&mocktrace.TracerProvider{})
+	otelglobal.SetTracerProvider(tracetest.NewTracerProvider())
 
 	m := macaron.Classic()
 	m.Use(Middleware("foobar"))
 	m.Get("/user/:id", func(ctx *macaron.Context) {
 		span := oteltrace.SpanFromContext(ctx.Req.Request.Context())
-		_, ok := span.(*mocktrace.Span)
+		_, ok := span.(*tracetest.Span)
 		assert.True(t, ok)
 		spanTracer := span.Tracer()
-		mockTracer, ok := spanTracer.(*mocktrace.Tracer)
+		mockTracer, ok := spanTracer.(*tracetest.Tracer)
 		require.True(t, ok)
-		assert.Equal(t, "go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron", mockTracer.Name)
-		ctx.Resp.WriteHeader(http.StatusOK)
-	})
-
-	r := httptest.NewRequest("GET", "/user/123", nil)
-	w := httptest.NewRecorder()
-
-	m.ServeHTTP(w, r)
-}
-
-func TestChildSpanFromCustomTracer(t *testing.T) {
-	tracer := mocktrace.NewTracer("test-tracer")
-
-	m := macaron.Classic()
-	m.Use(Middleware("foobar", WithTracer(tracer)))
-	m.Get("/user/:id", func(ctx *macaron.Context) {
-		span := oteltrace.SpanFromContext(ctx.Req.Request.Context())
-		_, ok := span.(*mocktrace.Span)
-		assert.True(t, ok)
-		spanTracer := span.Tracer()
-		mockTracer, ok := spanTracer.(*mocktrace.Tracer)
-		require.True(t, ok)
-		assert.Equal(t, "test-tracer", mockTracer.Name)
+		assert.Equal(t, instrumentationName, mockTracer.Name)
 		ctx.Resp.WriteHeader(http.StatusOK)
 	})
 
@@ -77,10 +55,11 @@ func TestChildSpanFromCustomTracer(t *testing.T) {
 }
 
 func TestChildSpanNames(t *testing.T) {
-	tracer := mocktrace.NewTracer("test-tracer")
+	sr := new(tracetest.StandardSpanRecorder)
+	tp := tracetest.NewTracerProvider(tracetest.WithSpanRecorder(sr))
 
 	m := macaron.Classic()
-	m.Use(Middleware("foobar", WithTracer(tracer)))
+	m.Use(Middleware("foobar", WithTracerProvider(tp)))
 	m.Get("/user/:id", func(ctx *macaron.Context) {
 		ctx.Resp.WriteHeader(http.StatusOK)
 	})
@@ -94,30 +73,32 @@ func TestChildSpanNames(t *testing.T) {
 	r := httptest.NewRequest("GET", "/user/123", nil)
 	w := httptest.NewRecorder()
 	m.ServeHTTP(w, r)
-	spans := tracer.EndedSpans()
-	require.Len(t, spans, 1)
-	span := spans[0]
-	assert.Equal(t, "/user/123", span.Name) // TODO: span name should show router template, eg /user/:id
-	assert.Equal(t, oteltrace.SpanKindServer, span.Kind)
-	assert.Equal(t, label.StringValue("foobar"), span.Attributes["http.server_name"])
-	assert.Equal(t, label.IntValue(http.StatusOK), span.Attributes["http.status_code"])
-	assert.Equal(t, label.StringValue("GET"), span.Attributes["http.method"])
-	assert.Equal(t, label.StringValue("/user/123"), span.Attributes["http.target"])
-	// TODO: span name should show router template, eg /user/:id
-	//assert.Equal(t, label.StringValue("/user/:id"), span.Attributes["http.route"])
 
 	r = httptest.NewRequest("GET", "/book/foo", nil)
 	w = httptest.NewRecorder()
 	m.ServeHTTP(w, r)
-	spans = tracer.EndedSpans()
-	require.Len(t, spans, 1)
-	span = spans[0]
-	assert.Equal(t, "/book/foo", span.Name) // TODO: span name should show router template, eg /book/:title
-	assert.Equal(t, oteltrace.SpanKindServer, span.Kind)
-	assert.Equal(t, label.StringValue("foobar"), span.Attributes["http.server_name"])
-	assert.Equal(t, label.IntValue(http.StatusOK), span.Attributes["http.status_code"])
-	assert.Equal(t, label.StringValue("GET"), span.Attributes["http.method"])
-	assert.Equal(t, label.StringValue("/book/foo"), span.Attributes["http.target"])
+
+	spans := sr.Completed()
+	require.Len(t, spans, 2)
+	span := spans[0]
+	assert.Equal(t, "/user/123", span.Name()) // TODO: span name should show router template, eg /user/:id
+	assert.Equal(t, oteltrace.SpanKindServer, span.SpanKind())
+	attrs := span.Attributes()
+	assert.Equal(t, label.StringValue("foobar"), attrs["http.server_name"])
+	assert.Equal(t, label.IntValue(http.StatusOK), attrs["http.status_code"])
+	assert.Equal(t, label.StringValue("GET"), attrs["http.method"])
+	assert.Equal(t, label.StringValue("/user/123"), attrs["http.target"])
+	// TODO: span name should show router template, eg /user/:id
+	//assert.Equal(t, label.StringValue("/user/:id"), span.Attributes["http.route"])
+
+	span = spans[1]
+	assert.Equal(t, "/book/foo", span.Name()) // TODO: span name should show router template, eg /book/:title
+	assert.Equal(t, oteltrace.SpanKindServer, span.SpanKind())
+	attrs = span.Attributes()
+	assert.Equal(t, label.StringValue("foobar"), attrs["http.server_name"])
+	assert.Equal(t, label.IntValue(http.StatusOK), attrs["http.status_code"])
+	assert.Equal(t, label.StringValue("GET"), attrs["http.method"])
+	assert.Equal(t, label.StringValue("/book/foo"), attrs["http.target"])
 	// TODO: span name should show router template, eg /book/:title
 	//assert.Equal(t, label.StringValue("/book/:title"), span.Attributes["http.route"])
 }
@@ -138,7 +119,7 @@ func TestGetSpanNotInstrumented(t *testing.T) {
 }
 
 func TestPropagationWithGlobalPropagators(t *testing.T) {
-	tracer := mocktrace.NewTracer("test-tracer")
+	tracer := tracetest.NewTracerProvider().Tracer("test-tracer")
 
 	r := httptest.NewRequest("GET", "/user/123", nil)
 	w := httptest.NewRecorder()
@@ -147,21 +128,22 @@ func TestPropagationWithGlobalPropagators(t *testing.T) {
 	otelpropagation.InjectHTTP(ctx, otelglobal.Propagators(), r.Header)
 
 	m := macaron.Classic()
-	m.Use(Middleware("foobar", WithTracer(tracer)))
+	m.Use(Middleware("foobar"))
 	m.Get("/user/:id", func(ctx *macaron.Context) {
 		span := oteltrace.SpanFromContext(ctx.Req.Request.Context())
-		mspan, ok := span.(*mocktrace.Span)
+		mspan, ok := span.(*tracetest.Span)
 		require.True(t, ok)
 		assert.Equal(t, pspan.SpanContext().TraceID, mspan.SpanContext().TraceID)
-		assert.Equal(t, pspan.SpanContext().SpanID, mspan.ParentSpanID)
+		assert.Equal(t, pspan.SpanContext().SpanID, mspan.ParentSpanID())
 		ctx.Resp.WriteHeader(http.StatusOK)
 	})
 
 	m.ServeHTTP(w, r)
 }
 
 func TestPropagationWithCustomPropagators(t *testing.T) {
-	tracer := mocktrace.NewTracer("test-tracer")
+	tp := tracetest.NewTracerProvider()
+	tracer := tp.Tracer("test-tracer")
 	b3 := b3prop.B3{}
 	props := otelpropagation.New(
 		otelpropagation.WithExtractors(b3),
@@ -175,13 +157,13 @@ func TestPropagationWithCustomPropagators(t *testing.T) {
 	otelpropagation.InjectHTTP(ctx, props, r.Header)
 
 	m := macaron.Classic()
-	m.Use(Middleware("foobar", WithTracer(tracer), WithPropagators(props)))
+	m.Use(Middleware("foobar", WithTracerProvider(tp), WithPropagators(props)))
 	m.Get("/user/:id", func(ctx *macaron.Context) {
 		span := oteltrace.SpanFromContext(ctx.Req.Request.Context())
-		mspan, ok := span.(*mocktrace.Span)
+		mspan, ok := span.(*tracetest.Span)
 		require.True(t, ok)
 		assert.Equal(t, pspan.SpanContext().TraceID, mspan.SpanContext().TraceID)
-		assert.Equal(t, pspan.SpanContext().SpanID, mspan.ParentSpanID)
+		assert.Equal(t, pspan.SpanContext().SpanID, mspan.ParentSpanID())
 		ctx.Resp.WriteHeader(http.StatusOK)
 	})
 
