refactor(otelgin): Modify span name formatter function signature and default implementation

Author: flc1125

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
 

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

instrumentation/github.com/gin-gonic/gin/otelgin/option.go

@@ -7,19 +7,40 @@ package otelgin // import "go.opentelemetry.io/contrib/instrumentation/github.co
 
 import (
 	"net/http"
+	"slices"
+	"strings"
 
 	"github.com/gin-gonic/gin"
 
 	"go.opentelemetry.io/otel/propagation"
 	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
+		}
 	})
 }

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