proxy,ui-svelte: add request/response capturing#508
Conversation
- ReqRespCapture keeps the request and response headers and body. - add an API endpoint to possibly fetch the capture record - add config option to enable captures
|
Caution Review failedThe pull request is closed. WalkthroughAdds a numeric captureBuffer config option and Config field; implements in-memory request/response capture storage (size accounting, FIFO eviction, redaction) in the metrics monitor; exposes a GET /api/captures/:id endpoint; adds UI types, client call, modal component and Activity integration; and updates tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Remove overflow-hidden and h-full that caused Safari to compute dialog height as 0px, making content invisible despite backdrop showing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ui-svelte/src/components/CaptureDialog.svelte`:
- Around line 26-33: The decodeBody function uses atob which yields a Latin-1
string and corrupts multi-byte UTF-8; change decodeBody (function decodeBody) to
base64-decode into a byte array and then decode with TextDecoder('utf-8') to
reconstruct UTF-8 characters reliably, e.g., call atob(body) to get the binary
string, convert each char to its code unit into a Uint8Array, pass that to new
TextDecoder().decode(...), and keep the existing try/catch to fallback to
returning the original body on any error.
Redact Authorization, Proxy-Authorization, Cookie, Set-Cookie, and X-Api-Key headers to prevent credential leakage in stored captures.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/metrics_monitor.go (1)
214-217:⚠️ Potential issue | 🟠 MajorNon-200 responses are never captured — this undermines the core debugging use-case.
Issue
#464was specifically about debugging a 401 error from a peer. The current implementation returns early for any non-OK status without storing a capture, which means the exact scenario that motivated this feature won't produce captures.Consider moving the capture-storage logic before this early return (or into a
defer), so that request/response data for error responses is also captured. At minimum, the request headers/body and the error status code would be valuable for debugging.
🤖 Fix all issues with AI agents
In `@proxy/metrics_monitor_test.go`:
- Around line 988-1027: The test TestMetricsMonitor_WrapHandler_Capture is
missing an assertion that the recorded metric's HasCapture flag is true; after
calling mm.wrapHandler and retrieving metrics via mm.getMetrics() (and before
using metricID to fetch the capture with mm.getCaptureByID), add an assertion
that metrics[0].HasCapture == true to ensure the metric indicates a stored
capture (use the same metrics slice produced by getMetrics and the metric at
index 0).
🧹 Nitpick comments (3)
proxy/metrics_monitor.go (1)
268-283:RespBodystores decompressed content, not raw bytes on the wire.Line 282 uses
body, which has already been throughdecompressBody(line 236). This is likely fine for debugging (readable JSON), but worth noting in the struct or API docs so consumers don't expect wire-format content.proxy/metrics_monitor_test.go (2)
944-986: Rename toTestMetricsMonitor_RedactHeadersto match established convention.Every other test function in this file follows the
TestMetricsMonitor_*pattern.TestRedactHeadersbreaks consistency. As per coding guidelines, test names should follow theTest<Component>_<test name>convention.Proposed fix
-func TestRedactHeaders(t *testing.T) { +func TestMetricsMonitor_RedactHeaders(t *testing.T) {Based on learnings: "Follow test naming conventions like
TestProxyManager_<test name>,TestProcessGroup_<test name>, etc."
1029-1057: Missing assertion thatHasCaptureisfalsewhen captures are disabled.Symmetric to the enabled case, this would confirm the flag stays
falsewhen captures are off.Proposed fix
// Metrics should still be recorded metrics := mm.getMetrics() assert.Equal(t, 1, len(metrics)) + assert.False(t, metrics[0].HasCapture) // But no capture
| func TestMetricsMonitor_WrapHandler_Capture(t *testing.T) { | ||
| t.Run("captures request and response when enabled", func(t *testing.T) { | ||
| mm := newMetricsMonitor(testLogger, 10, true) | ||
|
|
||
| requestBody := `{"model": "test", "prompt": "hello"}` | ||
| responseBody := `{"usage": {"prompt_tokens": 100, "completion_tokens": 50}}` | ||
|
|
||
| nextHandler := func(modelID string, w http.ResponseWriter, r *http.Request) error { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Header().Set("X-Custom", "header-value") | ||
| w.WriteHeader(http.StatusOK) | ||
| w.Write([]byte(responseBody)) | ||
| return nil | ||
| } | ||
|
|
||
| req := httptest.NewRequest("POST", "/test", bytes.NewBufferString(requestBody)) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Authorization", "Bearer secret") | ||
| rec := httptest.NewRecorder() | ||
| ginCtx, _ := gin.CreateTestContext(rec) | ||
|
|
||
| err := mm.wrapHandler("test-model", ginCtx.Writer, req, nextHandler) | ||
| assert.NoError(t, err) | ||
|
|
||
| // Check metric was recorded | ||
| metrics := mm.getMetrics() | ||
| assert.Equal(t, 1, len(metrics)) | ||
| metricID := metrics[0].ID | ||
|
|
||
| // Check capture was stored with same ID | ||
| capture := mm.getCaptureByID(metricID) | ||
| assert.NotNil(t, capture) | ||
| assert.Equal(t, metricID, capture.ID) | ||
| assert.Equal(t, []byte(requestBody), capture.ReqBody) | ||
| assert.Equal(t, []byte(responseBody), capture.RespBody) | ||
| assert.Equal(t, "application/json", capture.ReqHeaders["Content-Type"]) | ||
| assert.Equal(t, "[REDACTED]", capture.ReqHeaders["Authorization"]) | ||
| assert.Equal(t, "application/json", capture.RespHeaders["Content-Type"]) | ||
| assert.Equal(t, "header-value", capture.RespHeaders["X-Custom"]) | ||
| }) |
There was a problem hiding this comment.
Missing assertion on HasCapture flag.
This test verifies capture storage but doesn't assert that metrics[0].HasCapture == true, which is the field that tells the UI whether a capture link should be shown. Consider adding the assertion to prevent regressions on that flag.
Proposed fix
// Check metric was recorded
metrics := mm.getMetrics()
assert.Equal(t, 1, len(metrics))
+ assert.True(t, metrics[0].HasCapture)
metricID := metrics[0].ID🤖 Prompt for AI Agents
In `@proxy/metrics_monitor_test.go` around lines 988 - 1027, The test
TestMetricsMonitor_WrapHandler_Capture is missing an assertion that the recorded
metric's HasCapture flag is true; after calling mm.wrapHandler and retrieving
metrics via mm.getMetrics() (and before using metricID to fetch the capture with
mm.getCaptureByID), add an assertion that metrics[0].HasCapture == true to
ensure the metric indicates a stored capture (use the same metrics slice
produced by getMetrics and the metric at index 0).
Use TextDecoder instead of raw atob() to properly handle multi-byte UTF-8 characters like CJK text and emoji in request/response bodies.
Remove Content-Encoding header from captured response headers since the stored body is already decompressed. Change redactHeaders to not return a value since it mutates in-place.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/metrics_monitor.go (1)
214-217:⚠️ Potential issue | 🟠 MajorNon-200 responses are silently skipped — defeats the stated debugging goal.
The linked issue
#464specifically describes needing to debug a 401 response from an upstream peer. However,wrapHandlerreturns early on line 214–217 whenrecorder.Status() != http.StatusOK, meaning no capture is ever stored for 4xx/5xx responses. This makes the capture feature unable to help with the exact scenario that motivated it.Consider storing the capture (request + response headers/body) even when the status is non-200, since those are often the most valuable for debugging.
🤖 Fix all issues with AI agents
In `@proxy/metrics_monitor.go`:
- Around line 182-189: The code uses io.ReadAll(request.Body) without limits
(reqBody), risking OOM; switch to reading via
io.ReadAll(io.LimitReader(request.Body, int64(mp.maxCaptureSize)+1)) to cap the
read, then if len(reqBody) > mp.maxCaptureSize treat the capture as too large
(drop capture or flag it) but still reconstruct the full request.Body for
downstream by chaining the already-read bytes back with the remaining stream
(e.g., io.NopCloser(io.MultiReader(bytes.NewReader(reqBody), request.Body))).
Update the block around reqBody, request.Body and use mp.maxCaptureSize,
io.LimitReader, io.NopCloser and bytes.NewBuffer (or bytes.NewReader)
accordingly.
🧹 Nitpick comments (2)
proxy/metrics_monitor.go (1)
126-134: FIFO eviction re-slices from the front repeatedly — minor memory leak.
mp.captureOrder = mp.captureOrder[1:]on line 129 advances the slice start but the underlying array retains references to evicted IDs and never shrinks. Over a long-running process with many captures this is a slow leak of the backing array. It's bounded by the number of captures stored over the lifetime, so for typical usage it's negligible, but worth being aware of.One lightweight fix: periodically compact, or use a ring-buffer /
container/listinstead.proxy/metrics_monitor_test.go (1)
944-986:TestRedactHeaders— consider aligning name with the test naming convention.The project convention is
Test<Type>_<name>. SinceredactHeadersis a standalone helper closely tied tometricsMonitor, a name likeTestMetricsMonitor_RedactHeaderswould be more consistent with the rest of this file. As per coding guidelines, follow test naming conventions likeTestProxyManager_<test name>,TestProcessGroup_<test name>, etc.
| if request.Body != nil { | ||
| var err error | ||
| reqBody, err = io.ReadAll(request.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read request body for capture: %w", err) | ||
| } | ||
| request.Body.Close() | ||
| request.Body = io.NopCloser(bytes.NewBuffer(reqBody)) |
There was a problem hiding this comment.
Unbounded io.ReadAll on the request body when captures are enabled.
There's no size limit on io.ReadAll(request.Body) at line 184. A large upload or a malformed client could cause excessive memory allocation before the capture-size check at line 286. Consider capping the read with io.LimitReader:
Suggested mitigation
- reqBody, err = io.ReadAll(request.Body)
+ reqBody, err = io.ReadAll(io.LimitReader(request.Body, int64(mp.maxCaptureSize)+1))After reading, you can check len(reqBody) > mp.maxCaptureSize and decide whether to discard the capture early (and still reconstruct the full body for the downstream handler by chaining the remaining unread bytes back).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if request.Body != nil { | |
| var err error | |
| reqBody, err = io.ReadAll(request.Body) | |
| if err != nil { | |
| return fmt.Errorf("failed to read request body for capture: %w", err) | |
| } | |
| request.Body.Close() | |
| request.Body = io.NopCloser(bytes.NewBuffer(reqBody)) | |
| if request.Body != nil { | |
| var err error | |
| reqBody, err = io.ReadAll(io.LimitReader(request.Body, int64(mp.maxCaptureSize)+1)) | |
| if err != nil { | |
| return fmt.Errorf("failed to read request body for capture: %w", err) | |
| } | |
| request.Body.Close() | |
| request.Body = io.NopCloser(bytes.NewBuffer(reqBody)) |
🤖 Prompt for AI Agents
In `@proxy/metrics_monitor.go` around lines 182 - 189, The code uses
io.ReadAll(request.Body) without limits (reqBody), risking OOM; switch to
reading via io.ReadAll(io.LimitReader(request.Body, int64(mp.maxCaptureSize)+1))
to cap the read, then if len(reqBody) > mp.maxCaptureSize treat the capture as
too large (drop capture or flag it) but still reconstruct the full request.Body
for downstream by chaining the already-read bytes back with the remaining stream
(e.g., io.NopCloser(io.MultiReader(bytes.NewReader(reqBody), request.Body))).
Update the block around reqBody, request.Body and use mp.maxCaptureSize,
io.LimitReader, io.NopCloser and bytes.NewBuffer (or bytes.NewReader)
accordingly.
Change config from a boolean toggle to a size-based setting in MB. CaptureBuffer: 0 disables captures, >0 sets the buffer size. - default captureBuffer is 5 (MB) - remove DefaultMaxCaptureSize constant - derive enableCaptures and maxCaptureSize from captureBuffer - update config schema and example config
|
@mostlygeek Thanks for the new api. I could use an |
2 participants
Add saving request and response headers and bodies that go through llama-swap in memory.
QOL features like json formatting and recombining SSE chat streams
Fixes #464
Ref #503
Screenshot:
Summary by CodeRabbit
New Features
Documentation