-
-
Notifications
You must be signed in to change notification settings - Fork 27
fix(tracing): optimize HTTP response body content reading in GuzzleHttpClientAspect #900
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
fix(tracing): optimize HTTP response body content reading in GuzzleHttpClientAspect #900
Conversation
…tpClientAspect - Add content-type detection to only read textual responses - Limit binary/non-textual content to '[binary omitted]' to prevent memory issues - Set 8KB limit for response body content reading to avoid performance impact - Use Guzzle's Utils::copyToString for safer stream handling
|
Warning Rate limit exceeded@huangdijia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 27 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
Walkthrough在 GuzzleHttpClientAspect 中调整响应体抓取逻辑:依据 Content-Type 仅截取最多 8KB 的文本类响应体(text/*、application/json|xml|x-www-form-urlencoded),否则记录 "[binary omitted]";保留原有是否捕获 body 的开关与可回绕流的 rewind 行为。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Guzzle as Guzzle Client
participant Aspect as GuzzleHttpClientAspect
participant Resp as HTTP Response
App->>Guzzle: send(request)
Guzzle->>Resp: execute
Guzzle-->>App: response
App->>Aspect: onResponse(response)
Aspect->>Resp: getHeader("Content-Type")
alt capture enabled
alt Content-Type 为文本类
Aspect->>Resp: copyToString(max 8KB)
Aspect->>Aspect: store body snippet
opt 可回绕
Aspect->>Resp: rewind()
end
else 非文本/未知
Aspect->>Aspect: store "[binary omitted]"
end
else 未启用捕获
Aspect->>Aspect: skip body capture
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ 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
|
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 (2)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (2)
129-129: 放宽文本类型判定:支持 +json/+xml、NDJSON、GraphQL、SVG 等很多文本响应使用结构化语法后缀(如
application/problem+json、application/vnd.api+json、image/svg+xml),或常见的application/x-ndjson、application/graphql。当前正则会误判为二进制。建议扩展匹配:- $isTextual = \preg_match('/^(text\/|application\/(json|xml|x-www-form-urlencoded))/i', $contentType) === 1; + $isTextual = \preg_match( + '/^(?:text\/|application\/(?:json|xml|x-www-form-urlencoded|[\w.+-]+\+json|[\w.+-]+\+xml|javascript|graphql|x-ndjson)|image\/svg\+xml)/i', + $contentType + ) === 1;
131-131: 将 8KB 魔数提取为类常量/可配置项便于后续统一调整与 A/B 验证。
在类中添加常量(示例放置于
use SpanStarter;之后):class GuzzleHttpClientAspect extends AbstractAspect { use SpanStarter; + private const RESPONSE_BODY_PREVIEW_LIMIT = 8192; // 8KB并在读取处使用该常量(已在上一个 diff 中示例)。
📜 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 (1)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(1 hunks)
⏰ 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.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 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 6.0.2
- GitHub Check: Test on PHP 8.1 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: git-mirror
🔇 Additional comments (1)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
131-133: 请确认项目中未覆盖 Guzzle 7.x 默认的 decode_content 选项
Guzzle 7.x 默认 decode_content=true,会自动解压 gzip/deflate 响应,仅在显式设置为 false 时才会保留压缩字节流。
如无覆盖默认值,可关闭此检查;若项目中有禁用自动解码的场景,建议在无解码时统一使用 "[compressed omitted]" 防止误导。
…eHttpClientAspect
…leHttpClientAspect
…h in GuzzleHttpClientAspect
…eHttpClientAspect
…tpClientAspect (#900) * fix(tracing): optimize HTTP response body content reading in GuzzleHttpClientAspect - Add content-type detection to only read textual responses - Limit binary/non-textual content to '[binary omitted]' to prevent memory issues - Set 8KB limit for response body content reading to avoid performance impact - Use Guzzle's Utils::copyToString for safer stream handling * fix(tracing): improve handling of HTTP response body content in GuzzleHttpClientAspect * fix(tracing): standardize request and response data keys in GuzzleHttpClientAspect * fix(tracing): add HTTP response body size to breadcrumb data in GuzzleHttpClientAspect * fix(tracing): add comment to clarify on_stats option override in GuzzleHttpClientAspect * fix(tracing): add additional response header fields for content length in GuzzleHttpClientAspect * fix(tracing): improve handling of HTTP response body content in GuzzleHttpClientAspect * fix(tracing): preserve position of response body stream in GuzzleHttpClientAspect --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
Test plan
Summary by CodeRabbit
新功能
Bug Fixes
Refactor