Add request, response data to recorded metrics #214
Conversation
WalkthroughA new configuration option, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Activity Page)
participant API Server
participant Metrics Middleware
participant Metrics Monitor
User->>UI (Activity Page): Requests metrics data
UI (Activity Page)->>API Server: Fetch metrics (with logHTTPRequests enabled)
API Server->>Metrics Middleware: Handle HTTP request
Metrics Middleware->>Metrics Monitor: Record metrics, pass request & response bodies if logging enabled
Metrics Monitor-->>Metrics Middleware: Store metrics with bodies
Metrics Middleware-->>API Server: Complete request
API Server-->>UI (Activity Page): Return metrics data (with request/response bodies)
UI (Activity Page)-->>User: Display expandable rows with request/response bodies (optionally beautified)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–25 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (3)
ui/src/pages/Activity.tsx (3)
28-37: Consider explicit undefined handling.The function handles edge cases well with proper error handling. However, the type guard could be more explicit about handling undefined.
- const beautifyJson = (jsonString?: string): string => { - if (typeof jsonString !== "string") - return ""; + const beautifyJson = (jsonString?: string): string => { + if (!jsonString || typeof jsonString !== "string") { + return ""; + }
62-105: Consider dynamic colSpan calculation.The expandable row implementation is well-structured with proper conditional rendering and good UX considerations (scrollable content areas, text wrapping). However, the hardcoded
colSpan={7}could become outdated if table columns change.- <td colSpan={7} className="px-6 py-4 bg-gray-50 border-t"> + <td colSpan={hasRequestData ? 7 : 6} className="px-6 py-4 bg-gray-50 border-t">Or better yet, calculate the column count dynamically based on the table headers.
138-138: Consider conditional column header.The "Request data" header is always visible, but the toggle button only appears when
hasRequestDatais true, potentially causing inconsistent table layouts when some rows have request data and others don't.Consider either:
- Always showing the column (with empty cells when no data)
- Making the header conditional based on whether any metrics have request data
- Using a different UI pattern that doesn't affect table structure
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config.example.yaml(1 hunks)proxy/config.go(2 hunks)proxy/metrics_middleware.go(6 hunks)proxy/metrics_monitor.go(3 hunks)ui/src/contexts/APIProvider.tsx(1 hunks)ui/src/pages/Activity.tsx(4 hunks)
🔇 Additional comments (13)
proxy/config.go (1)
144-144: LGTM! Configuration field addition follows established patterns.The new
LogHTTPRequestsboolean field is properly integrated into theConfigstruct with appropriate YAML tag and sensible default value offalsefor security reasons.Also applies to: 197-197
ui/src/contexts/APIProvider.tsx (1)
33-34: LGTM! Interface extension properly supports the new feature.The optional
request_bodyandresponse_bodyfields correctly extend theMetricsinterface to support the new HTTP logging functionality while maintaining backward compatibility.config.example.yaml (1)
18-21: LGTM! Clear documentation of the new configuration option.The documentation properly explains the
logHTTPRequestsoption with accurate default value and clear description of its functionality.proxy/metrics_monitor.go (2)
20-21: LGTM! Proper struct extension with appropriate JSON tags.The new
RequestBodyandResponseBodyfields correctly extend theTokenMetricsstruct withomitemptytags to exclude empty values from JSON serialization.
39-39: LGTM! Proper integration of the configuration flag.The
logHTTPRequestsfield is correctly added to theMetricsMonitorstruct and properly initialized from the configuration in the constructor. The comment update accurately reflects the new functionality.Also applies to: 50-50, 57-57
proxy/metrics_middleware.go (3)
77-111: LGTM! Clean refactoring with proper conditional logging.The refactored
parseAndRecordMetricsmethod correctly:
- Accepts raw response bytes and handles JSON parsing internally
- Conditionally attaches request/response bodies only when logging is enabled
- Maintains all existing functionality while adding the new feature
The implementation is clean and follows good practices.
65-65: LGTM! Appropriate field addition to support request body storage.The new
requestBodyfield properly extends theMetricsRecorderstruct to support the HTTP request logging feature.
140-141: LGTM! Consistent updates to response processing methods.Both streaming and non-streaming response processing methods are correctly updated to pass raw bytes to the refactored
parseAndRecordMetricsmethod.Also applies to: 153-153
ui/src/pages/Activity.tsx (5)
1-1: LGTM!Fragment import correctly added for the new expandable row functionality.
19-20: LGTM!State variables are well-typed and appropriately initialized. Using
Set<string>for tracking expanded metrics is efficient for membership operations.
50-60: LGTM!Proper immutable state update pattern with functional setState. The toggle logic correctly maintains the Set of expanded metric IDs.
111-121: LGTM!Properly implemented controlled checkbox with good accessibility (id/htmlFor association) and clear labeling.
142-142: LGTM!Good refactoring that improves code organization by extracting complex row rendering logic into a separate function.
| metricsRecorder := &MetricsRecorder{ | ||
| metricsMonitor: pm.metricsMonitor, | ||
| realModelName: realModelName, | ||
| isStreaming: gjson.GetBytes(bodyBytes, "stream").Bool(), | ||
| startTime: time.Now(), | ||
| } | ||
| if pm.metricsMonitor.logHTTPRequests { | ||
| metricsRecorder.requestBody = bodyBytes | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider memory and security implications of request body storage.
The conditional capture of request bodies is functionally correct, but consider the following:
- Memory usage: Large request bodies will be held in memory until metrics are processed
- PII exposure: Request bodies may contain sensitive user data that shouldn't be logged
- Performance: String conversion and storage adds overhead
Consider adding safeguards like size limits or content filtering.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any size limits or content filtering mechanisms in the codebase
# Search for any existing request size limits or filtering
rg -A 5 -B 5 "size.*limit|limit.*size|max.*body|body.*max" --type go
# Look for any existing PII filtering or sanitization
rg -A 5 -B 5 "sanitiz|filter.*pii|pii.*filter|redact" --type goLength of output: 5717
Consider adding size limits and PII sanitization when logging request bodies
I searched the codebase and didn’t find any existing limits or redaction for stored HTTP payloads, so the current implementation will retain arbitrarily large or sensitive data in memory. To mitigate memory and security risks, please:
• In proxy/metrics_middleware.go (around lines 39–47), wrap the assignment of metricsRecorder.requestBody with a configurable maximum-size check (e.g., drop or truncate if len(bodyBytes) exceeds a threshold).
• Introduce an allowlist/denylist or redaction step before storing the payload to filter out known PII patterns (e.g., email addresses, credit-card numbers) or sensitive JSON fields.
• Surface these safeguards via your metrics configuration (e.g., pm.metricsMonitor.maxBodySize and pm.metricsMonitor.redactFields) so they can be tuned per deployment.
🤖 Prompt for AI Agents
In proxy/metrics_middleware.go around lines 39 to 47, the current code assigns
the entire request body to metricsRecorder.requestBody without size limits or
PII sanitization. To fix this, add a check against a configurable maxBodySize
from pm.metricsMonitor and truncate or drop the body if it exceeds this size.
Implement a redaction step using a configurable redactFields list to filter or
mask sensitive fields or patterns like emails before storing the payload. Ensure
these configurations are exposed in pm.metricsMonitor for deployment tuning.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proxy/proxymanager_test.go (1)
777-815: Good test coverage for the new logging feature, but consider adding negative test case.The test correctly verifies that request and response bodies are recorded when
LogHTTPRequestsis enabled. However, consider adding a companion test to verify that bodies are NOT recorded when this flag is disabled, ensuring the feature works as expected in both scenarios.Consider adding this negative test case:
+func TestProxyManager_RequestResponseBodyIsNotRecorded(t *testing.T) { + // Create config with logHTTPRequests disabled (default false) + config := AddDefaultGroupToConfig(Config{ + HealthCheckTimeout: 15, + Models: map[string]ModelConfig{ + "model1": getTestSimpleResponderConfig("model1"), + }, + LogLevel: "error", + LogHTTPRequests: false, // explicitly disabled + MetricsMaxInMemory: 100, + }) + + proxy := New(config) + defer proxy.StopProcesses(StopWaitForInflightRequest) + + // Make a request + reqBody := `{"model":"model1", "prompt": "test prompt"}` + req := httptest.NewRequest("POST", "/v1/chat/completions", bytes.NewBufferString(reqBody)) + w := httptest.NewRecorder() + + proxy.ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + + // Check that metrics were recorded but without bodies + metrics := proxy.metricsMonitor.GetMetrics() + if !assert.NotEmpty(t, metrics, "metrics should be recorded") { + return + } + + // Verify the last metric has empty request and response bodies + lastMetric := metrics[len(metrics)-1] + assert.Empty(t, lastMetric.RequestBody, "request body should be empty when logHTTPRequests is false") + assert.Empty(t, lastMetric.ResponseBody, "response body should be empty when logHTTPRequests is false") +}
|
I'm not so sure about maintaining this feature in llama-swap. It seems to be pretty niche to be used as a development/debugging feature. I also have concerns about the memory usage. Some prompts when doing agentic coding would getup to the 40K tokens (~120K bytes) on input. What use case is this feature for? |
It is for debugging. For some applications, I think it's useful to inspect the prompt being delivered if the application doesn't allow you to, or if doing so is too clunky. For something like the new Qwen CLI which doesn't allow you to easily inspect its prompt, this feature would be a nice interface for debugging without having to spin a new proxy server to intercept the requests. |
|
@g2mt Could you please include another column for logging the source IP Address, It would be useful to get the Unique number of users :) |
Debugging for me, I agree with your concern regarding memory usage so also looking for an appropriate log inspection tool.. |
Adds request, response data to the recorded metrics, which can be viewed in the Activity page. The data is only recorded if logHTTPRequests is set to true in the config.
Qwen 3 was used in this PR.
Summary by CodeRabbit
New Features
Improvements
Tests