From c99bac0579379cc11b311f9455b7ecfd230cb839 Mon Sep 17 00:00:00 2001 From: Flc Date: Tue, 3 Dec 2024 09:01:50 +0800 Subject: [PATCH] refactor(otelgin): Modify span name formatter function signature and default implementation --- CHANGELOG.md | 1 + .../gin-gonic/gin/otelgin/gintrace.go | 10 ++-- .../gin-gonic/gin/otelgin/option.go | 31 ++++++++++-- .../gin/otelgin/test/gintrace_test.go | 47 ++++++++++++++----- 4 files changed, 66 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e61f0af8f6b..85bca7eb9a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Change the span name to be `GET /path` so it complies with the OTel HTTP semantic conventions in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) - Record errors instead of setting the `gin.errors` attribute in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#6346) +- Change the default span name to be `GET /path` so it complies with the OTel HTTP semantic conventions, and set `spanNameFormatter` to a non-public property in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#6381) ### Fixed diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go b/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go index 3a676586212..ebec0c44144 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go @@ -43,6 +43,9 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc { if cfg.Propagators == nil { cfg.Propagators = otel.GetTextMapPropagator() } + if cfg.spanNameFormatter == nil { + cfg.spanNameFormatter = defaultSpanNameFormatter + } return func(c *gin.Context) { for _, f := range cfg.Filters { if !f(c.Request) { @@ -71,12 +74,7 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc { oteltrace.WithAttributes(semconv.HTTPRoute(c.FullPath())), oteltrace.WithSpanKind(oteltrace.SpanKindServer), } - var spanName string - if cfg.SpanNameFormatter == nil { - spanName = c.FullPath() - } else { - spanName = cfg.SpanNameFormatter(c.Request) - } + spanName := cfg.spanNameFormatter(c.FullPath(), c.Request) if spanName == "" { spanName = fmt.Sprintf("HTTP %s route not found", c.Request.Method) } diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/option.go b/instrumentation/github.com/gin-gonic/gin/otelgin/option.go index 143ca8e849e..fca22d20b44 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/option.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/option.go @@ -7,6 +7,8 @@ package otelgin // import "go.opentelemetry.io/contrib/instrumentation/github.co import ( "net/http" + "slices" + "strings" "github.com/gin-gonic/gin" @@ -14,12 +16,31 @@ import ( oteltrace "go.opentelemetry.io/otel/trace" ) +var defaultSpanNameFormatter = func(path string, r *http.Request) string { + method := strings.ToUpper(r.Method) + if !slices.Contains([]string{ + http.MethodGet, http.MethodHead, + http.MethodPost, http.MethodPut, + http.MethodPatch, http.MethodDelete, + http.MethodConnect, http.MethodOptions, + http.MethodTrace, + }, method) { + method = "HTTP" + } + + if path != "" { + return method + " " + path + } + + return method +} + type config struct { TracerProvider oteltrace.TracerProvider Propagators propagation.TextMapPropagator Filters []Filter GinFilters []GinFilter - SpanNameFormatter SpanNameFormatter + spanNameFormatter SpanNameFormatter } // Filter is a predicate used to determine whether a given http.request should @@ -31,7 +52,7 @@ type Filter func(*http.Request) bool type GinFilter func(*gin.Context) bool // SpanNameFormatter is used to set span name by http.request. -type SpanNameFormatter func(r *http.Request) string +type SpanNameFormatter func(path string, r *http.Request) string // Option specifies instrumentation configuration options. type Option interface { @@ -86,8 +107,10 @@ func WithGinFilter(f ...GinFilter) Option { // WithSpanNameFormatter takes a function that will be called on every // request and the returned string will become the Span Name. -func WithSpanNameFormatter(f func(r *http.Request) string) Option { +func WithSpanNameFormatter(f func(path string, r *http.Request) string) Option { return optionFunc(func(c *config) { - c.SpanNameFormatter = f + if f != nil { + c.spanNameFormatter = f + } }) } diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go b/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go index f63d1955f0e..72d3a803879 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go @@ -7,6 +7,7 @@ package test import ( "errors" + "fmt" "html/template" "net/http" "net/http/httptest" @@ -157,7 +158,7 @@ func TestTrace200(t *testing.T) { spans := sr.Ended() require.Len(t, spans, 1) span := spans[0] - assert.Equal(t, "/user/:id", span.Name()) + assert.Equal(t, "GET /user/:id", span.Name()) assert.Equal(t, trace.SpanKindServer, span.SpanKind()) attr := span.Attributes() assert.Contains(t, attr, attribute.String("net.host.name", "foobar")) @@ -193,7 +194,7 @@ func TestError(t *testing.T) { spans := sr.Ended() require.Len(t, spans, 1) span := spans[0] - assert.Equal(t, "/server_err", span.Name()) + assert.Equal(t, "GET /server_err", span.Name()) attr := span.Attributes() assert.Contains(t, attr, attribute.String("net.host.name", "foobar")) assert.Contains(t, attr, attribute.Int("http.status_code", http.StatusInternalServerError)) @@ -263,27 +264,47 @@ func TestSpanStatus(t *testing.T) { } func TestSpanName(t *testing.T) { + imsb := tracetest.NewInMemoryExporter() + provider := sdktrace.NewTracerProvider( + sdktrace.WithSyncer(imsb), + ) + testCases := []struct { + method string + route string requestPath string spanNameFormatter otelgin.SpanNameFormatter wantSpanName string }{ - {"/user/1", nil, "/user/:id"}, - {"/user/1", func(r *http.Request) string { return r.URL.Path }, "/user/1"}, + // Test for standard methods + {http.MethodGet, "/user/:id", "/user/1", nil, "GET /user/:id"}, + {http.MethodPost, "/user/:id", "/user/1", nil, "POST /user/:id"}, + {http.MethodPut, "/user/:id", "/user/1", nil, "PUT /user/:id"}, + {http.MethodPatch, "/user/:id", "/user/1", nil, "PATCH /user/:id"}, + {http.MethodDelete, "/user/:id", "/user/1", nil, "DELETE /user/:id"}, + {http.MethodConnect, "/user/:id", "/user/1", nil, "CONNECT /user/:id"}, + {http.MethodOptions, "/user/:id", "/user/1", nil, "OPTIONS /user/:id"}, + {http.MethodTrace, "/user/:id", "/user/1", nil, "TRACE /user/:id"}, + // Test for no route + {http.MethodGet, "", "/user/1", nil, "GET"}, + // Test for invalid method + {"INVALID", "/user/:id", "/user/1", nil, "HTTP /user/:id"}, + // Test for custom span name formatter + {http.MethodGet, "/user/:id", "/user/1", func(_ string, r *http.Request) string { return r.URL.Path }, "/user/1"}, } + for _, tc := range testCases { - t.Run(tc.requestPath, func(t *testing.T) { - sr := tracetest.NewSpanRecorder() - provider := sdktrace.NewTracerProvider() - provider.RegisterSpanProcessor(sr) + t.Run(fmt.Sprintf("method: %s, route: %s, requestPath: %s", tc.method, tc.route, tc.requestPath), func(t *testing.T) { + defer imsb.Reset() + router := gin.New() router.Use(otelgin.Middleware("foobar", otelgin.WithTracerProvider(provider), otelgin.WithSpanNameFormatter(tc.spanNameFormatter))) - router.GET("/user/:id", func(c *gin.Context) {}) + router.Handle(tc.method, tc.route, func(c *gin.Context) {}) - router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", tc.requestPath, nil)) + router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(tc.method, tc.requestPath, nil)) - require.Len(t, sr.Ended(), 1, "should emit a span") - assert.Equal(t, tc.wantSpanName, sr.Ended()[0].Name(), "span name not correct") + require.Len(t, imsb.GetSpans(), 1, "should emit a span") + assert.Equal(t, tc.wantSpanName, imsb.GetSpans()[0].Name, "span name not correct") }) } } @@ -295,7 +316,7 @@ func TestHTTPRouteWithSpanNameFormatter(t *testing.T) { router := gin.New() router.Use(otelgin.Middleware("foobar", otelgin.WithTracerProvider(provider), - otelgin.WithSpanNameFormatter(func(r *http.Request) string { + otelgin.WithSpanNameFormatter(func(_ string, r *http.Request) string { return r.URL.Path }), ),