diff --git a/pkg/inst-api-semconv/instrumenter/http/http_attrs_extractor.go b/pkg/inst-api-semconv/instrumenter/http/http_attrs_extractor.go index 8315645c1..8bb589e2b 100644 --- a/pkg/inst-api-semconv/instrumenter/http/http_attrs_extractor.go +++ b/pkg/inst-api-semconv/instrumenter/http/http_attrs_extractor.go @@ -209,13 +209,18 @@ func (h *HttpServerAttrsExtractor[REQUEST, RESPONSE, GETTER1, GETTER2, GETTER3]) localRootSpan, ok := span.(sdktrace.ReadOnlySpan) if ok && span.IsRecording() { route := h.Base.HttpGetter.GetHttpRoute(request) - if !strings.Contains(localRootSpan.Name(), route) { - route = localRootSpan.Name() + spanName := localRootSpan.Name() + if route == "" || (route != "" && !strings.Contains(spanName, route)) { + if tmpl := httpRouteFromSpanName(spanName); tmpl != "" { + route = tmpl + } + } + if route != "" { + attributes = append(attributes, attribute.KeyValue{ + Key: semconv.HTTPRouteKey, + Value: attribute.StringValue(route), + }) } - attributes = append(attributes, attribute.KeyValue{ - Key: semconv.HTTPRouteKey, - Value: attribute.StringValue(route), - }) } if h.Base.AttributesFilter != nil { attributes = h.Base.AttributesFilter(attributes) diff --git a/pkg/inst-api-semconv/instrumenter/http/http_route.go b/pkg/inst-api-semconv/instrumenter/http/http_route.go new file mode 100644 index 000000000..5a36fd835 --- /dev/null +++ b/pkg/inst-api-semconv/instrumenter/http/http_route.go @@ -0,0 +1,56 @@ +// Copyright (c) 2024 Alibaba Group Holding Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package http + +import "strings" + +// httpRouteFromSpanName returns a low-cardinality route template encoded in the +// span name by framework hooks (gin FullPath, mux template, etc.). Raw request +// paths such as "/users/123" must not be inferred from span names. +func httpRouteFromSpanName(spanName string) string { + if spanName == "" { + return "" + } + path := spanName + if idx := strings.Index(spanName, " "); idx >= 0 { + path = spanName[idx+1:] + } + if path == "" { + return "" + } + if strings.Contains(path, "{") && strings.Contains(path, "}") { + return path + } + for _, segment := range strings.Split(path, "/") { + if strings.HasPrefix(segment, ":") || strings.HasPrefix(segment, "*") { + return path + } + } + return "" +} + +// NetHttpRouteFromPattern normalizes net/http Request.Pattern (Go 1.22+). +func NetHttpRouteFromPattern(pattern string) string { + if pattern == "" { + return "" + } + if _, rest, ok := strings.Cut(pattern, " "); ok { + pattern = rest + } + if idx := strings.IndexByte(pattern, '/'); idx >= 0 { + return pattern[idx:] + } + return "" +} diff --git a/pkg/inst-api-semconv/instrumenter/http/http_route_test.go b/pkg/inst-api-semconv/instrumenter/http/http_route_test.go new file mode 100644 index 000000000..d634cf065 --- /dev/null +++ b/pkg/inst-api-semconv/instrumenter/http/http_route_test.go @@ -0,0 +1,56 @@ +// Copyright (c) 2024 Alibaba Group Holding Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package http + +import "testing" + +func TestHttpRouteFromSpanName(t *testing.T) { + for _, tc := range []struct { + name string + want string + }{ + {"GET /users/123", ""}, + {"/user/:name", "/user/:name"}, + {"/test/{key}", "/test/{key}"}, + {"/hertz/:version/*action", "/hertz/:version/*action"}, + {"GET /a", ""}, + {"GET /time/12:30", ""}, + {"GET /files/*path", "/files/*path"}, + {"GET /literal*value", ""}, + } { + if got := httpRouteFromSpanName(tc.name); got != tc.want { + t.Fatalf("%q: got %q want %q", tc.name, got, tc.want) + } + } +} + +func TestNetHttpRouteFromPattern(t *testing.T) { + for _, tc := range []struct { + pattern string + want string + }{ + {"GET /users/{id}", "/users/{id}"}, + {"/health", "/health"}, + {"example.com/x", "/x"}, + {"GET example.com/x/{id}", "/x/{id}"}, + {"/files/{path...}", "/files/{path...}"}, + {"/{$}", "/{$}"}, + {"", ""}, + } { + if got := NetHttpRouteFromPattern(tc.pattern); got != tc.want { + t.Fatalf("%q: got %q want %q", tc.pattern, got, tc.want) + } + } +} diff --git a/pkg/rules/echo/echo_server_setup.go b/pkg/rules/echo/echo_server_setup.go index 5341a30f3..5b5b1dab7 100644 --- a/pkg/rules/echo/echo_server_setup.go +++ b/pkg/rules/echo/echo_server_setup.go @@ -40,7 +40,7 @@ func otelTraceMiddleware() echo.MiddlewareFunc { c.Error(err) } lcs := trace.LocalRootSpanFromGLS() - if lcs != nil && c.Path() != "" && c.Request() != nil && c.Request().URL != nil && (c.Request().URL.Path != c.Path()) { + if lcs != nil && c.Path() != "" { lcs.SetName(c.Path()) } return diff --git a/pkg/rules/gin/gin_html_setup.go b/pkg/rules/gin/gin_html_setup.go index 767e00e57..2fe04d495 100644 --- a/pkg/rules/gin/gin_html_setup.go +++ b/pkg/rules/gin/gin_html_setup.go @@ -32,7 +32,7 @@ func htmlOnEnter(call api.CallContext, c *gin.Context, code int, name string, ob return } lcs := trace.LocalRootSpanFromGLS() - if lcs != nil && c.FullPath() != "" && c.Request != nil && c.Request.URL != nil && (c.FullPath() != c.Request.URL.Path) { + if lcs != nil && c.FullPath() != "" { lcs.SetName(c.FullPath()) } } diff --git a/pkg/rules/gin/gin_server_setup.go b/pkg/rules/gin/gin_server_setup.go index 7d1887459..381bf3f60 100644 --- a/pkg/rules/gin/gin_server_setup.go +++ b/pkg/rules/gin/gin_server_setup.go @@ -32,7 +32,7 @@ func nextOnEnter(call api.CallContext, c *gin.Context) { return } lcs := trace.LocalRootSpanFromGLS() - if lcs != nil && c.FullPath() != "" && c.Request != nil && c.Request.URL != nil && (c.FullPath() != c.Request.URL.Path) { + if lcs != nil && c.FullPath() != "" { lcs.SetName(c.FullPath()) } } diff --git a/pkg/rules/gorestful/gorestful_server_setup.go b/pkg/rules/gorestful/gorestful_server_setup.go index bdb0d95a6..8e587eca4 100644 --- a/pkg/rules/gorestful/gorestful_server_setup.go +++ b/pkg/rules/gorestful/gorestful_server_setup.go @@ -75,7 +75,7 @@ var filterRest = func(req *restful.Request, resp *restful.Response, chain *restf return } lcs := trace.LocalRootSpanFromGLS() - if lcs != nil && req.SelectedRoutePath() != "" && req.Request != nil && req.Request.URL != nil && (req.SelectedRoutePath() != req.Request.URL.Path) { + if lcs != nil && req.SelectedRoutePath() != "" { lcs.SetName(req.SelectedRoutePath()) } } diff --git a/pkg/rules/http/net_http_data_type.go b/pkg/rules/http/net_http_data_type.go index 94e6c9803..a0b6ff880 100644 --- a/pkg/rules/http/net_http_data_type.go +++ b/pkg/rules/http/net_http_data_type.go @@ -27,6 +27,7 @@ type netHttpRequest struct { isTls bool header http.Header version string + routePattern string requestHeaders string requestBody string } diff --git a/pkg/rules/http/net_http_otel_instrumenter.go b/pkg/rules/http/net_http_otel_instrumenter.go index c4d823a63..cc6158227 100644 --- a/pkg/rules/http/net_http_otel_instrumenter.go +++ b/pkg/rules/http/net_http_otel_instrumenter.go @@ -210,7 +210,7 @@ func (n netHttpServerAttrsGetter) GetNetworkPeerPort(request *netHttpRequest, re } func (n netHttpServerAttrsGetter) GetHttpRoute(request *netHttpRequest) string { - return request.url.Path + return http.NetHttpRouteFromPattern(request.routePattern) } func BuildNetHttpClientOtelInstrumenter() *instrumenter.PropagatingToDownstreamInstrumenter[*netHttpRequest, *netHttpResponse] { diff --git a/pkg/rules/http/server_setup.go b/pkg/rules/http/server_setup.go index c0ce3a4a6..fdca0ca88 100644 --- a/pkg/rules/http/server_setup.go +++ b/pkg/rules/http/server_setup.go @@ -23,6 +23,8 @@ import ( _ "unsafe" "github.com/alibaba/loongsuite-go/pkg/api" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/trace" ) var netHttpServerInstrumenter = BuildNetHttpServerOtelInstrumenter() @@ -39,12 +41,12 @@ func serverOnEnter(call api.CallContext, _ interface{}, w http.ResponseWriter, r return } request := &netHttpRequest{ - method: r.Method, - url: r.URL, - header: r.Header, - version: getProtocolVersion(r.ProtoMajor, r.ProtoMinor), - host: r.Host, - isTls: r.TLS != nil, + method: r.Method, + url: r.URL, + header: r.Header, + version: getProtocolVersion(r.ProtoMajor, r.ProtoMinor), + host: r.Host, + isTls: r.TLS != nil, } request.requestHeaders = netHttpCaptureConfig.captureHeaders(r.Header) request.requestBody = captureHTTPRequestBody(r, netHttpCaptureConfig) @@ -58,9 +60,10 @@ func serverOnEnter(call api.CallContext, _ interface{}, w http.ResponseWriter, r } call.SetParam(1, x1) } - data := make(map[string]interface{}, 2) + data := make(map[string]interface{}, 3) data["ctx"] = ctx data["request"] = request + data["httpRequest"] = r call.SetData(data) return } @@ -79,6 +82,35 @@ func serverOnExit(call api.CallContext) { if !ok { return } + if rawRequest, ok := data["httpRequest"].(*http.Request); ok { + // ServeMux fills Pattern after this hook's entry. Middleware that passes + // r.WithContext(...) to the mux replaces the request pointer, so the + // outer hook cannot observe that copy's pattern. + request.routePattern = rawRequest.Pattern + span := trace.SpanFromContext(ctx) + // Framework hooks set a validated static route as the span name. When + // that route equals the raw path, retain it without inferring arbitrary + // request paths as route templates. + if request.routePattern == "" && rawRequest.URL != nil { + if readable, ok := span.(sdktrace.ReadOnlySpan); ok && + readable.Name() == rawRequest.URL.Path { + request.routePattern = rawRequest.URL.Path + } + } + // A framework may already have published a more specific route template + // (for example go-restful's /users/{id}). Do not replace it with the + // outer ServeMux catch-all pattern. A framework static route equals the + // raw path, so it is still safe to add the HTTP method in that case. + spanName := "" + if readable, ok := span.(sdktrace.ReadOnlySpan); ok { + spanName = readable.Name() + } + if route := (netHttpServerAttrsGetter{}).GetHttpRoute(request); route != "" && + (spanName == request.method || + (rawRequest.URL != nil && spanName == rawRequest.URL.Path)) { + span.SetName(request.method + " " + route) + } + } if p, ok := call.GetParam(1).(http.ResponseWriter); ok { if w1, ok := p.(*writerWrapper); ok { netHttpServerInstrumenter.End(ctx, request, &netHttpResponse{ diff --git a/pkg/rules/iris/router_setup.go b/pkg/rules/iris/router_setup.go index 8c4c44ea6..de175d4d0 100644 --- a/pkg/rules/iris/router_setup.go +++ b/pkg/rules/iris/router_setup.go @@ -32,7 +32,7 @@ func irisHttpOnEnter(call api.CallContext, _ interface{}, iCtx *iContext.Context } r := iCtx.Request() lcs := trace.LocalRootSpanFromGLS() - if lcs != nil && r != nil && iCtx.Path() != "" && r.URL != nil && (iCtx.Path() != r.URL.Path) { + if lcs != nil && r != nil && iCtx.Path() != "" { lcs.SetName(iCtx.Path()) } } diff --git a/pkg/rules/mux/mux_server_setup.go b/pkg/rules/mux/mux_server_setup.go index e360afa5a..147891f66 100644 --- a/pkg/rules/mux/mux_server_setup.go +++ b/pkg/rules/mux/mux_server_setup.go @@ -46,7 +46,7 @@ func muxRoute130OnEnter(call api.CallContext, req *http.Request, route interface r, ok := route.(*mux.Route) if ok { tmpl, err := r.GetPathTemplate() - if err == nil && req.URL != nil && tmpl != req.URL.Path { + if err == nil && tmpl != "" { lcs.SetName(tmpl) } } @@ -65,7 +65,7 @@ func muxRoute174OnEnter(call api.CallContext, req *http.Request, route *mux.Rout lcs := trace.LocalRootSpanFromGLS() if lcs != nil && route != nil { tmpl, err := route.GetPathTemplate() - if err == nil && req.URL != nil && tmpl != req.URL.Path { + if err == nil && tmpl != "" { lcs.SetName(tmpl) } } diff --git a/test/otel_tests.go b/test/otel_tests.go index fcbf5fb04..81e24a586 100644 --- a/test/otel_tests.go +++ b/test/otel_tests.go @@ -26,5 +26,7 @@ func TestSpanFromContext(t *testing.T, env ...string) { UseApp("otel") RunGoBuild(t, "go", "build", "--", "test_span_from_context.go") stdout, _ := RunApp(t, "test_span_from_context", env...) - ExpectContains(t, stdout, "GET /otel") + // The ServeMux route is applied when the server hook exits, after this + // application prints the in-flight span from inside the handler. + ExpectContains(t, stdout, "GET") } diff --git a/test/verifier/verifier.go b/test/verifier/verifier.go index 99a582888..8541c36aa 100644 --- a/test/verifier/verifier.go +++ b/test/verifier/verifier.go @@ -85,7 +85,15 @@ func VerifyHttpServerAttributes(span tracetest.SpanStub, name, method, protocolN Assert(GetAttribute(span.Attributes, "url.scheme").AsString() == scheme, "Except url scheme to be %s, got %s", scheme, GetAttribute(span.Attributes, "url.scheme").AsString()) Assert(GetAttribute(span.Attributes, "url.path").AsString() == path, "Except url path to be %s, got %s", path, GetAttribute(span.Attributes, "url.path").AsString()) Assert(GetAttribute(span.Attributes, "url.query").AsString() == query, "Except url query to be %s, got %s", query, GetAttribute(span.Attributes, "url.query").AsString()) - Assert(GetAttribute(span.Attributes, "http.route").AsString() == route, "Except http route to be %s, got %s", route, GetAttribute(span.Attributes, "http.route").AsString()) + if route == "" { + for _, attr := range span.Attributes { + if attr.Key == semconv.HTTPRouteKey { + Assert(false, "http.route should be omitted, got %q", attr.Value.AsString()) + } + } + } else { + Assert(GetAttribute(span.Attributes, "http.route").AsString() == route, "Except http route to be %s, got %s", route, GetAttribute(span.Attributes, "http.route").AsString()) + } Assert(GetAttribute(span.Attributes, "http.response.status_code").AsInt64() == statusCode, "Except status code to be %d, got %d", statusCode, GetAttribute(span.Attributes, "http.response.status_code").AsInt64()) } diff --git a/test/zap/test_zap_with_field.go b/test/zap/test_zap_with_field.go index 3ffa3be36..7709e0fc8 100644 --- a/test/zap/test_zap_with_field.go +++ b/test/zap/test_zap_with_field.go @@ -50,7 +50,7 @@ func main() { port, err := strconv.Atoi(Url.Port()) verifier.WaitAndAssertTraces(func(stubs []tracetest.SpanStubs) { verifier.VerifyHttpClientAttributes(stubs[0][0], "GET", "GET", ts_a.URL, "http", "1.1", "tcp", "ipv4", "", Url.Host, 200, 0, int64(port)) - verifier.VerifyHttpServerAttributes(stubs[0][1], "GET /", "GET", "http", "tcp", "ipv4", "", Url.Host, "Go-http-client/1.1", "http", "/", "", "/", 200) + verifier.VerifyHttpServerAttributes(stubs[0][1], "GET", "GET", "http", "tcp", "ipv4", "", Url.Host, "Go-http-client/1.1", "http", "/", "", "", 200) if stubs[0][1].Parent.TraceID().String() != stubs[0][0].SpanContext.TraceID().String() { log.Fatal("span 1 should be child of span 0") }