feat: improve HTTP span tracing with route patterns, status codes, and semconv v1.40.0#54
feat: improve HTTP span tracing with route patterns, status codes, and semconv v1.40.0#54
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.
|
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 4 minutes and 46 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. 📝 WalkthroughWalkthroughServer-side HTTP tracing was updated: spans are started using the request method, response status is captured via a new statusRecorder and recorded on span end (5xx -> error), gRPC‑gateway routing middleware updates span name and http.route after routing, and OpenTelemetry semantic conventions and resource attributes were upgraded. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as HTTP Server
participant Router as gRPC‑gateway Router
participant Tracer as OpenTelemetry Tracer
Client->>Server: HTTP request (method + path)
Server->>Tracer: Start span named by method (e.g., "GET")
Server->>Router: Route request (middleware chain)
Router->>Tracer: Update span name to "GET /matched/route" and add http.route
Server->>Server: Handler writes response (Write / WriteHeader)
Server->>Server: statusRecorder captures final status
Server->>Tracer: endSpan records HTTP status and marks error if >=500, then ends span
Server->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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
This PR updates the HTTP server tracing middleware to produce more descriptive OpenTelemetry span names for incoming HTTP requests, aiming to improve trace readability in observability backends.
Changes:
- Replace the hardcoded OTEL span name (
"ServeHTTP") with a dynamic name based onr.Method + " " + r.URL.Path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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.go (1)
181-181: Add a span-name assertion to tracing tests for this behavior change.
core_coverage_test.go(Lines 158-171) only checks HTTP 200 and won’t catch future regressions in span naming. Please add a test assertion that inspects emitted span names fortracingWrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core.go` at line 181, The tracingWrapper now names spans using r.Method+" "+r.URL.Path (see otel.Tracer("coldbrew-http").Start call), so update core_coverage_test.go to assert the emitted span's Name equals that expected value; modify the test to capture spans from the test/exporter used (e.g., the in-memory or test tracer) and add an assertion that the recorded span name matches r.Method+" "+r.URL.Path for the request exercised by the test to prevent future regressions in span naming.
🤖 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`:
- Line 181: Span naming currently uses r.Method+" "+r.URL.Path which creates
high-cardinality span names; change the call to
otel.Tracer("coldbrew-http").Start to use a low-cardinality name (fallback to
method-only, e.g., r.Method) instead of including r.URL.Path, and if you later
obtain a route template set it as an attribute (http.route) on the span
(references: ctx, serverSpan, otel.Tracer("coldbrew-http").Start and the
r.Method+" "+r.URL.Path usage).
---
Nitpick comments:
In `@core.go`:
- Line 181: The tracingWrapper now names spans using r.Method+" "+r.URL.Path
(see otel.Tracer("coldbrew-http").Start call), so update core_coverage_test.go
to assert the emitted span's Name equals that expected value; modify the test to
capture spans from the test/exporter used (e.g., the in-memory or test tracer)
and add an assertion that the recorded span name matches r.Method+" "+r.URL.Path
for the request exercised by the test to prevent future regressions in span
naming.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- 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
ankurs
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core_coverage_test.go (1)
173-287: These tests don't exercise the tracing contract yet.The new cases only assert the HTTP response code and that
nextran, so they would still pass ifendSpanstopped settinghttp.response.status_code, if 5xx spans stopped being marked as errors, or ifspanRouteMiddlewarestopped renaming the span / settinghttp.route. Please capture spans with an in-memory exporter and assert the emitted span name, attributes, and span status directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core_coverage_test.go` around lines 173 - 287, Update the tests to actually verify tracing behavior by installing an in-memory OpenTelemetry exporter and collecting spans emitted by tracingWrapper and spanRouteMiddleware: for TestTracingWrapper_StatusCodes, after calling wrapped.ServeHTTP capture the emitted span and assert its name, that the attribute "http.response.status_code" equals tt.wantStatus and that a 5xx status yields a span with Error status; for TestSpanRouteMiddleware, capture the span emitted by spanRouteMiddleware and assert the span name was renamed to the route, that the "http.route" attribute matches the expected route, and that the span status reflects errors for 5xx responses; ensure you hook/reset the in-memory exporter between subtests and reference tracingWrapper, spanRouteMiddleware and endSpan/statusRecorder where appropriate to locate instrumentation points.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/README.md`:
- Around line 198-201: Document the breaking change that
OTLPUseOpenTracingBridge (struct field OTLPUseOpenTracingBridge) defaulted to
false (was true) by adding a clear note to the release notes and creating a
short migration guide that explains steps to move OpenTracing instrumentation to
OpenTelemetry and how to temporarily enable the bridge; additionally, add a
startup warning log in the telemetry initialization code (where
OTLPUseOpenTracingBridge is read) that emits a one-time warning if
OTLPUseOpenTracingBridge == true advising users to migrate and linking to the
new migration guide.
In `@core.go`:
- Around line 224-225: The middleware currently appends r.URL.RawQuery via
semconv.URLQuery which leaks sensitive data and increases span cardinality;
change this to either (1) gate capturing query strings behind a configuration
flag (e.g., CaptureQueryParams or similar) and only append when enabled, or (2)
scrub/redact sensitive query keys before appending by parsing r.URL.RawQuery,
removing or masking keys like access_token, token, password, email, etc., and
then passing the sanitized string to semconv.URLQuery; update the code that
references r, attrs and semconv.URLQuery to use the config flag or the sanitized
value and ensure the default behavior is to not export raw queries.
- Around line 178-195: statusRecorder currently misses Write and explicit
optional-interface methods and records the first WriteHeader (including 1xx), so
implement a proper Write method on statusRecorder that ensures a default final
status is recorded when Write is called (if no final status yet), modify
WriteHeader to only record the status when code >= 200 and when no final status
has been recorded, and add delegating methods for optional interfaces (Flush,
Hijack, CloseNotify or ReadFrom as appropriate) that forward to the underlying
ResponseWriter (using type assertions) so middleware can use
w.(http.Flusher)/w.(http.Hijacker) transparently; keep Unwrap but ensure all
delegating methods preserve behavior and return underlying errors as-is.
---
Nitpick comments:
In `@core_coverage_test.go`:
- Around line 173-287: Update the tests to actually verify tracing behavior by
installing an in-memory OpenTelemetry exporter and collecting spans emitted by
tracingWrapper and spanRouteMiddleware: for TestTracingWrapper_StatusCodes,
after calling wrapped.ServeHTTP capture the emitted span and assert its name,
that the attribute "http.response.status_code" equals tt.wantStatus and that a
5xx status yields a span with Error status; for TestSpanRouteMiddleware, capture
the span emitted by spanRouteMiddleware and assert the span name was renamed to
the route, that the "http.route" attribute matches the expected route, and that
the span status reflects errors for 5xx responses; ensure you hook/reset the
in-memory exporter between subtests and reference tracingWrapper,
spanRouteMiddleware and endSpan/statusRecorder where appropriate to locate
instrumentation points.
🪄 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: e12b34fd-ed4e-4803-b27a-14093036f38c
📒 Files selected for processing (4)
config/README.mdcore.gocore_coverage_test.goinitializers.go
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 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 4 out of 4 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.
Actionable comments posted: 2
🤖 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 219-239: In httpSpanAttributes, replace the conditional that reads
r.URL.Scheme with logic that derives the URL scheme from the TLS connection by
checking r.TLS != nil (set "https" when non-nil, otherwise "http") and append
semconv.URLScheme(...) accordingly; update the httpSpanAttributes function to
use this TLS-based scheme determination so spans capture the actual inbound
request scheme.
- Around line 248-264: The defer that calls endSpan(serverSpan, rec) runs during
panic unwinding and can record a 200 because statusRecorder.wroteHeader is still
false; change the wrapper so panics are detected and treated as 500 before
ending the span: replace the simple defer endSpan(serverSpan, rec) with a
recovery-aware wrapper around han that uses defer+recover to check for a panic,
and if recover() != nil and rec.wroteHeader == false set rec.status =
http.StatusInternalServerError (or otherwise ensure endSpan treats "no header
written" as 500) then call endSpan(serverSpan, rec) and re-panic to preserve
behavior; reference the statusRecorder struct, rec variable, endSpan function,
and the call to han(w, r.WithContext(ctx)) when making this change.
🪄 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: 679ab95c-aef6-43ae-9c81-1fc5372746ff
📒 Files selected for processing (2)
core.goinitializers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- initializers.go
| func httpSpanAttributes(r *http.Request) []attribute.KeyValue { | ||
| host, port, err := net.SplitHostPort(r.Host) | ||
| if err != nil { | ||
| host = r.Host | ||
| } | ||
| attrs := []attribute.KeyValue{ | ||
| semconv.HTTPRequestMethodKey.String(r.Method), | ||
| semconv.URLPath(r.URL.Path), | ||
| semconv.ServerAddress(host), | ||
| } | ||
| if port != "" { | ||
| if p, err := strconv.Atoi(port); err == nil { | ||
| attrs = append(attrs, semconv.ServerPort(p)) | ||
| } | ||
| } | ||
| if r.URL.RawQuery != "" { | ||
| attrs = append(attrs, semconv.URLQuery(r.URL.RawQuery)) | ||
| } | ||
| if r.URL.Scheme != "" { | ||
| attrs = append(attrs, semconv.URLScheme(r.URL.Scheme)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the core.go file
find . -name "core.go" -type f | head -20Repository: go-coldbrew/core
Length of output: 69
🏁 Script executed:
# Look for the httpSpanAttributes function to confirm the code matches the review
rg -n "func httpSpanAttributes" --type go -A 25Repository: go-coldbrew/core
Length of output: 1131
🏁 Script executed:
# Check if there's any proxy header handling (X-Forwarded-Proto, etc.) in the codebase
rg -i "forwarded-proto|x-forwarded|proto" --type goRepository: go-coldbrew/core
Length of output: 4179
🌐 Web query:
Go http.Request URL.Scheme behavior inbound requests net/http server
💡 Result:
In Go's net/http server, for inbound requests, http.Request.URL.Scheme is always an empty string (""). The server does not set it. The URL field in http.Request is populated by parsing the request-target from the HTTP Request-Line (stored in RequestURI) using url.ParseRequestURI (for HTTP/1.x via readRequest in request.go). Per RFC 7230 Section 5.3, request-targets in HTTP/1.x are typically relative URIs (absolute-path [ "?" query ]) without scheme or authority for non-authority forms (like origin-form used in direct requests). Thus, when parsed, the Scheme field remains empty, while Path and RawQuery are populated. Official documentation confirms: "For server requests, the URL is parsed from the URI supplied on the Request-Line as stored in RequestURI. For most requests, fields other than Path and RawQuery will be empty." (pkg.go.dev/net/http#Request) The scheme (http or https) is determined by the connection: check if r.TLS != nil ("https") or nil ("http"). The server sets r.TLS for TLS-enabled connections before invoking the handler. Behind proxies/load balancers, use headers like X-Forwarded-Proto (via Server.ProxyHeaders if configured). To construct full URL: scheme + "://" + r.Host + r.URL.RequestURI
Citations:
- 1: https://pkg.go.dev/net/http
- 2: https://pkg.go.dev/net/http@go1.19
- 3: net/http/httptest: NewRequest easily misused as client side requests with full URLs golang/go#73151
- 4: https://stackoverflow.com/questions/40826664/get-scheme-of-the-current-request-url
- 5: https://github.com/golang/go/blob/master/src/net/http/request.go
- 6: https://pkg.go.dev/net/http@go1.25
- 7: https://go.dev/src/net/http/request.go
- 8: https://go.dev/src/net/http/request.go?h=Reader
- 9: net/http: *Request.URL.Scheme returns an empty string. No alternative way present to get request url scheme. golang/go#28940
Derive url.scheme from the TLS connection rather than r.URL.Scheme.
On inbound server requests, r.URL.Scheme is always empty per Go's http.Request semantics (RFC 7230). Check r.TLS != nil to determine HTTPS vs HTTP instead, so spans always capture the actual scheme.
if r.URL.RawQuery != "" {
attrs = append(attrs, semconv.URLQuery(r.URL.RawQuery))
}
- if r.URL.Scheme != "" {
- attrs = append(attrs, semconv.URLScheme(r.URL.Scheme))
- }
+ switch {
+ case r.URL.Scheme != "":
+ attrs = append(attrs, semconv.URLScheme(r.URL.Scheme))
+ case r.TLS != nil:
+ attrs = append(attrs, semconv.URLScheme("https"))
+ default:
+ attrs = append(attrs, semconv.URLScheme("http"))
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core.go` around lines 219 - 239, In httpSpanAttributes, replace the
conditional that reads r.URL.Scheme with logic that derives the URL scheme from
the TLS connection by checking r.TLS != nil (set "https" when non-nil, otherwise
"http") and append semconv.URLScheme(...) accordingly; update the
httpSpanAttributes function to use this TLS-based scheme determination so spans
capture the actual inbound request scheme.
| ctx := otel.GetTextMapPropagator().Extract(r.Context(), propagation.HeaderCarrier(r.Header)) | ||
|
|
||
| if interceptors.FilterMethodsFunc(ctx, r.URL.Path) { | ||
| var serverSpan oteltrace.Span | ||
| ctx, serverSpan = otel.Tracer("coldbrew-http").Start(ctx, "ServeHTTP", | ||
| ctx, serverSpan = otel.Tracer("coldbrew-http").Start(ctx, r.Method, | ||
| oteltrace.WithSpanKind(oteltrace.SpanKindServer), | ||
| oteltrace.WithAttributes( | ||
| semconv.HTTPMethodKey.String(r.Method), | ||
| semconv.HTTPTargetKey.String(r.URL.RequestURI()), | ||
| ), | ||
| oteltrace.WithAttributes(httpSpanAttributes(r)...), | ||
| ) | ||
| r = r.WithContext(ctx) | ||
| defer serverSpan.End() | ||
| } else { | ||
| r = r.WithContext(ctx) | ||
| rec := &statusRecorder{ResponseWriter: w, status: http.StatusOK} | ||
| w = rec | ||
| defer endSpan(serverSpan, rec) | ||
| } | ||
|
|
||
| _, han := interceptors.NRHttpTracer("", h.ServeHTTP) | ||
| // add this info to log | ||
| ctx = r.Context() | ||
| ctx = options.AddToOptions(ctx, "", "") | ||
| ctx = loggers.AddToLogContext(ctx, "httpPath", r.URL.Path) | ||
| r = r.WithContext(ctx) | ||
| han(w, r) | ||
| han(w, r.WithContext(ctx)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "\.go$" | head -20Repository: go-coldbrew/core
Length of output: 173
🏁 Script executed:
# Find the core.go file and examine the statusRecorder and endSpan implementations
fd -e go -name "core.go"Repository: go-coldbrew/core
Length of output: 290
🏁 Script executed:
# Get the statusRecorder and endSpan definitions
rg "type statusRecorder" -A 20Repository: go-coldbrew/core
Length of output: 702
🏁 Script executed:
# Find the endSpan function
rg "func endSpan" -A 15Repository: go-coldbrew/core
Length of output: 772
🏁 Script executed:
# Look at the full context around lines 248-264
sed -n '240,270p' core.goRepository: go-coldbrew/core
Length of output: 1302
🏁 Script executed:
# Check if there are any tests that verify panic handling
rg "panic" core_test.go -B 3 -A 10Repository: go-coldbrew/core
Length of output: 42
🏁 Script executed:
# Look for any existing panic recovery in the codebase
rg "recover\(\)" -B 3 -A 3Repository: go-coldbrew/core
Length of output: 832
🌐 Web query:
Go net/http Server panic recovery behavior writes 500 status code
💡 Result:
No, the standard Go net/http server does not write a 500 status code when recovering from a panic in a handler. Instead, it recovers from the panic (except for http.ErrAbortHandler), logs the panic details and stack trace to the server error log with a message like "http: panic serving [addr]: [error]\n[stack]", and then aborts the response by closing the network connection (for HTTP/1) or sending an HTTP/2 RST_STREAM. This typically results in the client receiving no response body and either a connection close (which may appear as an empty reply or connection reset, depending on the client) or an empty/incomplete response without a 500 status. This behavior is intentional to isolate the failed request and prevent it from crashing the server process, as documented in the net/http package and visible in the server.go source code around the defer recover in (*conn).serve. Custom middleware is commonly used to instead return a proper 500 Internal Server Error response, log the error, and continue.
Citations:
- 1: https://pkg.go.dev/net/http@go1.25
- 2: https://go.dev/src/net/http/server.go
- 3: https://cwrk.ai/blog/2025/03/handling-panic-in-a-go-api-server/
- 4: https://iximiuz.com/en/posts/go-http-handlers-panic-and-deadlocks/
- 5: proposal: allow net/http server to fail on panic instead of recovering golang/go#16542
🏁 Script executed:
rg "NRHttpTracer" -B 2 -A 10Repository: go-coldbrew/core
Length of output: 678
🏁 Script executed:
# Also search for its definition or implementation
rg "func NRHttpTracer" -A 20Repository: go-coldbrew/core
Length of output: 42
Unhandled panics will be recorded as successful (200 status) in traces.
If the wrapped handler panics before writing a response, the defer executes during panic unwinding and records the span with the initial status (200). Since wroteHeader remains false, no actual response was sent to the client, but the trace will show a successful request. This misleads debugging and monitoring.
Suggested fix
if interceptors.FilterMethodsFunc(ctx, r.URL.Path) {
var serverSpan oteltrace.Span
ctx, serverSpan = otel.Tracer("coldbrew-http").Start(ctx, r.Method,
oteltrace.WithSpanKind(oteltrace.SpanKindServer),
oteltrace.WithAttributes(httpSpanAttributes(r)...),
)
rec := &statusRecorder{ResponseWriter: w, status: http.StatusOK}
w = rec
- defer endSpan(serverSpan, rec)
+ defer func() {
+ if p := recover(); p != nil {
+ if !rec.wroteHeader {
+ rec.status = http.StatusInternalServerError
+ rec.wroteHeader = true
+ }
+ endSpan(serverSpan, rec)
+ panic(p)
+ }
+ endSpan(serverSpan, rec)
+ }()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core.go` around lines 248 - 264, The defer that calls endSpan(serverSpan,
rec) runs during panic unwinding and can record a 200 because
statusRecorder.wroteHeader is still false; change the wrapper so panics are
detected and treated as 500 before ending the span: replace the simple defer
endSpan(serverSpan, rec) with a recovery-aware wrapper around han that uses
defer+recover to check for a panic, and if recover() != nil and rec.wroteHeader
== false set rec.status = http.StatusInternalServerError (or otherwise ensure
endSpan treats "no header written" as 500) then call endSpan(serverSpan, rec)
and re-panic to preserve behavior; reference the statusRecorder struct, rec
variable, endSpan function, and the call to han(w, r.WithContext(ctx)) when
making this change.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| t.Fatalf("underlying writer expected %d, got %d", http.StatusBadGateway, w.Code) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
statusRecorder has special handling for 1xx (skip) and 101 Switching Protocols (terminal) in WriteHeader, but the tests only cover >=200 statuses. Add a test case that exercises WriteHeader(http.StatusSwitchingProtocols) (and ideally a 1xx-then-200 sequence) to prevent regressions in this branch logic.
| t.Run("switching protocols is terminal", func(t *testing.T) { | |
| t.Parallel() | |
| w := httptest.NewRecorder() | |
| rec := &statusRecorder{ResponseWriter: w, status: http.StatusOK} | |
| rec.WriteHeader(http.StatusSwitchingProtocols) | |
| // A later attempt to change the status should be ignored once 101 is written. | |
| rec.WriteHeader(http.StatusBadGateway) | |
| if rec.status != http.StatusSwitchingProtocols { | |
| t.Fatalf("expected recorder status %d, got %d", http.StatusSwitchingProtocols, rec.status) | |
| } | |
| if w.Code != http.StatusSwitchingProtocols { | |
| t.Fatalf("underlying writer expected %d, got %d", http.StatusSwitchingProtocols, w.Code) | |
| } | |
| }) | |
| t.Run("informational status followed by final status", func(t *testing.T) { | |
| t.Parallel() | |
| w := httptest.NewRecorder() | |
| rec := &statusRecorder{ResponseWriter: w, status: http.StatusOK} | |
| // First write a non-101 informational status; this should be skipped. | |
| rec.WriteHeader(102) | |
| // Then write a final status, which should be recorded and sent. | |
| rec.WriteHeader(http.StatusOK) | |
| if rec.status != http.StatusOK { | |
| t.Fatalf("expected recorder status %d after informational, got %d", http.StatusOK, rec.status) | |
| } | |
| if w.Code != http.StatusOK { | |
| t.Fatalf("underlying writer expected %d after informational, got %d", http.StatusOK, w.Code) | |
| } | |
| }) |
Summary
"ServeHTTP"span name with low-cardinality route patterns via grpc-gateway middleware (spanRouteMiddleware) — spans now showGET /api/v1/{resource}instead ofServeHTTPhttp.response.status_codeon every HTTP span usingstatusRecorderwrapper; mark 5xx responses as span errorshttp.request.method,url.path,url.query,http.route, etc.)tracingWrapper: extractendSpan()andhttpSpanAttributes(), consolidater.WithContext()calls, skip empty attributesTest plan
make testpasses with-racemake buildcompiles cleanlySummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores