Skip to content

Conversation

@huangdijia
Copy link
Contributor

@huangdijia huangdijia commented Dec 12, 2025

Summary

  • Improves the GuzzleHttpClientAspect to properly detect and handle binary responses
  • Prevents attempting to read binary content as text, avoiding potential memory issues
  • Adds Content-Type header checking to determine if response is textual

Changes

  • Added Content-Type header validation to detect textual vs binary responses
  • Returns 'Binary Response' placeholder for non-textual content to avoid processing
  • Maintains backward compatibility for all textual responses (JSON, XML, HTML, etc.)
  • Improves performance by avoiding unnecessary stream operations on binary data

Test Plan

  • Verify binary responses (images, files) show as 'Binary Response' in Telescope
  • Confirm JSON responses still display correctly
  • Test XML and text responses remain functional
  • Ensure no memory issues when handling large binary downloads
  • Check that stream is properly handled when seekable

Summary by CodeRabbit

  • Bug Fixes
    • 优化了 HTTP 响应处理:基于 Content-Type(含 grpc)区分文本/二进制并检测流可寻位性,对于非文本或不可寻位的响应使用占位符,避免直接读取流导致问题。
    • 响应记录现在会考虑请求选项:将请求级 options 传入统计回调,stream 模式返回“Streamed Response”,摘要生成更准确。
    • 增强了异常保护与流状态一致性,错误情况下返回简洁占位信息并保留对 HTML 清理/隐藏行为的既有支持。

✏️ Tip: You can customize this high-level summary in your review settings.

- Check Content-Type header to determine if response is textual before reading
- Return 'Binary Response' placeholder for non-textual content
- Avoid unnecessary stream operations on binary responses
- Maintain backward compatibility for textual responses

This prevents attempting to read or process binary content as text, which
could cause memory issues or corrupted logs when dealing with file downloads,
images, or other binary API responses.
Copilot AI review requested due to automatic review settings December 12, 2025 02:36
@coderabbitai
Copy link

coderabbitai bot commented Dec 12, 2025

Warning

Rate limit exceeded

@huangdijia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 59 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 9493633 and 01a9c1d.

📒 Files selected for processing (2)
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (4 hunks)
  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php (5 hunks)

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

在 Guzzle HTTP 客户端切面中将 $options 传入 stats 回调并抽取响应读取逻辑为 getResponsePayload,新增 options['stream']、基于 Content-Type 的文本/二进制判断及对不可寻流的条件性 rewind 与错误占位处理(不含主观评价)。

Changes

Cohort / File(s) Change summary
Telescope: Guzzle 响应处理与选项传播
src/telescope/src/Aspect/GuzzleHttpClientAspect.php
on_stats 闭包中传入并使用 $options;将响应读取逻辑抽取并改签名为 public function getResponsePayload(ResponseInterface $response, array $options = []): mixed;支持 options['stream'] 分支、基于 Content-Type 的文本性判断(缺失 Content-Type 视为文本)、对二进制/流返回使用占位字符串;在可寻性下条件性 rewind 并保留异常与 purge 逻辑。
Sentry: 提取并复用响应读取逻辑
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
新增受保护方法 protected function getResponsePayload(ResponseInterface $response, array $options = []): mixed,将原 inline 响应体提取迁移至该方法;方法支持 stream 标记、Content-Type 文本性判断(含 grpc)、不可寻流与异常情况下返回占位;调用处改为使用该方法,仅在采集 http.response.body.contents 时生效。

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • 审查 Content-Type 的匹配/正则与 grpc 覆盖情况
  • 验证对不可寻性流(non-seekable)的处理与 finally 中条件性 rewind 是否安全
  • 检查 options['stream'] 在不同请求场景下的兼容性及回退行为

Possibly related PRs

Suggested reviewers

  • guandeng

诗一首

🐰 我在代码巷里轻轻跳,
若是流动我便喊一号,
文本二进制我分清道,
小心 rewind 不发骄,
选项随身,响应安好。

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题清晰准确地总结了主要变更:改进GuzzleHttpClientAspect中二进制响应的处理。标题与changeset的核心内容完全相关,简洁明了。

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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: 1

🧹 Nitpick comments (2)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (2)

99-102: 建议:考虑扩展文本内容类型检测范围

当前的正则表达式较为保守,可能会遗漏一些常见的文本内容类型,例如:

  • application/javascriptapplication/ecmascript
  • application/*+json(如 application/ld+json
  • application/*+xml(如 application/soap+xmlapplication/atom+xml

如果项目需要处理这些内容类型,建议扩展正则表达式以包含它们。

如需支持更多文本类型,可应用此修改:

         $isTextual = \preg_match(
-            '/^(text\/|application\/(json|xml|x-www-form-urlencoded|grpc))/i',
+            '/^(text\/|application\/(json|xml|javascript|ecmascript|x-www-form-urlencoded|grpc|\w+\+(json|xml)))/i',
             $response->getHeaderLine('Content-Type')
         ) === 1;

104-107: 可选优化:占位符消息可能产生误导

当条件 ! ($isTextual && $stream->isSeekable()) 为真时,既包括二进制响应,也包括文本但不可定位的流。对于后者,返回 "Binary Response" 可能会产生误导。

虽然不处理不可定位的流在技术上是正确的(因为无法在读取后重置位置),但考虑使用更准确的消息,如 "Response Not Captured""Binary or Non-Seekable Response"

如需提高准确性,可应用此修改:

-        if (! ($isTextual && $stream->isSeekable())) {
-            return 'Binary Response';
-        }
+        if (! $isTextual) {
+            return 'Binary Response';
+        }
+        
+        if (! $stream->isSeekable()) {
+            return 'Non-Seekable Stream';
+        }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 96adae5 and e9ae153.

📒 Files selected for processing (1)
  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/*/src/**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

All components use the namespace pattern: FriendsOfHyperf\{ComponentName}

Files:

  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php
**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.php: Follow PSR-12 coding standards for all PHP code
Use PHP-CS-Fixer for code formatting as configured in .php-cs-fixer.php
Use PHPStan for static analysis as configured in phpstan.neon.dist

Files:

  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php
⏰ 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). (20)
  • GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
  • GitHub Check: Agent
  • GitHub Check: Seer Code Review
🔇 Additional comments (1)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)

109-141: 代码结构改进得当

新增的 try-finally 块确保即使在处理过程中发生异常,可定位的流也会被正确重置。这种结构可以防止流状态问题,是一个良好的防御性编程实践。

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to improve binary response handling in the Telescope component's GuzzleHttpClientAspect by detecting Content-Type headers and avoiding unnecessary processing of binary data. However, the implementation introduces critical bugs that will break existing functionality.

Key Changes:

  • Adds Content-Type header validation to distinguish textual from binary responses
  • Returns 'Binary Response' placeholder for non-textual content
  • Modifies stream handling logic to check seekability earlier
Comments suppressed due to low confidence (1)

src/telescope/src/Aspect/GuzzleHttpClientAspect.php:144

  • The getResponsePayload method lacks test coverage. Given the complexity of this logic and the various edge cases (binary vs textual responses, seekable vs non-seekable streams, different content types, size limits), comprehensive tests should be added to verify correct behavior for all scenarios mentioned in the test plan.
    public function getResponsePayload(ResponseInterface $response)
    {
        $stream = $response->getBody();

        // Determine if the response is textual based on the Content-Type header.
        $isTextual = \preg_match(
            '/^(text\/|application\/(json|xml|x-www-form-urlencoded))/i',
            $response->getHeaderLine('Content-Type')
        ) === 1;

        // If the response is not textual or the stream is not seekable, we will return a placeholder.
        if (! ($isTextual && $stream->isSeekable())) {
            return 'Binary Response';
        }

        try {
            $content = $stream->getContents();

            if (is_string($content)) {
                if (! $this->contentWithinLimits($content)) {
                    return 'Purged By Hyperf Telescope';
                }
                if (
                    is_array(json_decode($content, true))
                    && json_last_error() === JSON_ERROR_NONE
                ) {
                    return $this->contentWithinLimits($content) /* @phpstan-ignore-line */
                    ? $this->hideParameters(json_decode($content, true), Telescope::$hiddenResponseParameters)
                    : 'Purged By Hyperf Telescope';
                }
                if (Str::startsWith(strtolower($response->getHeaderLine('content-type') ?: ''), 'text/plain')) {
                    return $this->contentWithinLimits($content) ? $content : 'Purged By Hyperf Telescope'; /* @phpstan-ignore-line */
                }
                if (Str::contains($response->getHeaderLine('content-type'), 'application/grpc') !== false) {
                    return TelescopeContext::getGrpcResponsePayload() ?: 'Purged By Hyperf Telescope';
                }
            }

            if (empty($content)) {
                return 'Empty Response';
            }
        } catch (Throwable $e) {
            return 'Purged By Hyperf Telescope: ' . $e->getMessage();
        } finally {
            if ($stream->isSeekable()) {
                $stream->rewind();
            }
        }

        return 'HTML Response';
    }

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: 2

♻️ Duplicate comments (2)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (2)

102-111: gRPC 响应处理被破坏

正则表达式 /^(text\/|application\/(json|xml|x-www-form-urlencoded))/i 不匹配 application/grpc,导致 gRPC 响应被错误地识别为二进制,在第 110 行提前返回 "Binary Response",永远无法到达第 131-133 行的 gRPC 专用处理逻辑。

建议将 gRPC 添加到正则表达式中:

         $isTextual = \preg_match(
-            '/^(text\/|application\/(json|xml|x-www-form-urlencoded))/i',
+            '/^(text\/|application\/(json|xml|x-www-form-urlencoded|grpc))/i',
             $response->getHeaderLine('Content-Type')
         ) === 1;

113-114: 读取流内容前需要先 rewind

与 Sentry 版本存在相同问题:$stream->getContents() 从当前位置读取,但在 on_stats 回调时流指针可能已在末尾,导致返回空字符串。

参考相同文件中其他位置(如 RequestHandledListener.php 第 136-139 行)的实现,应在读取前先 rewind。

         try {
+            if ($stream->isSeekable()) {
+                $stream->rewind();
+            }
             $content = $stream->getContents();
🧹 Nitpick comments (1)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)

94-98: 占位符字符串风格不一致

Sentry 版本使用方括号格式(如 [Streamed Response]),而 Telescope 版本使用无括号格式(Streamed Response)。建议保持两个模块的一致性,便于用户识别这些是占位符而非实际响应内容。

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c7588b and 2f539be.

📒 Files selected for processing (2)
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (3 hunks)
  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/*/src/**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

All components use the namespace pattern: FriendsOfHyperf\{ComponentName}

Files:

  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php
**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.php: Follow PSR-12 coding standards for all PHP code
Use PHP-CS-Fixer for code formatting as configured in .php-cs-fixer.php
Use PHPStan for static analysis as configured in phpstan.neon.dist

Files:

  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php
🧬 Code graph analysis (2)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (3)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)
  • getResponsePayload (94-146)
src/telescope/src/Middleware/TelescopeMiddleware.php (1)
  • getResponsePayload (106-138)
src/telescope/src/Listener/RequestHandledListener.php (1)
  • getResponsePayload (133-170)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
  • getResponsePayload (147-179)
⏰ 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). (19)
  • GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
  • 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.2 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
  • GitHub Check: Seer Code Review
🔇 Additional comments (2)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)

119-123: LGTM!

正确地将 $options 传递给 getResponsePayload 方法,支持流式响应检测。

src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)

63-78: LGTM!

正确地将 $options 捕获到闭包中并传递给 getResponsePayload,支持流式响应检测功能。

huangdijia and others added 7 commits December 12, 2025 11:19
Updated the regex in GuzzleHttpClientAspect to include 'application/grpc' as a textual Content-Type. This ensures that gRPC responses are correctly identified as textual for further processing.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@huangdijia huangdijia requested a review from Copilot December 12, 2025 03:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

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: 2

♻️ Duplicate comments (3)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)

154-163: 非 seekable 流会被 getContents() 消耗且无法恢复,可能影响业务侧后续读取(且 on_stats 场景下也容易读到空)。这点在切面/监控里风险较高,建议对非 seekable 直接返回占位符而不是读取。

         $stream = $response->getBody();

         try {
-            if ($stream->isSeekable()) {
-                $stream->rewind();
-            }
+            if (! $stream->isSeekable()) {
+                return '[Non-Seekable Stream]';
+            }
+            $stream->rewind();

             $content = $stream->getContents();

Also applies to: 165-183

src/telescope/src/Aspect/GuzzleHttpClientAspect.php (2)

100-117: gRPC 响应会被提前判定为二进制而直接返回,导致 TelescopeContext::getGrpcResponsePayload() 分支永远到不了application/grpc 不匹配当前正则)。

-            $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|grpc))/i',
+                $contentType
+            ) === 1;

Also applies to: 143-145


101-117: No Content-Type Header (treated as text) 这条分支逻辑当前不可达(空 Content-Type 已强制 $isTextual = true,建议删除该三元分支,避免误导维护者。

         if (! $isTextual) {
-            return $contentType === ''
-                ? 'No Content-Type Header (treated as text)'
-                : 'Binary Response';
+            return 'Binary Response';
         }
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)

154-158: 文本 MIME 识别建议补充 +json/+xml 等常见类型,避免误判成二进制(如 application/problem+jsonapplication/vnd.api+json)。

src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)

106-110: 文本 MIME 识别建议覆盖 .*\+json / .*\+xml,避免把常见 API 响应误判为二进制。

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f539be and 58d28de.

📒 Files selected for processing (2)
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (3 hunks)
  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/*/src/**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

All components use the namespace pattern: FriendsOfHyperf\{ComponentName}

Files:

  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.php: Follow PSR-12 coding standards for all PHP code
Use PHP-CS-Fixer for code formatting as configured in .php-cs-fixer.php
Use PHPStan for static analysis as configured in phpstan.neon.dist

Files:

  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
🧬 Code graph analysis (2)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
  • getResponsePayload (148-184)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (3)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)
  • getResponsePayload (94-158)
src/telescope/src/Middleware/TelescopeMiddleware.php (1)
  • getResponsePayload (106-138)
src/telescope/src/Listener/RequestHandledListener.php (1)
  • getResponsePayload (133-170)
⏰ 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). (3)
  • GitHub Check: Seer Code Review
  • GitHub Check: PHP CS Fixer on PHP 8.1
  • GitHub Check: PHP CS Fixer on PHP 8.1
🔇 Additional comments (2)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)

120-124: 将响应体采集逻辑抽到 getResponsePayload() 的方向是对的on_stats 里只在 tag 开启时写入 http.response.body.contents,避免无谓开销。

src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)

63-79: on_stats$options 透传进 getResponsePayload() 是必要的(例如 stream 场景),这块改动合理。

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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: 1

♻️ Duplicate comments (2)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (2)

117-153: 非可寻址(non-seekable)响应体在 on_stats 阶段易落在 EOF:建议直接占位而不是读空
现在只在 seekable 时 rewind,但无论是否 seekable 都会 getContents();对于 non-seekable(且未显式 stream=true)常见结果是读到空串并记录 'Empty Response',属于误报/丢信息。

建议增加 non-seekable 的降级占位(避免“读空当空响应”):

         $stream = $response->getBody();

         try {
+            if (! $stream->isSeekable()) {
+                return 'Streamed Response';
+            }
             if ($stream->isSeekable()) {
                 $stream->rewind();
             }
@@
         } finally {
             $stream->isSeekable() && $stream->rewind();
         }

100-115: gRPC 被误判为二进制,导致后续 gRPC 专用处理永远到不了(需修复)
application/grpc 目前不匹配正则,直接返回 'Binary Response',从而绕过 Line 141-143 的 gRPC payload 逻辑。

-            $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|grpc))/i',
+                $contentType
+            ) === 1;
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 58d28de and 9493633.

📒 Files selected for processing (2)
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (4 hunks)
  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/*/src/**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

All components use the namespace pattern: FriendsOfHyperf\{ComponentName}

Files:

  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.php: Follow PSR-12 coding standards for all PHP code
Use PHP-CS-Fixer for code formatting as configured in .php-cs-fixer.php
Use PHPStan for static analysis as configured in phpstan.neon.dist

Files:

  • src/telescope/src/Aspect/GuzzleHttpClientAspect.php
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (3)
src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)
  • getResponsePayload (94-156)
src/telescope/src/Listener/RequestHandledListener.php (1)
  • getResponsePayload (133-170)
src/telescope/src/Middleware/TelescopeMiddleware.php (1)
  • getResponsePayload (106-138)
⏰ 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). (19)
  • GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
  • GitHub Check: Seer Code Review
🔇 Additional comments (3)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (2)

36-37: 新增响应体上限(8KB)方向正确,能避免 OOM/大包拖垮 worker
MAX_RESPONSE_BODY_SIZE + Utils::copyToString(..., +1) 做截断判断很实用。


122-126: 抽取 getResponsePayload() 并在 span 写入点复用:可维护性更好
http.response.body.contents 采集逻辑下沉后,后续对“二进制/流式/截断”等策略调整更集中。

src/telescope/src/Aspect/GuzzleHttpClientAspect.php (1)

63-79: on_stats 透传 $options 给 payload 计算:行为更可控
能让 stream 等选项影响采集逻辑,符合本 PR 目标。

@huangdijia huangdijia merged commit a71c6c7 into main Dec 12, 2025
23 checks passed
@huangdijia huangdijia deleted the feature/telescope-guzzle-aspect-binary-response-fix branch December 12, 2025 05:32
huangdijia added a commit that referenced this pull request Dec 12, 2025
…1041)

* fix: improve handling of binary responses in GuzzleHttpClientAspect

- Check Content-Type header to determine if response is textual before reading
- Return 'Binary Response' placeholder for non-textual content
- Avoid unnecessary stream operations on binary responses
- Maintain backward compatibility for textual responses

This prevents attempting to read or process binary content as text, which
could cause memory issues or corrupted logs when dealing with file downloads,
images, or other binary API responses.

* fix: 修复 GuzzleHttpClientAspect 中对响应有效负载的处理逻辑

* fix: 修复 GuzzleHttpClientAspect 中对流响应的处理逻辑

* fix: 修复 GuzzleHttpClientAspect 中对流响应的处理逻辑

* fix: 改进 GuzzleHttpClientAspect 中对二进制响应的处理逻辑

* fix: 简化 GuzzleHttpClientAspect 中对流响应的处理逻辑

* fix: 修复 GuzzleHttpClientAspect 中对二进制响应的处理逻辑

* fix: 修改 GuzzleHttpClientAspect 中对空字符串响应的处理逻辑

* fix: 修复 GuzzleHttpClientAspect 中对可回溯流的处理逻辑

* Add grpc to textual Content-Type detection

Updated the regex in GuzzleHttpClientAspect to include 'application/grpc' as a textual Content-Type. This ensures that gRPC responses are correctly identified as textual for further processing.

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix: 修复 GuzzleHttpClientAspect 中对 Content-Type 头的处理逻辑

* fix: 处理 GuzzleHttpClientAspect 中的空响应情况

* fix: 优化 GuzzleHttpClientAspect 中的响应类型声明

* fix: 修复 GuzzleHttpClientAspect 中的响应处理逻辑,确保不再接受空响应

* fix: 修复 GuzzleHttpClientAspect 中的响应内容检查逻辑,确保正确处理空字符串响应

* Apply suggestion from @Copilot

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix: 优化 GuzzleHttpClientAspect 中的响应内容处理,增加响应内容截断逻辑

* fix: 优化 GuzzleHttpClientAspect 中的响应内容截断逻辑,改善代码可读性

* fix: 修正 GuzzleHttpClientAspect 中的常量注释格式,改善代码一致性

* fix: 优化 GuzzleHttpClientAspect 中的响应内容处理逻辑,确保在异常情况下不影响请求流程

* fix: 更新 GuzzleHttpClientAspect 中的内容类型匹配逻辑,支持 gRPC 响应类型

* fix: 简化 GuzzleHttpClientAspect 中的响应内容类型判断逻辑

* fix: 优化 GuzzleHttpClientAspect 中的响应内容处理逻辑,支持 gRPC 和纯文本响应

---------

Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.

2 participants