-
-
Notifications
You must be signed in to change notification settings - Fork 27
fix(tracing): standardize HTTP response field names in GuzzleHttpClientAspect #899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ntAspect - Update response field names to follow HTTP semantic conventions - Change response.status to http.response.status_code - Change response.reason to http.response.reason - Change response.headers to http.response.headers - Change response.body.size to http.response.body.size - Change response.body to http.response.body.contents - Update configuration key from response.body to http.response.body.contents - Add proper HTTP status-based span status setting - Improve error handling with standardized field names
Walkthrough该变更统一了 HTTP 响应相关的追踪标签命名(由 response.* 迁移到 http.response.*),在 Guzzle 拦截器中补充了基于状态码的 span 状态设置,并微调异常与响应体记录逻辑;同时在发布配置中同步更新了对应的 extra_tags 键名。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant A as Caller
participant H as GuzzleHttpClientAspect
participant G as Guzzle Client
participant S as Sentry Span
A->>H: 发起 HTTP 请求
H->>S: 创建/获取 Span
H->>G: 发送请求(可能覆写请求头)
G-->>H: 返回 Response 或抛出异常
alt 成功
H->>S: setStatus(createFromHttpStatusCode)
H->>S: setTags(http.response.status_code, reason, headers, body.size)
opt 开启 http.response.body.contents
H->>S: setTag(http.response.body.contents)
end
H-->>A: 返回 Response
else 异常
H->>S: setTag(error=true)
H->>S: setTags(exception.class, exception.code, exception.message)
opt 开启 exception.stack_trace
H->>S: setTag(exception.stack_trace)
end
H-->>A: 抛出异常
end
note over H,S: 所有原 response.* 标签迁移为 http.response.*
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.17)Note: Using configuration file /phpstan.neon.dist. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
…ntAspect (#899) - Update response field names to follow HTTP semantic conventions - Change response.status to http.response.status_code - Change response.reason to http.response.reason - Change response.headers to http.response.headers - Change response.body.size to http.response.body.size - Change response.body to http.response.body.contents - Update configuration key from response.body to http.response.body.contents - Add proper HTTP status-based span status setting - Improve error handling with standardized field names Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (4)
84-85: 注释说明到位;可考虑避免覆盖已有传播头当前用 array_replace 强制覆盖 sentry-trace/traceparent/baggage。如果调用方已注入这些头,覆盖可能引入链路断裂。可改为仅在目标头缺失时写入,或做不区分大小写的合并策略。
118-124: 字段重命名对齐语义规范,建议对 headers 做脱敏/裁剪改为 http.response.* 👍。为降低体积与敏感信息风险,建议对 http.response.headers 进行:
- 脱敏:过滤/哈希 Authorization、Cookie、Set-Cookie 等。
- 裁剪:限制头数量或总字节数,超限用占位符标记。
134-137: 避免在 tags 中记录高基数的 reason已在 data 中写入 http.response.reason;建议在 tags 仅保留 error=true(或同时保留 status_code),移除 reason,避免可能的高基数字段污染指标/聚合。
- $span->setTags([ - 'error' => true, - 'http.response.reason' => $response->getReasonPhrase(), - ]); + $span->setTags([ + 'error' => true, + // 可选:如需保留,改为低基数字段 + // 'http.response.status_code' => (string) $response->getStatusCode(), + ]);
150-153: 将 exception.message 的开关与 stack_trace 解耦目前异常 message 与 stack_trace 共用一个开关。建议为 message 增加独立配置键(如 exception.message),或默认记录受限长度的 message,以便在不开启堆栈时也能定位问题。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/sentry/publish/sentry.php(1 hunks)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
src/sentry/src/Switcher.php (1)
isTracingExtraTagEnable(53-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
🔇 Additional comments (2)
src/sentry/publish/sentry.php (1)
118-118: 补充全局替换脚本并更新发布说明
- 请使用支持 lookaround 的命令(如
rg --pcre2 -n 'response\.body(?!\.size)' -S)或等效正则(例如rg -n 'response\.body' -S \| grep -Ev 'response\.body\.size')重新检查response.body除response.body.size外的残留引用- 新键名
http.response.body.contents已在src/sentry/publish/sentry.php(118 行)和src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(126–127 行)中使用,确认文档及变更日志/升级指南中已说明此破坏性变更并同步示例代码src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
131-131: 基于状态码设置 SpanStatus ✅使用 SpanStatus::createFromHttpStatusCode() 正确、可读性好,与 Sentry 语义一致。
| if ($this->switcher->isTracingExtraTagEnable('http.response.body.contents')) { | ||
| $data['http.response.body.contents'] = $response->getBody()->getContents(); | ||
| $response->getBody()->isSeekable() && $response->getBody()->rewind(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
读取响应体应限流并按类型白名单,避免内存放大与隐私泄露
当前直接 getContents() 会读完整正文(可能为大文件/二进制)。建议:
- 仅对白名单类型(text/*、application/json 等)采集;
- 设定最大读取字节(如 8KB),其余以占位符替代;
- 仍保留 rewind 逻辑。
可在本段直接替换为(使用完全限定名避免新增 use):
- if ($this->switcher->isTracingExtraTagEnable('http.response.body.contents')) {
- $data['http.response.body.contents'] = $response->getBody()->getContents();
- $response->getBody()->isSeekable() && $response->getBody()->rewind();
- }
+ if ($this->switcher->isTracingExtraTagEnable('http.response.body.contents')) {
+ $body = $response->getBody();
+ $contentType = $response->getHeaderLine('Content-Type');
+ $isTextual = \preg_match('/^(text\/|application\/(json|xml|x-www-form-urlencoded))/i', $contentType) === 1;
+ $data['http.response.body.contents'] = $isTextual
+ ? \GuzzleHttp\Psr7\Utils::copyToString($body, 8192) // 8KB 上限
+ : '[binary omitted]';
+ $body->isSeekable() && $body->rewind();
+ }📝 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 ($this->switcher->isTracingExtraTagEnable('http.response.body.contents')) { | |
| $data['http.response.body.contents'] = $response->getBody()->getContents(); | |
| $response->getBody()->isSeekable() && $response->getBody()->rewind(); | |
| } | |
| if ($this->switcher->isTracingExtraTagEnable('http.response.body.contents')) { | |
| $body = $response->getBody(); | |
| $contentType = $response->getHeaderLine('Content-Type'); | |
| $isTextual = \preg_match('/^(text\/|application\/(json|xml|x-www-form-urlencoded))/i', $contentType) === 1; | |
| $data['http.response.body.contents'] = $isTextual | |
| ? \GuzzleHttp\Psr7\Utils::copyToString($body, 8192) // 8KB 上限 | |
| : '[binary omitted]'; | |
| $body->isSeekable() && $body->rewind(); | |
| } |
🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php around lines
126-129, replace the unconditional getContents() with logic that first inspects
the response Content-Type header and only proceeds for a whitelist of safe
text-like types (e.g., text/*, application/json, application/xml,
application/*+json, application/*+xml); for whitelist matches read at most 8192
bytes from the body (use stream read/getContents with a byte limit or
stream->read(8192)), and if the body is larger append a placeholder like
"[TRUNCATED]" (optionally including original size); for non-whitelisted or
binary types do not collect body contents; always preserve the existing rewind
call when the stream is seekable; implement this replacement inline and avoid
adding new use statements (use fully qualified names if needed).
Summary
Changes
response.status→http.response.status_coderesponse.reason→http.response.reasonresponse.headers→http.response.headersresponse.body.size→http.response.body.sizeresponse.body→http.response.body.contentsresponse.bodytohttp.response.body.contentsSpanStatus::createFromHttpStatusCode()Test plan
http.response.body.contentsworks as expectedSummary by CodeRabbit
新功能
重构