diff --git a/CHANGELOG.md b/CHANGELOG.md index 332f4965ac4..9d4e1c1f050 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 - Use Gin's own `ClientIP` method to detect the client's IP, which supports custom proxy headers in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#6095) - Added test for Fields in `go.opentelemetry.io/contrib/propagators/jaeger`. (#7119) - Allow configuring samplers in `go.opentelemetry.io/contrib/otelconf`. (#7148) +- Rerun the span name formatter after the request ran if a `req.Pattern` is set, so the span name can include it in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#7192) ### Changed diff --git a/instrumentation/net/http/otelhttp/config.go b/instrumentation/net/http/otelhttp/config.go index a01bfafbe07..2d565017860 100644 --- a/instrumentation/net/http/otelhttp/config.go +++ b/instrumentation/net/http/otelhttp/config.go @@ -4,6 +4,7 @@ package otelhttp // import "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" import ( + "cmp" "context" "net/http" "net/http/httptrace" @@ -174,9 +175,13 @@ func WithMessageEvents(events ...event) Option { }) } -// WithSpanNameFormatter takes a function that will be called on every +// WithSpanNameFormatter takes a [SpanNameFormatter] function that will be called on every // request and the returned string will become the Span Name. -func WithSpanNameFormatter(f func(operation string, r *http.Request) string) Option { +// +// When using `http.ServeMux` (or any middleware that sets `request.Pattern`), +// the span name formatter will run twice. Once when the span is created, and +// once after the middleware, so the pattern can be used. +func WithSpanNameFormatter(f SpanNameFormatter) Option { return optionFunc(func(c *config) { c.SpanNameFormatter = f }) @@ -205,3 +210,24 @@ func WithMetricAttributesFn(metricAttributesFn func(r *http.Request) []attribute c.MetricAttributesFn = metricAttributesFn }) } + +// SpanNameFormatter returns the span name to use for a given request. +type SpanNameFormatter = func(operation string, r *http.Request) string + +// SpanNameFromOperation always uses the operation name as the span name. +// It is the default formatter for handlers. +func SpanNameFromOperation(operation string, _ *http.Request) string { + return operation +} + +// SpanNameFromPattern uses the matched request pattern as the span name. +// It falls back to the operation name if there is no pattern. +func SpanNameFromPattern(operation string, r *http.Request) string { + return cmp.Or(r.Pattern, operation) +} + +// SpanNameFromMethod uses "HTTP " + the request method as the span name. +// It is the default formatter for transports. +func SpanNameFromMethod(_ string, r *http.Request) string { + return "HTTP " + r.Method +} diff --git a/instrumentation/net/http/otelhttp/config_test.go b/instrumentation/net/http/otelhttp/config_test.go index 56e0f8de0c3..e0f1008aa96 100644 --- a/instrumentation/net/http/otelhttp/config_test.go +++ b/instrumentation/net/http/otelhttp/config_test.go @@ -60,24 +60,25 @@ func TestBasicFilter(t *testing.T) { func TestSpanNameFormatter(t *testing.T) { testCases := []struct { name string - formatter func(s string, r *http.Request) string + formatter otelhttp.SpanNameFormatter operation string expected string }{ { - name: "default handler formatter", - formatter: func(operation string, _ *http.Request) string { - return operation - }, + name: "default handler formatter", + formatter: otelhttp.SpanNameFromOperation, operation: "test_operation", expected: "test_operation", }, { - name: "default transport formatter", - formatter: func(_ string, r *http.Request) string { - return "HTTP " + r.Method - }, - expected: "HTTP GET", + name: "default transport formatter", + formatter: otelhttp.SpanNameFromMethod, + expected: "HTTP GET", + }, + { + name: "request pattern formatter", + formatter: otelhttp.SpanNameFromPattern, + expected: "GET /hello/{thing}", }, { name: "custom formatter", @@ -85,7 +86,7 @@ func TestSpanNameFormatter(t *testing.T) { return r.URL.Path }, operation: "", - expected: "/hello", + expected: "/hello/world", }, } @@ -100,13 +101,15 @@ func TestSpanNameFormatter(t *testing.T) { t.Fatal(err) } }) + mux := http.NewServeMux() + mux.Handle("GET /hello/{thing}", handler) h := otelhttp.NewHandler( - handler, + mux, tc.operation, otelhttp.WithTracerProvider(provider), otelhttp.WithSpanNameFormatter(tc.formatter), ) - r, err := http.NewRequest(http.MethodGet, "http://localhost/hello", nil) + r, err := http.NewRequest(http.MethodGet, "http://localhost/hello/world", nil) if err != nil { t.Fatal(err) } diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go index de36ec912aa..0f3c511e296 100644 --- a/instrumentation/net/http/otelhttp/handler.go +++ b/instrumentation/net/http/otelhttp/handler.go @@ -28,7 +28,7 @@ type middleware struct { readEvent bool writeEvent bool filters []Filter - spanNameFormatter func(string, *http.Request) string + spanNameFormatter SpanNameFormatter publicEndpoint bool publicEndpointFn func(*http.Request) bool metricAttributesFn func(*http.Request) []attribute.KeyValue @@ -36,10 +36,6 @@ type middleware struct { semconv semconv.HTTPServer } -func defaultHandlerFormatter(operation string, _ *http.Request) string { - return operation -} - // NewHandler wraps the passed handler in a span named after the operation and // enriches it with metrics. func NewHandler(handler http.Handler, operation string, opts ...Option) http.Handler { @@ -56,7 +52,7 @@ func NewMiddleware(operation string, opts ...Option) func(http.Handler) http.Han defaultOpts := []Option{ WithSpanOptions(trace.WithSpanKind(trace.SpanKindServer)), - WithSpanNameFormatter(defaultHandlerFormatter), + WithSpanNameFormatter(SpanNameFromOperation), } c := newConfig(append(defaultOpts, opts...)...) @@ -176,7 +172,12 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http ctx = ContextWithLabeler(ctx, labeler) } - next.ServeHTTP(w, r.WithContext(ctx)) + r = r.WithContext(ctx) + next.ServeHTTP(w, r) + + if r.Pattern != "" { + span.SetName(h.spanNameFormatter(h.operation, r)) + } statusCode := rww.StatusCode() bytesWritten := rww.BytesWritten() diff --git a/instrumentation/net/http/otelhttp/handler_test.go b/instrumentation/net/http/otelhttp/handler_test.go index d05f7624514..ee32914a65b 100644 --- a/instrumentation/net/http/otelhttp/handler_test.go +++ b/instrumentation/net/http/otelhttp/handler_test.go @@ -437,6 +437,66 @@ func TestHandlerRequestWithTraceContext(t *testing.T) { assert.Equal(t, spans[1].SpanContext().SpanID(), spans[0].Parent().SpanID()) } +func TestWithSpanNameFormatter(t *testing.T) { + for _, tt := range []struct { + name string + + formatter func(operation string, r *http.Request) string + wantSpanName string + }{ + { + name: "with the default span name formatter", + wantSpanName: "test_handler", + }, + { + name: "with a custom span name formatter", + formatter: func(op string, r *http.Request) string { + return fmt.Sprintf("%s %s", r.Method, r.URL.Path) + }, + wantSpanName: "GET /foo/123", + }, + { + name: "with a custom span name formatter using the pattern", + formatter: func(op string, r *http.Request) string { + return fmt.Sprintf("%s %s", r.Method, r.Pattern) + }, + wantSpanName: "GET /foo/{id}", + }, + } { + t.Run(tt.name, func(t *testing.T) { + spanRecorder := tracetest.NewSpanRecorder() + provider := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(spanRecorder), + ) + + opts := []Option{ + WithTracerProvider(provider), + } + if tt.formatter != nil { + opts = append(opts, WithSpanNameFormatter(tt.formatter)) + } + + mux := http.NewServeMux() + mux.Handle("/foo/{id}", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Nothing to do here + })) + h := NewHandler(mux, "test_handler", opts...) + + r, err := http.NewRequest(http.MethodGet, "http://localhost/foo/123", nil) + require.NoError(t, err) + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, r) + assert.Equal(t, http.StatusOK, rr.Result().StatusCode) + + assert.NoError(t, spanRecorder.ForceFlush(context.Background())) + spans := spanRecorder.Ended() + assert.Len(t, spans, 1) + assert.Equal(t, tt.wantSpanName, spans[0].Name()) + }) + } +} + func TestWithPublicEndpoint(t *testing.T) { spanRecorder := tracetest.NewSpanRecorder() provider := sdktrace.NewTracerProvider( diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index 44b86ad8609..7cc623c707f 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -56,7 +56,7 @@ func NewTransport(base http.RoundTripper, opts ...Option) *Transport { defaultOpts := []Option{ WithSpanOptions(trace.WithSpanKind(trace.SpanKindClient)), - WithSpanNameFormatter(defaultTransportFormatter), + WithSpanNameFormatter(SpanNameFromMethod), } c := newConfig(append(defaultOpts, opts...)...) @@ -76,10 +76,6 @@ func (t *Transport) applyConfig(c *config) { t.metricAttributesFn = c.MetricAttributesFn } -func defaultTransportFormatter(_ string, r *http.Request) string { - return "HTTP " + r.Method -} - // RoundTrip creates a Span and propagates its context via the provided request's headers // before handing the request to the configured base RoundTripper. The created span will // end when the response body is closed or when a read from the body returns io.EOF.