feat(httpserver): split OTLP debug attributes into structured sub-attributes#579
Merged
Merged
Conversation
…ributes Replace the single monolithic `http.request` and `http.response` span attributes with structured sub-attributes for better observability: - http.request.method, http.request.url, http.request.host, http.request.proto - http.request.headers, http.request.body - http.response.headers, http.response.body This makes it possible to filter and search on individual attributes in observability tools (Signoz, Jaeger, etc.) instead of parsing a raw dump.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughMiddleware replaces raw request/response dumps with structured span attributes (method, url, host, proto, headers, conditional JSON body capture), restores request body for handlers, and buffers responses for status/body attributes; tests add a logger Enabled method; LocalStack test harness now starts a shared server with adjusted container env/port binding. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MW as Middleware
participant Handler as Handler
participant RW as BufferedResponseWriter
participant Tracer as Tracer
Client->>MW: HTTP request
MW->>MW: Set span attrs: method, url, host, proto, headers
alt debug && content-type contains "application/json"
MW->>MW: Read request body\nSet span attr: http.request.body\nRestore r.Body
end
MW->>Handler: Forward request
Handler->>RW: Write status, headers, body
RW->>MW: Buffered response captured
MW->>MW: Set span attrs: http.response.status_code, http.response.headers
alt debug && captured JSON body
MW->>MW: Set span attr: http.response.body
end
MW->>Tracer: Record/end span
MW->>Client: Flush buffered response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/transport/httpserver/otlp_middleware.go (2)
88-88: Reuse thespanvariable from line 45.
spanis already captured in the enclosing scope; callingtrace.SpanFromContextagain is unnecessary.♻️ Proposed fix
- trace.SpanFromContext(r.Context()).SetAttributes(respAttrs...) + span.SetAttributes(respAttrs...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/transport/httpserver/otlp_middleware.go` at line 88, The code redundantly calls trace.SpanFromContext(r.Context()) to set attributes even though the enclosing scope already captures the span in the variable named span; replace trace.SpanFromContext(r.Context()).SetAttributes(respAttrs...) with span.SetAttributes(respAttrs...) (or guard with a nil/IsRecording() check if needed) to reuse the existing span variable in otlp_middleware.go.
62-69: Consider limiting body size and usingbytes.NewReader.Two concerns with the current body handling:
Unbounded memory usage:
io.ReadAllconsumes the entire request body. In debug mode on high-traffic services, large request bodies (file uploads, batch payloads) could cause memory pressure.Inefficient conversion:
strings.NewReader(string(body))performs an unnecessary[]byte→string→ reader conversion.♻️ Proposed fix
Add
bytesimport and apply:+import "bytes" ... // Body if r.Body != nil { - body, err := io.ReadAll(r.Body) + body, err := io.ReadAll(io.LimitReader(r.Body, 64*1024)) // 64KB limit if err == nil { attrs = append(attrs, attribute.String("http.request.body", string(body))) - r.Body = io.NopCloser(strings.NewReader(string(body))) + r.Body = io.NopCloser(bytes.NewReader(body)) } }Note: With
io.LimitReader, truncated bodies won't match actual request content. You may want to append a marker (e.g.,"[truncated]") or set a separate attribute indicating truncation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/transport/httpserver/otlp_middleware.go` around lines 62 - 69, The current request-body handling uses io.ReadAll and strings.NewReader which can allocate unbounded memory and double-convert bytes→string; change it to read a bounded amount using io.LimitReader (e.g., wrap r.Body with io.LimitReader using a defined maxBodySize constant), read into a []byte buffer, and replace r.Body with bytes.NewReader(buf) (or io.NopCloser(bytes.NewReader(buf))) to avoid the string conversion; also detect if the original body was larger than the limit and append a truncation marker or separate attribute (e.g., "http.request.body_truncated") when adding attribute.String("http.request.body", ...) so callers know the payload was truncated — update imports to include "bytes" and remove the unnecessary "strings" usage and adjust error handling around the read.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/transport/httpserver/otlp_middleware.go`:
- Line 88: The code redundantly calls trace.SpanFromContext(r.Context()) to set
attributes even though the enclosing scope already captures the span in the
variable named span; replace
trace.SpanFromContext(r.Context()).SetAttributes(respAttrs...) with
span.SetAttributes(respAttrs...) (or guard with a nil/IsRecording() check if
needed) to reuse the existing span variable in otlp_middleware.go.
- Around line 62-69: The current request-body handling uses io.ReadAll and
strings.NewReader which can allocate unbounded memory and double-convert
bytes→string; change it to read a bounded amount using io.LimitReader (e.g.,
wrap r.Body with io.LimitReader using a defined maxBodySize constant), read into
a []byte buffer, and replace r.Body with bytes.NewReader(buf) (or
io.NopCloser(bytes.NewReader(buf))) to avoid the string conversion; also detect
if the original body was larger than the limit and append a truncation marker or
separate attribute (e.g., "http.request.body_truncated") when adding
attribute.String("http.request.body", ...) so callers know the payload was
truncated — update imports to include "bytes" and remove the unnecessary
"strings" usage and adjust error handling around the read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef201f9a-cfde-44e6-b9f3-03e1d058ce15
📒 Files selected for processing (1)
pkg/transport/httpserver/otlp_middleware.go
The Logger interface was updated with an Enabled(level) method in 21e6135 but the mock in licence_test.go was not updated, breaking compilation.
fguery
previously approved these changes
Mar 23, 2026
| func (m *mockLogger) Warnf(format string, args ...any) {} | ||
| func (m *mockLogger) Debug(args ...any) {} | ||
| func (m *mockLogger) Debugf(format string, args ...any) {} | ||
| func (m *mockLogger) Enabled(level logging.Level) bool { return true } |
…equests - Always: method, url, host, proto, response status_code - Debug only: request/response headers and body - Limit debug body capture to 64KB (io.LimitReader) - Use bytes.NewReader instead of strings.NewReader for body restore - Reuse span variable in defer (CodeRabbit nitpick) - Capture response status_code via responseWriter.WriteHeader Addresses review feedback from @fguery and CodeRabbit.
- Remove arbitrary 64KB body size limit - Only capture request/response body when Content-Type is application/json - Skip body buffering entirely for non-JSON responses (no perf overhead) - Keep always-on: method, url, host, proto, status_code - Keep debug-only: headers, body (JSON only)
LocalStack now requires LOCALSTACK_AUTH_TOKEN or acknowledgement env var. - Add LOCALSTACK_ACKNOWLEDGE_ACCOUNT_REQUIREMENT=1 to localstacktesting wrapper - Migrate queue integration tests from elgohr/go-localstack to our own localstacktesting wrapper (elgohr lib doesn't support custom env vars) - Remove unused elgohr/go-localstack dependency
The host port was hardcoded to 4566, causing "port already allocated" errors when multiple localstack test suites run in parallel. Let Docker allocate a random available port instead.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #579 +/- ##
==========================================
- Coverage 25.92% 25.65% -0.27%
==========================================
Files 180 180
Lines 7098 7163 +65
==========================================
- Hits 1840 1838 -2
- Misses 5146 5214 +68
+ Partials 112 111 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/transport/httpserver/otlp_middleware.go (1)
82-88:⚠️ Potential issue | 🟡 MinorRequest body not restored if
io.ReadAllfails.If
io.ReadAllreturns an error (e.g., client disconnects mid-send), the originalr.Bodyis already consumed, but the code doesn't restore it. Downstream handlers will receive an empty or broken body.🛡️ Proposed fix: always attempt to restore body
// Debug: request body (JSON only) if captureBody && r.Body != nil { body, err := io.ReadAll(r.Body) + // Always restore body with whatever was read + r.Body = io.NopCloser(bytes.NewReader(body)) if err == nil { span.SetAttributes(attribute.String("http.request.body", string(body))) - r.Body = io.NopCloser(bytes.NewReader(body)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/transport/httpserver/otlp_middleware.go` around lines 82 - 88, When capturing the request body in the otlp middleware (see captureBody, r.Body and io.ReadAll usage), ensure the request body is always restored for downstream handlers even if io.ReadAll returns an error: read r.Body into a byte slice, and regardless of whether ReadAll returned an error, reconstruct r.Body as io.NopCloser(bytes.NewReader(body)) so downstream handlers get the (possibly partial) bytes; still set span attributes only on successful full reads (span.SetAttributes), and consider logging the read error, but do not leave r.Body consumed when io.ReadAll fails.
🧹 Nitpick comments (1)
pkg/transport/httpserver/otlp_middleware.go (1)
47-50: Consider logging instead of panicking on write failure.If the client disconnects mid-response,
Writewill fail and panic will crash the goroutine. While recovery middleware may catch this, logging the error would be more graceful.♻️ Suggested improvement
- _, err := w.ResponseWriter.Write(w.data) - if err != nil { - panic(err) - } + if _, err := w.ResponseWriter.Write(w.data); err != nil { + // Client may have disconnected; log and continue + // Consider injecting a logger if needed + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/transport/httpserver/otlp_middleware.go` around lines 47 - 50, The write error handling in the OTLP middleware currently panics on failure (the block calling w.ResponseWriter.Write(w.data) and checking err), which is harsh for client disconnects; replace the panic with a graceful log of the error using the repository's logger (or the standard logger if none exists) and return immediately from the handler so the request finishes cleanly. Locate the write in otlp_middleware.go where w.ResponseWriter.Write(w.data) is used and change the panic(err) to a logger call (including context like the request path/remote addr if available) and ensure the function exits after logging instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/transport/httpserver/otlp_middleware.go`:
- Line 71: The current logic uses captureBody (set from r.Header via
isJSONContent) for both request and response capture; change this to two
separate booleans—captureRequestBody (based on r.Header and isJSONContent) and
captureResponseBody (determined from the response Content-Type after the handler
runs, using the wrapped response headers via the response recorder/writer) and
only capture/append the response body when captureResponseBody is true; update
the middleware around the handler invocation (the response buffering/wrapping
code and the section referencing captureBody, e.g., the variable captureBody and
the response-handling block) to buffer responses in debug mode so you can
inspect the response Content-Type, then decide to log the response body if
captureResponseBody is true.
- Around line 15-27: The responseWriter wrapper currently only implements
WriteHeader and doesn't delegate optional interfaces (http.Flusher,
http.Hijacker, http.CloseNotifier and http.Pusher) so callers lose access to
streaming, hijack, push and close-notify behavior; add methods on type
responseWriter: Flush() to forward to the embedded ResponseWriter if it
implements http.Flusher, Hijack() (returning net.Conn, *bufio.ReadWriter, error)
to forward to http.Hijacker on the underlying writer, CloseNotify() to forward
to http.CloseNotifier (or an equivalent channel) and Push(target string, opts
*http.PushOptions) to forward to http.Pusher, each performing a type assertion
and delegating, and add the required imports ("bufio" and "net").
---
Duplicate comments:
In `@pkg/transport/httpserver/otlp_middleware.go`:
- Around line 82-88: When capturing the request body in the otlp middleware (see
captureBody, r.Body and io.ReadAll usage), ensure the request body is always
restored for downstream handlers even if io.ReadAll returns an error: read
r.Body into a byte slice, and regardless of whether ReadAll returned an error,
reconstruct r.Body as io.NopCloser(bytes.NewReader(body)) so downstream handlers
get the (possibly partial) bytes; still set span attributes only on successful
full reads (span.SetAttributes), and consider logging the read error, but do not
leave r.Body consumed when io.ReadAll fails.
---
Nitpick comments:
In `@pkg/transport/httpserver/otlp_middleware.go`:
- Around line 47-50: The write error handling in the OTLP middleware currently
panics on failure (the block calling w.ResponseWriter.Write(w.data) and checking
err), which is harsh for client disconnects; replace the panic with a graceful
log of the error using the repository's logger (or the standard logger if none
exists) and return immediately from the handler so the request finishes cleanly.
Locate the write in otlp_middleware.go where w.ResponseWriter.Write(w.data) is
used and change the panic(err) to a logger call (including context like the
request path/remote addr if available) and ensure the function exits after
logging instead of panicking.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1773d778-e382-4b11-a839-2677bbcfddf9
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
pkg/messaging/queue/localstack_integration_test.gopkg/testing/platform/localstacktesting/localstack.gopkg/transport/httpserver/otlp_middleware.go
Comment on lines
15
to
27
| type responseWriter struct { | ||
| http.ResponseWriter | ||
| data []byte | ||
| data []byte | ||
| statusCode int | ||
| captureBody bool | ||
| } | ||
|
|
||
| func (w *responseWriter) WriteHeader(code int) { | ||
| w.statusCode = code | ||
| if !w.captureBody { | ||
| w.ResponseWriter.WriteHeader(code) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing http.Flusher, http.Hijacker, and http.CloseNotifier interface implementations.
The responseWriter wrapper embeds http.ResponseWriter but does not expose the optional interfaces that the underlying writer may implement. This will break:
- SSE / streaming: Handlers calling
w.(http.Flusher).Flush()will panic or get a false type assertion. - WebSocket upgrades:
w.(http.Hijacker).Hijack()will fail, breaking upgrade handshakes. - HTTP/2 Server Push:
w.(http.Pusher)won't work.
🔧 Proposed fix: delegate optional interfaces to underlying writer
+func (w *responseWriter) Flush() {
+ if f, ok := w.ResponseWriter.(http.Flusher); ok {
+ f.Flush()
+ }
+}
+
+func (w *responseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
+ if h, ok := w.ResponseWriter.(http.Hijacker); ok {
+ return h.Hijack()
+ }
+ return nil, nil, fmt.Errorf("hijacking not supported")
+}You'll also need to add "bufio" and "net" imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/transport/httpserver/otlp_middleware.go` around lines 15 - 27, The
responseWriter wrapper currently only implements WriteHeader and doesn't
delegate optional interfaces (http.Flusher, http.Hijacker, http.CloseNotifier
and http.Pusher) so callers lose access to streaming, hijack, push and
close-notify behavior; add methods on type responseWriter: Flush() to forward to
the embedded ResponseWriter if it implements http.Flusher, Hijack() (returning
net.Conn, *bufio.ReadWriter, error) to forward to http.Hijacker on the
underlying writer, CloseNotify() to forward to http.CloseNotifier (or an
equivalent channel) and Push(target string, opts *http.PushOptions) to forward
to http.Pusher, each performing a type assertion and delegating, and add the
required imports ("bufio" and "net").
| attribute.String("http.request.proto", r.Proto), | ||
| ) | ||
|
|
||
| captureBody := debug && isJSONContent(r.Header.Get("Content-Type")) |
There was a problem hiding this comment.
Response body capture is conditional on request Content-Type, not response Content-Type.
captureBody is determined by the request's Content-Type (line 71), but it's also used to decide whether to capture the response body (line 115). This means:
- A
POST application/x-www-form-urlencodedrequest to a JSON API won't have its JSON response captured. - Conversely, a JSON request returning an HTML error page would attempt capture (though this may be acceptable).
Consider checking the response's Content-Type header for response body capture instead.
🔧 Proposed fix: separate capture decisions for request and response
captureBody := debug && isJSONContent(r.Header.Get("Content-Type"))
+ captureResponseBody := false
// ... existing code ...
rw := &responseWriter{
ResponseWriter: w,
data: make([]byte, 0, 1024),
- captureBody: captureBody,
+ captureBody: debug, // Capture if debug, check content-type later
}
defer func() {
rw.finalize()
// ... status code logic ...
if debug {
// Debug: response headers
// ... existing header code ...
// Debug: response body (only if JSON content)
- if captureBody && len(rw.data) > 0 {
+ if isJSONContent(rw.Header().Get("Content-Type")) && len(rw.data) > 0 {
span.SetAttributes(attribute.String("http.response.body", string(rw.data)))
}
}
}()Note: This would also require buffering all debug responses, which may have performance implications. An alternative is to accept the current behavior as a known limitation and document it.
Also applies to: 114-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/transport/httpserver/otlp_middleware.go` at line 71, The current logic
uses captureBody (set from r.Header via isJSONContent) for both request and
response capture; change this to two separate booleans—captureRequestBody (based
on r.Header and isJSONContent) and captureResponseBody (determined from the
response Content-Type after the handler runs, using the wrapped response headers
via the response recorder/writer) and only capture/append the response body when
captureResponseBody is true; update the middleware around the handler invocation
(the response buffering/wrapping code and the section referencing captureBody,
e.g., the variable captureBody and the response-handling block) to buffer
responses in debug mode so you can inspect the response Content-Type, then
decide to log the response body if captureResponseBody is true.
- Dirty: runs pre-commit (tidy, generate, lint) and checks for uncommitted changes - Tests: runs test suite and uploads coverage - Both jobs run in parallel for faster feedback - Remove Dependabot auto-merge (no longer needed)
fguery
approved these changes
Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
http.request/http.responsespan attributes with structured sub-attributes in debug modehttp.request.method,http.request.url,http.request.host,http.request.proto,http.request.headers,http.request.body,http.response.headers,http.response.bodyTest plan
go build ./pkg/transport/httpserver/...)debug=trueand verify structured attributes appear in traces