-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
feature: Update GuzzleHttpClientAspect to support transfer methods and enhance request logging #835
Conversation
""" Walkthrough此次提交对两个 GuzzleHttpClientAspect 类进行了重构。主要更新包括在拦截请求时由原来的 request/requestAsync 方法切换为 transfer 方法,同时引入 on_stats 回调来捕获传输统计信息。针对 Sentry 的面包屑记录和追踪信息注入,分别在处理逻辑中进行了优化和简化。部分冗余的计时代码和异常处理代码已被移除,并合并了对 no_sentry_aspect/no_sentry_tracing 选项的判断。 Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as 调用方
participant SentryAspect as Sentry拦截器
participant JoinPoint as ProceedingJoinPoint
participant GuzzleClient as HTTP 客户端
participant Sentry as Sentry系统
Caller->>SentryAspect: 调用 process(...)
SentryAspect->>JoinPoint: 执行 transfer 方法
JoinPoint-->>SentryAspect: 返回请求结果
SentryAspect->>GuzzleClient: 设置 on_stats 回调
GuzzleClient->>SentryAspect: 触发 on_stats (传递 TransferStats)
SentryAspect->>Sentry: 记录面包屑
sequenceDiagram
participant Caller as 调用方
participant TracingAspect as 跟踪拦截器
participant JoinPoint as ProceedingJoinPoint
participant GuzzleClient as HTTP 客户端
participant SentryTracing as Sentry 跟踪系统
Caller->>TracingAspect: 调用 process(...)
TracingAspect->>JoinPoint: 执行 transfer 方法
JoinPoint-->>TracingAspect: 返回请求结果
TracingAspect->>GuzzleClient: 设置 on_stats 回调
GuzzleClient->>TracingAspect: 触发 on_stats (传递 TransferStats)
TracingAspect->>SentryTracing: 注入 tracing headers,并更新 span 状态
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.0.3)/bin/bash: line 1: /vendor/bin/phpstan: No such file or directory Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
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: 0
🧹 Nitpick comments (2)
src/sentry/src/Aspect/GuzzleHttpClientAspect.php (1)
57-82
: 新的 on_stats 回调逻辑实现并记录面包屑。• 行 58-59: 通过 stats 获取 request 和 response 对象,逻辑正确。
• 行 61-69: 收集 URI、请求头、响应头及耗时等信息,适用于 Sentry 调试与追踪。需注意潜在敏感信息在 headers 中的泄露风险。
• 行 71-77: 调用 Integration::addBreadcrumb 记录面包屑等级为信息级,类型为默认,分类“guzzle”,符合预期用法。
• 行 79-80: 保持对原有用户 on_stats 回调的调用,避免破坏原有逻辑。若对响应头或请求参数进行更加严格的隐私保护,可在此处做筛选或脱敏处理。
可考虑对敏感头部或大体量响应内容进行脱敏或简化,以免干扰日志并避免潜在的隐私问题。
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
87-128
: on_stats 回调记录各类追踪信息,并在响应后结束 span。• lines 90-91: 获取请求和响应对象,用于后续收集信息。
• lines 95-110: 收集请求方法、响应状态、头部和传输耗时等细节,存储到 span 的 data 中。
• lines 112-114: 可选地记录响应体,这里需要注意潜在的敏感信息或过大的响应体导致存储过多问题。
• lines 117-119: 当状态码为 4xx 或 5xx 时,设置 span 状态为 internalError 并标记 error。
• lines 125-127: 调用用户自定义的 on_stats,避免覆盖用户原有逻辑。
• 最终在此回调结束后,对 span 进行 finish,完整记录一次请求-响应周期。建议在记录完整响应体时进行筛选或脱敏,以避免意外泄露。
可在响应体较大或含敏感字段时截断或屏蔽,以保持日志精简并提升安全性。
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sentry/src/Aspect/GuzzleHttpClientAspect.php
(3 hunks)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.6
🔇 Additional comments (14)
src/sentry/src/Aspect/GuzzleHttpClientAspect.php (7)
17-17
: 引入 TransferStats 用于统计信息回调的处理。此处新增的 use 语句是为了在后续的 on_stats 回调中使用 TransferStats 对象,逻辑合理,无需改动。
29-29
: 替换拦截方法为 transfer。将拦截的 Guzzle 方法从原先的 request/requestAsync 调整为 transfer,可在 on_stats 回调中收集更多网络传输信息,顺应最新的 Guzzle 使用方式。
38-38
: 在禁用情况时跳过记录。新增该注释以说明当 Guzzle 面包屑被禁用时直接返回,代码意图清晰。
43-48
: 获取配置信息的闭包写法。这里通过闭包和 match 搭配,动态判断使用 getConfig 还是 config 属性。实现逻辑巧妙,能兼容不同 Guzzle 版本。但要注意当 Guzzle 8 开始彻底移除 getConfig 时,应及时更新相关使用。
是否已确认 Guzzle 版本兼容?可在后续版本中去除对 getConfig 的依赖。
50-50
: 根据 no_sentry_aspect 配置跳过记录。判断用户配置后即可短路返回,符合需求。若后续需要更细粒度的跳过条件,可进一步扩展。
55-55
: 保存原始 on_stats 回调以便后续调用。此处先提取用户自定义的 on_stats 以实现回调链式调用,这是常见且安全的做法。
84-84
: 将处理委托给原始 ProceedingJoinPoint。on_stats 回调设置完毕后,再调用原始流程,让请求真正执行。该顺序正确,避免了回调丢失或无效。
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (7)
17-17
: 新增 TransferStats 引入。与转为拦截 transfer 方法的逻辑保持一致,允许在 on_stats 中使用传输统计信息。
35-35
: 拦截 transfer 方法以实现追踪。使用 transfer 代替 request/requestAsync,可更方便地捕获网络传输耗时信息并进行追踪。
54-55
: 提取 URI 和 method。将外部传入的 URI、method 参数通过 $arguments 获取并做默认值处理,有效避免空值导致的异常。
63-63
: 判断 no_sentry_tracing 配置以跳过追踪。通过相同的配置检测机制,如检测到 no_sentry_tracing,则直接执行原逻辑,不进行追踪。这在多场景下可满足用户自定义需求。
71-71
: 注入上下文以携带 Trace 信息。注释说明为什么要注入 traceparent、baggage 等头信息,以便分布式追踪。实现方式合理。
81-85
: 创建 span 并检查其有效性。• line 81: startSpan 创建新的 HTTP 客户端 span,名称包含 method 和 URI,方便排查请求。
• line 83-85: 若无有效 span(可能是分布式追踪未初始化),则直接继续原流程。
130-130
: 执行原始请求流程。在设置好追踪信息和回调后,调用 process 继续执行请求,流程衔接顺畅。
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: 0
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (2)
65-87
: 增强了追踪控制和上下文注入追踪功能得到了显著改进:
- 添加了
no_sentry_tracing
选项检查,提供了更细粒度的控制- 优化了追踪上下文的注入方式
- 改进了 span 的创建和验证逻辑
建议考虑添加以下改进:
- if (! $span) { + if ($span === null) { return $proceedingJoinPoint->process(); }
89-145
: 显著增强了统计收集和错误处理能力新的实现提供了更完整的监控和错误处理:
- 通过
TransferStats
收集了全面的请求统计信息- 增加了详细的错误处理逻辑,包括异常和HTTP错误状态
- 正确处理了原有的
on_stats
回调建议考虑以下优化:
- 考虑将统计数据收集逻辑抽取到单独的方法中,以提高代码可维护性
- 可以添加配置选项来控制是否收集某些统计信息,优化性能
示例重构:
+ private function collectStats(TransferStats $stats, array $arguments, array $guzzleConfig): array + { + $request = $stats->getRequest(); + $response = $stats->getResponse(); + $uri = $request->getUri(); + return [ + 'http.query' => $uri->getQuery(), + // ... other stats + ]; + } - $data = [ - 'http.query' => $uri->getQuery(), - // ... other stats - ]; + $data = $this->collectStats($stats, $arguments, $guzzleConfig);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.6
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.0
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.6
🔇 Additional comments (3)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (3)
17-23
: 导入更改符合新的实现需求更改后的导入声明更好地反映了类的职责,通过使用
TransferStats
和RequestInterface
来增强请求跟踪能力。
36-38
: 优化了方法拦截策略将拦截方法从
request/requestAsync
改为transfer
是一个很好的改进,因为:
transfer
方法是所有请求的底层实现- 可以同时覆盖同步和异步请求
- 减少了重复代码
56-63
: 改进了类型安全性和配置获取代码质量有了显著提升:
- 添加了
RequestInterface
类型提示,增强了类型安全性- 使用闭包优化了配置获取逻辑
- 通过
getInstance()
改进了实例访问方式
…d enhance request logging (#835) * feat: 更新 GuzzleHttpClientAspect 以支持 transfer 方法并增强请求记录功能 * feat: 更新 GuzzleHttpClientAspect 以支持 transfer 方法并增强请求追踪功能 * feat: 添加或覆盖 on_stats 选项以记录请求持续时间 * feat: 增强 GuzzleHttpClientAspect 错误处理,添加异常信息和堆栈跟踪标签 * feat: 更新 GuzzleHttpClientAspect 以使用 RequestInterface 获取请求方法和 URI --------- Co-authored-by: Deeka Wong <[email protected]>
Summary by CodeRabbit
新功能
重构