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..4910e850ce2 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go @@ -6,7 +6,9 @@ package otelgin // import "go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin" import ( - "fmt" + "net/http" + "slices" + "strings" "github.com/gin-gonic/gin" @@ -71,15 +73,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) - } - if spanName == "" { - spanName = fmt.Sprintf("HTTP %s route not found", c.Request.Method) - } + spanName := spanNameFormatter(c.FullPath(), c.Request) ctx, span := tracer.Start(ctx, spanName, opts...) defer span.End() @@ -128,3 +122,22 @@ func HTML(c *gin.Context, code int, name string, obj interface{}) { defer span.End() c.HTML(code, name, obj) } + +func spanNameFormatter(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 +} diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/option.go b/instrumentation/github.com/gin-gonic/gin/otelgin/option.go index 143ca8e849e..bf1f383a2bb 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/option.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/option.go @@ -15,11 +15,10 @@ import ( ) type config struct { - TracerProvider oteltrace.TracerProvider - Propagators propagation.TextMapPropagator - Filters []Filter - GinFilters []GinFilter - SpanNameFormatter SpanNameFormatter + TracerProvider oteltrace.TracerProvider + Propagators propagation.TextMapPropagator + Filters []Filter + GinFilters []GinFilter } // Filter is a predicate used to determine whether a given http.request should @@ -30,9 +29,6 @@ type Filter func(*http.Request) bool // gin.Context has FullPath() method, which returns a matched route full path. type GinFilter func(*gin.Context) bool -// SpanNameFormatter is used to set span name by http.request. -type SpanNameFormatter func(r *http.Request) string - // Option specifies instrumentation configuration options. type Option interface { apply(*config) @@ -83,11 +79,3 @@ func WithGinFilter(f ...GinFilter) Option { c.GinFilters = append(c.GinFilters, f...) }) } - -// 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 { - return optionFunc(func(c *config) { - 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..88d7e5368d4 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,44 @@ func TestSpanStatus(t *testing.T) { } func TestSpanName(t *testing.T) { + imsb := tracetest.NewInMemoryExporter() + provider := sdktrace.NewTracerProvider( + sdktrace.WithSyncer(imsb), + ) + testCases := []struct { - requestPath string - spanNameFormatter otelgin.SpanNameFormatter - wantSpanName string + method string + route string + requestPath string + 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", "GET /user/:id"}, + {http.MethodPost, "/user/:id", "/user/1", "POST /user/:id"}, + {http.MethodPut, "/user/:id", "/user/1", "PUT /user/:id"}, + {http.MethodPatch, "/user/:id", "/user/1", "PATCH /user/:id"}, + {http.MethodDelete, "/user/:id", "/user/1", "DELETE /user/:id"}, + {http.MethodConnect, "/user/:id", "/user/1", "CONNECT /user/:id"}, + {http.MethodOptions, "/user/:id", "/user/1", "OPTIONS /user/:id"}, + {http.MethodTrace, "/user/:id", "/user/1", "TRACE /user/:id"}, + // Test for no route + {http.MethodGet, "", "/user/1", "GET"}, + // Test for invalid method + {"INVALID", "/user/:id", "/user/1", "HTTP /user/:id"}, } + 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.Use(otelgin.Middleware("foobar", otelgin.WithTracerProvider(provider))) + 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,9 +313,6 @@ func TestHTTPRouteWithSpanNameFormatter(t *testing.T) { router := gin.New() router.Use(otelgin.Middleware("foobar", otelgin.WithTracerProvider(provider), - otelgin.WithSpanNameFormatter(func(r *http.Request) string { - return r.URL.Path - }), ), ) router.GET("/user/:id", func(c *gin.Context) {