fix: use runtime.HTTPPattern for route extraction in spans#55
Conversation
The tracingWrapper middleware hardcoded "ServeHTTP" as the OTEL span name for all HTTP requests, making traces hard to distinguish. Now uses r.Method + " " + r.URL.Path (e.g. "GET /api/v1/rules") following OpenTelemetry HTTP semantic conventions.
- Add statusRecorder to capture response status codes - Record http.status_code attribute on every HTTP span - Set span status to Error for 5xx responses - statusRecorder.Unwrap() preserves http.Flusher etc. for gzip middleware
- HTTPMethodKey → HTTPRequestMethodKey (http.request.method) - HTTPTargetKey → URLPath + URLQuery (url.path, url.query) - HTTPStatusCodeKey → HTTPResponseStatusCode (http.response.status_code) - ServiceNameKey/ServiceVersionKey → ServiceName/ServiceVersion functions - Add url.scheme and server.address attributes to HTTP spans
- Span starts as r.Method, then spanRouteMiddleware renames it to
"GET /api/v1/{resource}" using runtime.HTTPPathPattern after routing
- Sets http.route attribute per OTel HTTP semantic conventions
- Simplify tracingWrapper: extract endSpan() and httpSpanAttributes(),
consolidate r.WithContext() calls (3→1), skip empty url.scheme/query
- statusRecorder: track wroteHeader to record only the first status code, matching net/http behavior where subsequent calls are no-ops - httpSpanAttributes: split r.Host with net.SplitHostPort to set server.address (host only) and server.port separately per semconv
Read runtime/debug.BuildInfo to attach build metadata as OTEL resource attributes on all spans: - process.runtime.name, process.runtime.version (Go version) - vcs.repository.ref.head.revision (git commit hash) - vcs.time (commit timestamp) - vcs.modified (dirty working tree flag)
Read runtime/debug.BuildInfo to attach build metadata as OTEL resource attributes on all spans: - process.executable.name (binary name from os.Args[0]) - process.runtime.version (Go version) - vcs.repository.ref.head.revision (git commit hash) - vcs.time (commit timestamp) - vcs.modified (dirty working tree flag) These are set once on the OTEL resource, so they appear on every span automatically without per-request overhead.
- WriteHeader: only record first final status (>= 200), skip 1xx - Write: set status to 200 if WriteHeader was never called, matching net/http implicit status behavior
… tests
The spanRouteMiddleware was using runtime.HTTPPathPattern (set inside
the handler via AnnotateContext) instead of runtime.HTTPPattern (set
by handleHandler before the middleware runs). This caused the route
pattern to never be found.
Adds tests with in-memory OTel exporter that verify:
- Span name, http.request.method, url.path, url.query, server.address,
server.port, http.response.status_code attributes
- End-to-end grpc-gateway routing: span renamed to "GET /api/v1/rules/{rule_id=*}"
with http.route attribute set
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated HTTP tracing: derive URL scheme from headers/TLS, omit empty server address, switch route pattern source, and make span finalization recover panics and record errors. Added OpenTelemetry in-memory tests validating span attributes, naming, status, and panic handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TracingMiddleware
participant Handler
participant Tracer
Client->>TracingMiddleware: HTTP request (may include X-Forwarded-Proto)
TracingMiddleware->>Tracer: Start span (derive scheme, set attributes)
TracingMiddleware->>Handler: Invoke handler (wrap response writer)
alt handler completes normally
Handler-->>TracingMiddleware: Response (status)
TracingMiddleware->>Tracer: End span (set status from response)
else handler panics
Handler-->>TracingMiddleware: panic
TracingMiddleware->>Tracer: Record error, set status=Error, set 500
TracingMiddleware->>Tracer: End span
TracingMiddleware-->>Client: propagate panic (re-panic)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes HTTP gateway span naming by extracting the matched grpc-gateway route pattern from runtime.HTTPPattern and aligning HTTP span attributes/resource metadata with newer OpenTelemetry semantic conventions.
Changes:
- Update
spanRouteMiddlewareto rename spans and sethttp.routeusingruntime.HTTPPattern. - Improve HTTP tracing spans: record HTTP status codes on spans (and mark 5xx as errors) and emit updated semconv HTTP attributes.
- Add tests using an in-memory OTel exporter to validate span names, attributes, and end-to-end route pattern extraction.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
initializers.go |
Adds richer OTEL resource attributes (service/process/VCS) and updates semconv version usage. |
core.go |
Updates HTTP tracing span naming/attributes, adds status recording + span error status for 5xx, and fixes grpc-gateway route pattern extraction for spans. |
core_coverage_test.go |
Adds OTel in-memory exporter tests validating span attributes and grpc-gateway span naming/route extraction. |
config/README.md |
Documents deprecation and default change for OpenTracing bridge config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- httpSpanAttributes: only set server.address when host is non-empty - Tests: find spans by name instead of index to avoid flakiness - Add TestTracingWrapperSpanErrorStatus verifying codes.Error for 5xx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core_coverage_test.go (1)
176-251: Add regression cases for the101/1xxbranch.
statusRecordernow has special handling for101 Switching Protocolsand informational1xxresponses, but the new tests only cover200/404/500and implicit200. A101case plus a103 -> 200sequence would lock down the branch that makes the recorder non-trivial.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core_coverage_test.go` around lines 176 - 251, Add tests exercising the 1xx branch: in TestTracingWrapper_StatusCodes add a case that the handler calls WriteHeader(http.StatusSwitchingProtocols) and assert wrapped response code becomes 101, and add another case that first writes an informational status (e.g. 103) then later writes a final status (e.g. 200) to verify the recorder transitions to the final 200; target the tracingWrapper-wrapped handler and/or directly construct a statusRecorder to assert its status field behavior for the 101 and 103->200 sequence so the special handling in statusRecorder is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core.go`:
- Around line 248-259: The span records 200 when a handler panics before headers
because statusRecorder.initializes status to http.StatusOK; change the behavior
so a missing explicit status is distinguishable and treated as 500 in panic
paths: make statusRecorder start with a zero/undefined status, ensure
statusRecorder sets status only in WriteHeader (and on first Write if
WriteHeader was never called), and update endSpan (called in the deferred block
alongside serverSpan) to treat an undefined/zero status as
http.StatusInternalServerError before setting span attributes; keep the same
statusRecorder and endSpan symbols so the deferred endSpan(serverSpan, rec)
correctly records 500 for panics that write a 500 later by net/http.
---
Nitpick comments:
In `@core_coverage_test.go`:
- Around line 176-251: Add tests exercising the 1xx branch: in
TestTracingWrapper_StatusCodes add a case that the handler calls
WriteHeader(http.StatusSwitchingProtocols) and assert wrapped response code
becomes 101, and add another case that first writes an informational status
(e.g. 103) then later writes a final status (e.g. 200) to verify the recorder
transitions to the final 200; target the tracingWrapper-wrapped handler and/or
directly construct a statusRecorder to assert its status field behavior for the
101 and 103->200 sequence so the special handling in statusRecorder is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b0e0e04-7107-4b85-9443-0bf3da41334f
📒 Files selected for processing (4)
config/README.mdcore.gocore_coverage_test.goinitializers.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core.go`:
- Around line 217-244: In httpSpanAttributes, url.scheme is being taken from
r.URL.Scheme which is empty for server requests; change the logic in
httpSpanAttributes to set scheme by checking r.TLS first (if non-nil => "https",
else "http") and only fall back to r.URL.Scheme for edge cases, then append
semconv.URLScheme with that derived scheme; update uses of r.URL.Scheme in
httpSpanAttributes to use the new derived variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e95c3eb-c0c1-4375-a7fa-e43d73a40efb
📒 Files selected for processing (2)
core.gocore_coverage_test.go
✅ Files skipped from review due to trivial changes (1)
- core_coverage_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| scheme = "http" | ||
| } | ||
| } | ||
| attrs = append(attrs, semconv.URLScheme(scheme)) |
There was a problem hiding this comment.
httpSpanAttributes is documented as "omitting empty-valued attributes", but the new logic always appends url.scheme (defaulting to "http"). Either update the function comment to match the new behavior, or only set url.scheme when it’s explicitly known (URL scheme provided, TLS present, or a trusted forwarded-proto header).
| } else { | |
| scheme = "http" | |
| } | |
| } | |
| attrs = append(attrs, semconv.URLScheme(scheme)) | |
| } | |
| } | |
| if scheme != "" { | |
| attrs = append(attrs, semconv.URLScheme(scheme)) | |
| } |
There was a problem hiding this comment.
Good catch on the comment — url.scheme is now always set (derived from X-Forwarded-Proto / r.TLS / default http). The function comment should be updated. Will fix if we do another pass on this PR.
| defer func() { | ||
| if recovered := recover(); recovered != nil { | ||
| if !rec.wroteHeader { | ||
| rec.status = http.StatusInternalServerError | ||
| } | ||
| serverSpan.RecordError(fmt.Errorf("panic: %v", recovered)) | ||
| serverSpan.SetStatus(codes.Error, "panic") | ||
| endSpan(serverSpan, rec) | ||
| panic(recovered) | ||
| } | ||
| endSpan(serverSpan, rec) |
There was a problem hiding this comment.
The new panic recovery path updates the span (RecordError/SetStatus) and may force a 500 status, but there’s no test covering this behavior. Add a test that triggers a panic in the wrapped handler, asserts the request panics (recovered in the test), and verifies the exported span has http.response.status_code=500 and Status.Code==Error (and ideally an error event/recorded error).
There was a problem hiding this comment.
Valid — a panic recovery test would strengthen coverage. Adding OTel SDK as a test dep already enabled span assertions; a panic test is a good follow-up alongside the existing TestTracingWrapperSpanErrorStatus.
Summary
spanRouteMiddlewareto useruntime.HTTPPattern(Pattern struct set byhandleHandler) instead ofruntime.HTTPPathPattern(string set later insideAnnotateContext). This was causing route patterns to never appear in span names.Test plan
make testpasses with-raceTestTracingWrapperSpanAttributesverifies all span attributesTestTracingWrapperGatewaySpanNameverifies span renamed toGET /api/v1/rules/{rule_id=*}withhttp.routeattributeSummary by CodeRabbit
New Features
Tests