fix(v3/otel): avoid stream replacement for static responses#1902
fix(v3/otel): avoid stream replacement for static responses#1902edgarsilva wants to merge 2 commits into
Conversation
Stop wrapping request/response body streams for size metrics so middleware no longer triggers fasthttp stream reset/close side effects on static file readers. Add regression coverage for repeated static and .well-known requests while preserving size metrics when stream length is known.
edgarsilva
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the OpenTelemetry middleware to improve how request and response body sizes are determined. The previous implementation, which wrapped body streams to count bytes during transit, has been replaced with a more direct approach that leverages the 'Content-Length' header or the stream's internal length where available. This change simplifies the logic and addresses potential hanging issues, as verified by the newly added tests for static assets and 404 responses. Feedback was provided to use a case-insensitive comparison when identifying Server-Sent Events (SSE) to ensure the middleware correctly skips size calculations for those streams.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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. WalkthroughThis PR fixes a critical bug in the OTEL Fiber middleware that caused nil pointer dereferences and request hangs when tracing static assets. It replaces the unsafe ChangesBody Size Tracking and Metrics Recording Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 (1)
v3/otel/fiber_stream_test.go (1)
36-48: 💤 Low valueOptional: use Go 1.22+
for range Nto drop unused loop variables.The module targets Go 1.25.0, supporting the
for range Nsyntax. Variableiis unused in both nested loops at lines 36–48 and the loop at line 57. Simplify tofor range 25for clarity.♻️ Suggested changes
- for i := 0; i < 25; i++ { + for range 25 { for _, tc := range testCases {Apply the same to line 57 in
TestMiddleware_NotFoundPathDoesNotHang.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/otel/fiber_stream_test.go` around lines 36 - 48, Replace the unused index loops with Go 1.22+ "for range N" form: in fiber_stream_test.go change the outer loop in the test that iterates 25 times (currently written as "for i := 0; i < 25; i++") to "for range 25" to drop the unused variable, and do the same for the similar repetition loop in TestMiddleware_NotFoundPathDoesNotHang; update the loop surrounding the inner table-driven loop over testCases (and any other identical repetition loops) so the inner variable names (e.g., tc) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v3/otel/fiber_stream_test.go`:
- Around line 36-48: Replace the unused index loops with Go 1.22+ "for range N"
form: in fiber_stream_test.go change the outer loop in the test that iterates 25
times (currently written as "for i := 0; i < 25; i++") to "for range 25" to drop
the unused variable, and do the same for the similar repetition loop in
TestMiddleware_NotFoundPathDoesNotHang; update the loop surrounding the inner
table-driven loop over testCases (and any other identical repetition loops) so
the inner variable names (e.g., tc) remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bff155af-b5a5-4ac8-b992-a54c2751500a
📒 Files selected for processing (2)
v3/otel/fiber.gov3/otel/fiber_stream_test.go
📜 Review details
🔇 Additional comments (5)
v3/otel/fiber.go (4)
22-42: Helper looks correct and minimal.The non-wrapping approach via type assertions on
*io.LimitedReaderand the locally-declaredlenReaderinterface cleanly addresses the root cause from#1734(noSetBodyStreammutation, so fasthttp's static reader cleanup is preserved). The>= 0guards correctly exclude fasthttp's sentinel Content-Length values (-1for chunked,-2for identity) when this helper is used downstream.
147-161: Request size capture order is correct.Capturing
requestSizebeforec.Next()(line 188) ensuresLen()/LimitedReader.Nreflect the full body, not a partially-consumed remainder. No issue.
221-243: Metric/attribute gating is consistent.Both the deferred histogram recording and the span attribute set use
responseSizeKnown(andrequestSizeKnown) consistently, so unknown sizes are no longer recorded as0. This is a small correctness improvement on top of the hang fix.
201-219: ⚡ Quick winThe fix correctly avoids stream-wrapping side effects by only inspecting the stream, not replacing it.
The original concern was well-founded given the prior
SetBodyStreamissues, but the implementation is safe. ThebodyStreamSize()function performs only read-only inspections (accessing.Len()property or type-asserting to*io.LimitedReader) without consuming, closing, or modifying the stream. Regression tests confirm repeated static file and 404 requests work correctly across 25 iterations without hangs or body corruption—directly validating that callingresponse.BodyStream()here is side-effect-free.v3/otel/fiber_stream_test.go (1)
51-66: Targeted regression test for the/.well-known/...404 hang case.Coverage matches the symptom path called out in the linked issue.
app.Testwill surface a real hang via its default deadline, andio.ReadAll+Body.Close()ensures the response is fully drained per iteration.
Treat response Content-Type as media type plus optional parameters and match SSE values case-insensitively so event streams are consistently excluded from body-size calculation.
2 participants
Summary
v3/otelmiddleware, which avoidsSetBodyStream->ResetBodyside effects that can close fasthttp static readers and stall/panic requestsContent-Length,*io.LimitedReader, orLen()when available/.well-known/...not-found requests to verify middleware does not hang responsesValidation
go test ./...inv3/otelFixes #1734.
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements