diff --git a/CHANGELOG.md b/CHANGELOG.md index af73b4f1a18..2a8ae39822b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed +- 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) ### Fixed diff --git a/instrumentation/github.com/labstack/echo/otelecho/echo.go b/instrumentation/github.com/labstack/echo/otelecho/echo.go index 2771ebc5d9e..bdbbef24d80 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/echo.go +++ b/instrumentation/github.com/labstack/echo/otelecho/echo.go @@ -4,14 +4,15 @@ package otelecho // import "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" import ( - "fmt" + "net/http" + "slices" + "strings" "github.com/labstack/echo/v4" "github.com/labstack/echo/v4/middleware" - "go.opentelemetry.io/otel" - "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/propagation" semconv "go.opentelemetry.io/otel/semconv/v1.20.0" @@ -67,10 +68,7 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { rAttr := semconv.HTTPRoute(path) opts = append(opts, oteltrace.WithAttributes(rAttr)) } - spanName := c.Path() - if spanName == "" { - spanName = fmt.Sprintf("HTTP %s route not found", request.Method) - } + spanName := spanNameFormatter(c) ctx, span := tracer.Start(ctx, spanName, opts...) defer span.End() @@ -96,3 +94,22 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { } } } + +func spanNameFormatter(c echo.Context) string { + method, path := strings.ToUpper(c.Request().Method), c.Path() + 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/labstack/echo/otelecho/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/echo_test.go index 9e424a617c3..4669521d53d 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/echo_test.go @@ -14,12 +14,11 @@ import ( "github.com/labstack/echo/v4" "github.com/stretchr/testify/assert" + b3prop "go.opentelemetry.io/contrib/propagators/b3" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" "go.opentelemetry.io/otel/trace/noop" - - b3prop "go.opentelemetry.io/contrib/propagators/b3" ) func TestGetSpanNotInstrumented(t *testing.T) { diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index 905bb63dbd7..33487974c56 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -12,17 +12,15 @@ import ( "testing" "github.com/labstack/echo/v4" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" - - "go.opentelemetry.io/otel/attribute" oteltrace "go.opentelemetry.io/otel/trace" ) @@ -86,7 +84,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, oteltrace.SpanKindServer, span.SpanKind()) attrs := span.Attributes() assert.Contains(t, attrs, attribute.String("net.host.name", "foobar")) @@ -118,7 +116,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()) attrs := span.Attributes() assert.Contains(t, attrs, attribute.String("net.host.name", "foobar")) assert.Contains(t, attrs, attribute.Int("http.status_code", http.StatusInternalServerError)) @@ -177,7 +175,7 @@ func TestStatusError(t *testing.T) { spans := sr.Ended() require.Len(t, spans, 1) span := spans[0] - assert.Equal(t, "/err", span.Name()) + assert.Equal(t, "GET /err", span.Name()) assert.Equal(t, tc.spanCode, span.Status().Code) attrs := span.Attributes() @@ -202,3 +200,59 @@ func TestErrorNotSwallowedByMiddleware(t *testing.T) { err := h(c) assert.Equal(t, assert.AnError, err) } + +func TestSpanNameFormatter(t *testing.T) { + imsb := tracetest.NewInMemoryExporter() + provider := trace.NewTracerProvider(trace.WithSyncer(imsb)) + + tests := []struct { + name string + method string + path string + url string + expected string + }{ + // Test for standard methods + {"standard method of GET", http.MethodGet, "/user/:id", "/user/123", "GET /user/:id"}, + {"standard method of HEAD", http.MethodHead, "/user/:id", "/user/123", "HEAD /user/:id"}, + {"standard method of POST", http.MethodPost, "/user/:id", "/user/123", "POST /user/:id"}, + {"standard method of PUT", http.MethodPut, "/user/:id", "/user/123", "PUT /user/:id"}, + {"standard method of PATCH", http.MethodPatch, "/user/:id", "/user/123", "PATCH /user/:id"}, + {"standard method of DELETE", http.MethodDelete, "/user/:id", "/user/123", "DELETE /user/:id"}, + {"standard method of CONNECT", http.MethodConnect, "/user/:id", "/user/123", "CONNECT /user/:id"}, + {"standard method of OPTIONS", http.MethodOptions, "/user/:id", "/user/123", "OPTIONS /user/:id"}, + {"standard method of TRACE", http.MethodTrace, "/user/:id", "/user/123", "TRACE /user/:id"}, + {"standard method of GET, but it's another route.", http.MethodGet, "/", "/", "GET /"}, + + // Test for no route + {"no route", http.MethodGet, "/", "/user/id", "GET"}, + + // Test for case-insensitive method + {"all lowercase", "get", "/user/123", "/user/123", "GET /user/123"}, + {"partial capitalization", "Get", "/user/123", "/user/123", "GET /user/123"}, + {"full capitalization", "GET", "/user/:id", "/user/123", "GET /user/:id"}, + + // Test for invalid method + {"invalid method", "INVALID", "/user/123", "/user/123", "HTTP /user/123"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + defer imsb.Reset() + + router := echo.New() + router.Use(otelecho.Middleware("foobar", otelecho.WithTracerProvider(provider))) + router.Add(test.method, test.path, func(c echo.Context) error { + return c.NoContent(http.StatusOK) + }) + + r := httptest.NewRequest(test.method, test.url, nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + + spans := imsb.GetSpans() + require.Len(t, spans, 1) + assert.Equal(t, test.expected, spans[0].Name) + }) + } +}