diff --git a/CHANGELOG.md b/CHANGELOG.md index 9116374fb34..4fecb966b82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add the `WithLoggerProviderOptions`, `WithMeterProviderOptions` and `WithTracerProviderOptions` options to `NewSDK` to allow passing custom options to providers in `go.opentelemetry.io/contrib/otelconf`. (#7552) - Added V2 version of AWS EC2 detector `go.opentelemetry.io/contrib/detectors/aws/ec2/v2` due to deprecation of `github.com/aws/aws-sdk-go`. (#6961) +### Changed + +- Change the default span name to be `GET /path` so it complies with the HTTP semantic conventions in `go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux`. (#7551) + ### Deprecated - `WithSpanOptions` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` is deprecated. diff --git a/instrumentation/github.com/gorilla/mux/otelmux/mux.go b/instrumentation/github.com/gorilla/mux/otelmux/mux.go index 8a229c9ab2c..553c482fea0 100644 --- a/instrumentation/github.com/gorilla/mux/otelmux/mux.go +++ b/instrumentation/github.com/gorilla/mux/otelmux/mux.go @@ -84,8 +84,27 @@ type traceware struct { metricAttributesFn func(*http.Request) []attribute.KeyValue } -// defaultSpanNameFunc just reuses the route name as the span name. -func defaultSpanNameFunc(routeName string, _ *http.Request) string { return routeName } +// validMethods are all the OTel recognized HTTP methods. +var validMethods = map[string]struct{}{ + http.MethodGet: {}, + http.MethodHead: {}, + http.MethodPost: {}, + http.MethodPut: {}, + http.MethodPatch: {}, + http.MethodDelete: {}, + http.MethodConnect: {}, + http.MethodOptions: {}, + http.MethodTrace: {}, +} + +// defaultSpanNameFunc returns the semconv based default span name. +func defaultSpanNameFunc(routeName string, r *http.Request) string { + method := r.Method + if _, ok := validMethods[method]; !ok { + method = "HTTP" + } + return method + " " + routeName +} // ServeHTTP implements the http.Handler interface. It does the actual // tracing of the request. @@ -113,17 +132,7 @@ func (tw traceware) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } - routeStr := r.Pattern - - if routeStr == "" { - route := mux.CurrentRoute(r) - if route != nil { - routeStr, _ = route.GetPathTemplate() - if routeStr == "" { - routeStr, _ = route.GetPathRegexp() - } - } - } + routeStr := extractRoute(r) if routeStr == "" { routeStr = fmt.Sprintf("HTTP %s route not found", r.Method) @@ -202,3 +211,15 @@ func (tw traceware) metricAttributesFromRequest(r *http.Request) []attribute.Key } return attributeForRequest } + +func extractRoute(r *http.Request) string { + routeStr := r.Pattern + + if routeStr == "" { + route := mux.CurrentRoute(r) + if route != nil { + routeStr, _ = route.GetPathTemplate() + } + } + return routeStr +} diff --git a/instrumentation/github.com/gorilla/mux/otelmux/muxtest_test.go b/instrumentation/github.com/gorilla/mux/otelmux/muxtest_test.go index 72f7f40092b..c56e18f99d4 100644 --- a/instrumentation/github.com/gorilla/mux/otelmux/muxtest_test.go +++ b/instrumentation/github.com/gorilla/mux/otelmux/muxtest_test.go @@ -8,6 +8,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/gorilla/mux" @@ -25,6 +26,38 @@ import ( "go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux" ) +func TestDefaultTrace(t *testing.T) { + sr := tracetest.NewSpanRecorder() + provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr)) + router := mux.NewRouter() + router.Use(otelmux.Middleware("foobar", otelmux.WithTracerProvider(provider))) + + router.HandleFunc("/user/{id}", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + r := httptest.NewRequest(http.MethodGet, "/user/123", http.NoBody) + w := httptest.NewRecorder() + + router.ServeHTTP(w, r) + + assert.Equal(t, http.StatusOK, w.Code, "unexpected status code") + + spans := sr.Ended() + + require.Len(t, sr.Ended(), 1) + span := spans[0] + attr := span.Attributes() + assert.True(t, ensurePrefix(http.MethodGet, spans[0].Name())) + assert.Equal(t, "GET /user/{id}", span.Name()) + assert.Equal(t, trace.SpanKindServer, span.SpanKind()) + assert.Contains(t, attr, attribute.Int("http.response.status_code", http.StatusOK)) + assert.Contains(t, attr, attribute.String("http.request.method", "GET")) + assert.Contains(t, attr, attribute.String("http.route", "/user/{id}")) + assert.Equal(t, codes.Unset, span.Status().Code) + assert.Empty(t, span.Status().Description) +} + func TestCustomSpanNameFormatter(t *testing.T) { exporter := tracetest.NewInMemoryExporter() @@ -34,9 +67,9 @@ func TestCustomSpanNameFormatter(t *testing.T) { testdata := []struct { spanNameFormatter func(string, *http.Request) string - expected string + want string }{ - {nil, routeTpl}, + {nil, setDefaultName(http.MethodGet, routeTpl)}, { func(string, *http.Request) string { return "custom" }, "custom", @@ -50,7 +83,7 @@ func TestCustomSpanNameFormatter(t *testing.T) { } for i, d := range testdata { - t.Run(fmt.Sprintf("%d_%s", i, d.expected), func(t *testing.T) { + t.Run(fmt.Sprintf("%d_%s", i, d.want), func(t *testing.T) { router := mux.NewRouter() router.Use(otelmux.Middleware( "foobar", @@ -66,7 +99,7 @@ func TestCustomSpanNameFormatter(t *testing.T) { spans := exporter.GetSpans() require.Len(t, spans, 1) - assert.Equal(t, d.expected, spans[0].Name) + assert.Equal(t, d.want, spans[0].Name) exporter.Reset() }) @@ -95,28 +128,49 @@ func TestSDKIntegration(t *testing.T) { router.HandleFunc("/book/{title}", ok) tests := []struct { - name string - path string - reqFunc func(r *http.Request) - expected string + name string + method string + path string + reqFunc func(r *http.Request) + wantSpanName string + wantMethod string + wantRoute string }{ { - name: "user route", - path: "/user/123", - reqFunc: nil, - expected: "/user/{id:[0-9]+}", + name: "user route", + method: http.MethodGet, + path: "/user/123", + reqFunc: nil, + wantSpanName: "GET /user/{id:[0-9]+}", + wantMethod: http.MethodGet, + wantRoute: "/user/{id:[0-9]+}", }, { - name: "book route", - path: "/book/foo", - reqFunc: nil, - expected: "/book/{title}", + name: "POST book route", + method: http.MethodPost, + path: "/book/foo", + reqFunc: nil, + wantSpanName: "POST /book/{title}", + wantMethod: http.MethodPost, + wantRoute: "/book/{title}", }, { - name: "book route with custom pattern", - path: "/book/bar", - reqFunc: func(r *http.Request) { r.Pattern = "/book/{custom}" }, - expected: "/book/{custom}", + name: "book route with custom pattern", + method: http.MethodGet, + path: "/book/bar", + reqFunc: func(r *http.Request) { r.Pattern = "/book/{custom}" }, + wantSpanName: "GET /book/{custom}", + wantMethod: http.MethodGet, + wantRoute: "/book/{custom}", + }, + { + name: "Invalid HTTP Method", + method: "INVALID", + path: "/book/bar", + reqFunc: func(r *http.Request) { r.Pattern = "/book/{custom}" }, + wantSpanName: "HTTP /book/{custom}", + wantMethod: http.MethodGet, + wantRoute: "/book/{custom}", }, } @@ -124,22 +178,23 @@ func TestSDKIntegration(t *testing.T) { t.Run(tt.name, func(t *testing.T) { defer sr.Reset() - r := httptest.NewRequest(http.MethodGet, tt.path, http.NoBody) + r := httptest.NewRequest(tt.method, tt.path, http.NoBody) if tt.reqFunc != nil { tt.reqFunc(r) } w := httptest.NewRecorder() router.ServeHTTP(w, r) + spans := sr.Ended() - require.Len(t, sr.Ended(), 1) + require.Len(t, spans, 1) assertSpan(t, sr.Ended()[0], - tt.expected, + tt.wantSpanName, trace.SpanKindServer, attribute.String("server.address", "foobar"), attribute.Int("http.response.status_code", http.StatusOK), - attribute.String("http.request.method", "GET"), - attribute.String("http.route", tt.expected), + attribute.String("http.request.method", tt.wantMethod), + attribute.String("http.route", tt.wantRoute), ) }) } @@ -160,7 +215,7 @@ func TestNotFoundIsNotError(t *testing.T) { require.Len(t, sr.Ended(), 1) assertSpan(t, sr.Ended()[0], - "/does/not/exist", + "GET /does/not/exist", trace.SpanKindServer, attribute.String("server.address", "foobar"), attribute.Int("http.response.status_code", http.StatusNotFound), @@ -326,14 +381,14 @@ func TestHandlerWithMetricAttributesFn(t *testing.T) { serverDuration = "http.server.duration" ) testCases := []struct { - name string - fn func(r *http.Request) []attribute.KeyValue - expectedAdditionalAttribute []attribute.KeyValue + name string + fn func(r *http.Request) []attribute.KeyValue + wantAdditionalAttribute []attribute.KeyValue }{ { - name: "With a nil function", - fn: nil, - expectedAdditionalAttribute: []attribute.KeyValue{}, + name: "With a nil function", + fn: nil, + wantAdditionalAttribute: []attribute.KeyValue{}, }, { name: "With a function that returns an additional attribute", @@ -343,7 +398,7 @@ func TestHandlerWithMetricAttributesFn(t *testing.T) { attribute.String("barKey", "barValue"), } }, - expectedAdditionalAttribute: []attribute.KeyValue{ + wantAdditionalAttribute: []attribute.KeyValue{ attribute.String("fooKey", "fooValue"), attribute.String("barKey", "barValue"), }, @@ -379,12 +434,12 @@ func TestHandlerWithMetricAttributesFn(t *testing.T) { d, ok := m.Data.(metricdata.Sum[int64]) assert.True(t, ok) assert.Len(t, d.DataPoints, 1) - containsAttributes(t, d.DataPoints[0].Attributes, testCases[0].expectedAdditionalAttribute) + containsAttributes(t, d.DataPoints[0].Attributes, testCases[0].wantAdditionalAttribute) case serverDuration: d, ok := m.Data.(metricdata.Histogram[float64]) assert.True(t, ok) assert.Len(t, d.DataPoints, 1) - containsAttributes(t, d.DataPoints[0].Attributes, testCases[0].expectedAdditionalAttribute) + containsAttributes(t, d.DataPoints[0].Attributes, testCases[0].wantAdditionalAttribute) } } } @@ -397,3 +452,11 @@ func containsAttributes(t *testing.T, attrSet attribute.Set, expected []attribut assert.Equal(t, att.Value.AsString(), actualValue.AsString()) } } + +func setDefaultName(method, path string) string { + return method + " " + path +} + +func ensurePrefix(prefix, s string) bool { + return strings.HasPrefix(s, prefix) +}