fix(otelecho): comply with span naming semconv (#6365)

from:
#6363

This change adjusts the default span name and does not support backward
compatibility.

---------

Co-authored-by: Damien Mathieu <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>

Author: 3 people

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

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
+}

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) {

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)
+		})
+	}
+}