增加调试日志并记录请求体 - #2731
Closed
tanranv5 wants to merge 3 commits into
Closed
Conversation
Contributor
WalkthroughThis PR adds extensive observability and validation improvements to the relay system. It introduces comprehensive debug logging throughout relay and task retry paths, adds header value masking for security in logs, refactors header override processing with validation, and implements model mapping chain tracking with detailed logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/channel/api_request.go (1)
41-106: Mask header overrides by header name, not just API key.Current masking only replaces occurrences of
info.ApiKey. If overrides includeAuthorization,X-API-Key,Cookie, etc., those values will be logged in cleartext. Consider redacting known sensitive headers regardless of value.✅ Suggested fix (name-based redaction)
+var sensitiveHeaderKeys = map[string]struct{}{ + "authorization": {}, + "proxy-authorization": {}, + "x-api-key": {}, + "api-key": {}, + "cookie": {}, + "set-cookie": {}, +} + +func isSensitiveHeaderKey(key string) bool { + _, ok := sensitiveHeaderKeys[strings.ToLower(key)] + return ok +} + func maskHeaderOverrideForLog(headerOverride map[string]interface{}, apiKey string) map[string]interface{} { if len(headerOverride) == 0 { return nil } masked := make(map[string]interface{}, len(headerOverride)) for key, value := range headerOverride { + if isSensitiveHeaderKey(key) { + masked[key] = "***" + continue + } if str, ok := value.(string); ok { masked[key] = maskHeaderOverrideValue(str, apiKey) } else { masked[key] = value } } return masked } func maskHeaderOverrideForLogString(headerOverride map[string]string, apiKey string) map[string]string { if len(headerOverride) == 0 { return nil } masked := make(map[string]string, len(headerOverride)) for key, value := range headerOverride { + if isSensitiveHeaderKey(key) { + masked[key] = "***" + continue + } masked[key] = maskHeaderOverrideValue(value, apiKey) } return masked }
🤖 Fix all issues with AI agents
In `@controller/relay.go`:
- Around line 361-366: The current error logging stores the full raw request
body from common.GetRequestBody into the telemetry map (other["request_body"])
which may leak sensitive data and bloat logs; update the relay handler to avoid
persisting raw bodies by inspecting the request Content-Type and, for text
types, applying masking or truncation (e.g., limit to N bytes and replace
sensitive fields) before assigning to other["request_body"], and for non-text or
large payloads set a safe placeholder (e.g., "<omitted: binary/large payload>")
or set a flag like other["request_body_omitted"]=true; still record body read
errors to other["request_body_error"] as before.
In `@relay/helper/model_mapped.go`:
- Around line 64-71: The code computes a local upstreamModel but still calls
request.SetModelName(info.UpstreamModelName), which can pass an empty string and
clear the request; change the mutation to use the normalized upstreamModel (or
only call SetModelName when upstreamModel != "") so the request and the logger
remain consistent—update the block around upstreamModel, logger.LogDebug, and
request.SetModelName to use the upstreamModel variable (or guard the
SetModelName call) instead of info.UpstreamModelName.
Comment on lines
+361
to
+366
| requestBody, bodyErr := common.GetRequestBody(c) | ||
| if bodyErr == nil { | ||
| other["request_body"] = string(requestBody) | ||
| } else { | ||
| other["request_body_error"] = bodyErr.Error() | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid persisting raw request bodies to error logs.
Storing full request bodies in error logs risks leaking sensitive data and can bloat storage (especially for large or binary payloads). Consider truncation and/or masking, and optionally gating by content type.
✅ Suggested fix (truncate + flag)
requestBody, bodyErr := common.GetRequestBody(c)
if bodyErr == nil {
- other["request_body"] = string(requestBody)
+ const maxLogBytes = 2048
+ bodyForLog := requestBody
+ if len(bodyForLog) > maxLogBytes {
+ bodyForLog = bodyForLog[:maxLogBytes]
+ other["request_body_truncated"] = true
+ }
+ other["request_body"] = string(bodyForLog)
} else {
other["request_body_error"] = bodyErr.Error()
}🤖 Prompt for AI Agents
In `@controller/relay.go` around lines 361 - 366, The current error logging stores
the full raw request body from common.GetRequestBody into the telemetry map
(other["request_body"]) which may leak sensitive data and bloat logs; update the
relay handler to avoid persisting raw bodies by inspecting the request
Content-Type and, for text types, applying masking or truncation (e.g., limit to
N bytes and replace sensitive fields) before assigning to other["request_body"],
and for non-text or large payloads set a safe placeholder (e.g., "<omitted:
binary/large payload>") or set a flag like other["request_body_omitted"]=true;
still record body read errors to other["request_body_error"] as before.
Comment on lines
+64
to
71
| upstreamModel := info.UpstreamModelName | ||
| if upstreamModel == "" { | ||
| upstreamModel = info.OriginModelName | ||
| } | ||
| logger.LogDebug(c, "模型映射结果: 原模型=%q, 映射链路=%s, 上游模型=%q, 已映射=%t", info.OriginModelName, strings.Join(chain, "->"), upstreamModel, info.IsModelMapped) | ||
| if request != nil { | ||
| request.SetModelName(info.UpstreamModelName) | ||
| } |
Contributor
There was a problem hiding this comment.
Default upstream model before mutating the request.
upstreamModel is defaulted locally, but request.SetModelName(info.UpstreamModelName) can still receive an empty string when mapping is skipped, causing the request model to be cleared and logs to disagree with runtime behavior. Consider normalizing info.UpstreamModelName before use.
✅ Suggested fix
- upstreamModel := info.UpstreamModelName
- if upstreamModel == "" {
- upstreamModel = info.OriginModelName
- }
+ if info.UpstreamModelName == "" {
+ info.UpstreamModelName = info.OriginModelName
+ }
+ upstreamModel := info.UpstreamModelName
logger.LogDebug(c, "模型映射结果: 原模型=%q, 映射链路=%s, 上游模型=%q, 已映射=%t", info.OriginModelName, strings.Join(chain, "->"), upstreamModel, info.IsModelMapped)
if request != nil {
request.SetModelName(info.UpstreamModelName)
}📝 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.
Suggested change
| upstreamModel := info.UpstreamModelName | |
| if upstreamModel == "" { | |
| upstreamModel = info.OriginModelName | |
| } | |
| logger.LogDebug(c, "模型映射结果: 原模型=%q, 映射链路=%s, 上游模型=%q, 已映射=%t", info.OriginModelName, strings.Join(chain, "->"), upstreamModel, info.IsModelMapped) | |
| if request != nil { | |
| request.SetModelName(info.UpstreamModelName) | |
| } | |
| if info.UpstreamModelName == "" { | |
| info.UpstreamModelName = info.OriginModelName | |
| } | |
| upstreamModel := info.UpstreamModelName | |
| logger.LogDebug(c, "模型映射结果: 原模型=%q, 映射链路=%s, 上游模型=%q, 已映射=%t", info.OriginModelName, strings.Join(chain, "->"), upstreamModel, info.IsModelMapped) | |
| if request != nil { | |
| request.SetModelName(info.UpstreamModelName) | |
| } |
🤖 Prompt for AI Agents
In `@relay/helper/model_mapped.go` around lines 64 - 71, The code computes a local
upstreamModel but still calls request.SetModelName(info.UpstreamModelName),
which can pass an empty string and clear the request; change the mutation to use
the normalized upstreamModel (or only call SetModelName when upstreamModel !=
"") so the request and the logger remain consistent—update the block around
upstreamModel, logger.LogDebug, and request.SetModelName to use the
upstreamModel variable (or guard the SetModelName call) instead of
info.UpstreamModelName.
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.