Skip to content
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

Merged
merged 5 commits into from
Feb 7, 2025

Conversation

huangdijia
Copy link
Contributor

@huangdijia huangdijia commented Feb 7, 2025

Summary by CodeRabbit

  • 新功能

    • 强化了 HTTP 请求监控和追踪,提供更精确的请求状态、性能和错误反馈,帮助用户更高效地识别问题。
  • 重构

    • 优化并简化了请求处理流程,减少冗余操作,仅记录关键请求数据,进一步提升整体系统稳定性。

Copy link

coderabbitai bot commented Feb 7, 2025

"""

Walkthrough

此次提交对两个 GuzzleHttpClientAspect 类进行了重构。主要更新包括在拦截请求时由原来的 request/requestAsync 方法切换为 transfer 方法,同时引入 on_stats 回调来捕获传输统计信息。针对 Sentry 的面包屑记录和追踪信息注入,分别在处理逻辑中进行了优化和简化。部分冗余的计时代码和异常处理代码已被移除,并合并了对 no_sentry_aspect/no_sentry_tracing 选项的判断。

Changes

文件 更改描述
src/sentry/.../Aspect/GuzzleHttpClientAspect.php 替换了 request/requestAsync 为 transfer;重构 process 方法,移除了直接提取响应和计时代码,改用 on_stats 回调记录 Sentry 面包屑,同时合并 no_sentry_aspect 判断。
src/sentry/.../Tracing/Aspect/GuzzleHttpClientAspect.php 同样替换了 request/requestAsync 为 transfer;更新了请求跟踪逻辑,通过 on_stats 回调捕获传输信息,将 tracing 上下文注入请求头,并依据响应状态更新 span 状态,简化了错误处理流程。

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: 记录面包屑
Loading
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 状态
Loading

Possibly related PRs

  • Refactor GuzzleHttpClientAspect #834: 修改了 GuzzleHttpClientAspect 的请求处理和 on_stats 逻辑,与本次修改在拦截和统计信息处理上有较强的代码关联。

Suggested reviewers

  • guandeng

Poem

我是小兔,轻快飞,
新代码如春风拂过林隙,
on_stats 带来清脆节拍,
Sentry 与追踪共舞 —— 🐇
代码成诗,乐在其中。
"""

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between e51dc90 and eb9add7.

📒 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 继续执行请求,流程衔接顺畅。

Copy link

@coderabbitai coderabbitai bot left a 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 回调

建议考虑以下优化:

  1. 考虑将统计数据收集逻辑抽取到单独的方法中,以提高代码可维护性
  2. 可以添加配置选项来控制是否收集某些统计信息,优化性能

示例重构:

+ 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

📥 Commits

Reviewing files that changed from the base of the PR and between eb9add7 and 640af58.

📒 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: 导入更改符合新的实现需求

更改后的导入声明更好地反映了类的职责,通过使用 TransferStatsRequestInterface 来增强请求跟踪能力。


36-38: 优化了方法拦截策略

将拦截方法从 request/requestAsync 改为 transfer 是一个很好的改进,因为:

  • transfer 方法是所有请求的底层实现
  • 可以同时覆盖同步和异步请求
  • 减少了重复代码

56-63: 改进了类型安全性和配置获取

代码质量有了显著提升:

  • 添加了 RequestInterface 类型提示,增强了类型安全性
  • 使用闭包优化了配置获取逻辑
  • 通过 getInstance() 改进了实例访问方式

@huangdijia huangdijia changed the title feat: 更新 GuzzleHttpClientAspect 以支持 transfer 方法并增强请求记录功能 feature: Update GuzzleHttpClientAspect to support transfer methods and enhance request logging Feb 7, 2025
@huangdijia huangdijia marked this pull request as ready for review February 7, 2025 09:58
@huangdijia huangdijia merged commit a5143f7 into main Feb 7, 2025
16 checks passed
@huangdijia huangdijia deleted the patch-sentry branch February 7, 2025 09:58
huangdijia added a commit that referenced this pull request Feb 7, 2025
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant