Repository navigation
fix(http): stop using raw URL path as http.route for net/http - #761
happym51888 wants to merge 1 commit into
Conversation
Use Request.Pattern when available, omit http.route otherwise, and only promote span-name routes that look like templates. Fixes alibaba#729 (net/http portion). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Happy Mu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
y1yang0
left a comment
There was a problem hiding this comment.
Thanks for tackling #729. Moving away from the raw URL path as http.route is the right call. However, the PR can't be merged yet: the net/http rule doesn't compile, and even once it does, the route is never picked up.
Blocking
1. pkg/rules/http fails to compile
netHttpRouteFromPattern is unexported in pkg/inst-api-semconv/instrumenter/http, but it's called from a different package (and module), pkg/rules/http:
pkg/rules/http/net_http_otel_instrumenter.go:213:9: undefined: netHttpRouteFromPattern
Every otel go build of an app using net/http would break. Please either export it (e.g. http.NetHttpRouteFromPattern) or move it into pkg/rules/http.
2. r.Pattern is read too early, so it's always empty
The server hook is on serverHandler.ServeHTTP, and serverOnEnter reads r.Pattern at entry. But ServeMux.ServeHTTP only sets it later:
h, r.Pattern, r.pat, r.matches = mux.findHandler(r)As a result, routePattern is always "". The span name becomes plain GET, and http.route is always omitted, even for requests that matched a ServeMux pattern.
Suggested fix:
- Keep the
*http.Requestincall.SetData(...)and readr.PatterninserverOnExit. ServeMux mutates the same request, so the value is there by then. - The span name is decided at
Start, so rename the span once the route is known, e.g.span.SetName(method + " " + route). - Known limitation, worth a code comment: if middleware in front of the mux calls
r.WithContext(...), the pattern is set on the copy and the outer hook still won't see it.
3. The integration test expectations can't pass
The updated tests in test/nethttp, test/zap, test/logrus and test/zerolog expect the span name "GET /a" and http.route to be omitted at the same time. Neither version of the code satisfies both:
- With the current PR, the span name is
"GET"(see #2), sospan.Name == namefails. - With #2 fixed,
http.routewould be"/a", so the "must be omitted" assertion fails.
Only the old behaviour (raw path as route) produced "GET /a", which suggests these tests weren't run against this change. Expected values should be:
- Handlers registered on a ServeMux (e.g.
http.HandleFunc("/a", ...)): span nameGET /a,http.route=/a. - A new case where no pattern matches (e.g. a custom
http.Handlerpassed tohttp.Server): span nameGET,http.routeomitted.
Should fix
4. Host-prefixed patterns aren't normalized
netHttpRouteFromPattern returns "" for valid patterns such as example.com/users/{id}: they neither start with / nor contain a space. Strip the optional METHOD prefix first, then take everything from the first /:
if _, rest, ok := strings.Cut(pattern, " "); ok {
pattern = rest
}
if i := strings.IndexByte(pattern, '/'); i >= 0 {
return pattern[i:]
}
return ""Please also add unit tests for example.com/x, GET example.com/x/{id}, /files/{path...} and /{$}.
5. httpRouteFromSpanName can bring back high cardinality
Any span name containing : or * is treated as a route template, so a raw path like /time/12:30 would be promoted to http.route. This runs in the shared HttpServerAttrsExtractor.OnEnd, so it affects every HTTP framework, not just net/http. Consider matching only real template syntax: {...}, or path segments that start with : or *, instead of a substring anywhere in the path.
Summary
http.routefromRequest.Patternonly (Go 1.22+ ServeMux patterns).http.routewhen no template exists instead of falling back tourl.path.{id},*,:param), not raw paths like/users/123.Problem
High-cardinality
http.routelabels on REST APIs when raw paths were used (semconv violation). See #729.Test plan
http_route_test.gounit tests.HandleFuncservers.Fixes #729 (net/http scope; fasthttp/fiber follow-ups can be separate).
Made with Cursor