From 97f311484668f7153e669413734546a03bf420a9 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 6 Oct 2020 08:57:24 -0700 Subject: [PATCH] Update gopkg.in/macaron.v1/otelmacaron instrumentation to use TracerProvider (#374) * Update gopkg.in/macaron.v1/otelmacaron inst to use TracerProvider * Add changes to changelog --- CHANGELOG.md | 4 + .../gopkg.in/macaron.v1/otelmacaron/config.go | 58 +++++++---- .../macaron.v1/otelmacaron/macaron.go | 25 ++--- .../macaron.v1/otelmacaron/macaron_test.go | 96 ++++++++----------- 4 files changed, 92 insertions(+), 91 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25073b7b422..f73b2346892 100644 --- 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 index 76fbd9836cf..6e64f621202 100644 --- 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 index 8b772f9f041..a058c977aba 100644 --- 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 index 4a2891968e8..b978a43ffd7 100644 --- 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,13 +128,13 @@ 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) }) @@ -161,7 +142,8 @@ func TestPropagationWithGlobalPropagators(t *testing.T) { } 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) })