refactor(sentry): simplify tracing data collection and remove sensitive exception details #941
Conversation
…ve exception details - Remove detailed exception information (class, code, message) from tracing spans for security - Simplify HTTP status handling by using setHttpStatus() method instead of manual status/tag setting - Remove coroutine ID tracking from HTTP client spans to reduce data overhead - Standardize exception handling across all tracing components to only set status without exposing details - Clean up unused imports in GuzzleHttpClientAspect This improves security by preventing sensitive exception data from being sent to Sentry while maintaining essential tracing functionality and error status tracking.
Walkthrough统一简化 Sentry Tracing 的错误与 HTTP 状态处理：移除异常相关标签与元数据，仅将 span/transaction 标记为 internalError；HTTP 场景改用 setHttpStatus；Guzzle on_stats 与响应体采集逻辑精简；去掉协程 ID 注入与重复异常标签。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant G as GuzzleHttpClientAspect
participant C as GuzzleClient
participant S as Span/Transaction
App->>G: send(request, options)
G->>S: start span (inject tracing headers)
G->>C: execute request (options + on_stats)
C-->>G: response / exception
alt 有响应
G->>S: setHttpStatus(response.status)
G->>S: record response body (text or "[binary omitted]")
else 异常
G->>S: setStatus(internalError)
end
C-->>G: on_stats(stats)
G->>S: record timing/response metadata
G-->>App: return / rethrow
S-->>S: finish
sequenceDiagram
autonumber
participant Server
participant L as EventHandleListener
participant T as Transaction
Server->>L: RequestHandled(event)
L->>T: setHttpStatus(event.status)
L->>Server: propagate traceId -> RpcContext / response header
alt event 有 throwable
L->>T: setStatus(internalError)
end
T-->>T: finish
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)src/*/src/**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{src,tests}/**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/*/**📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧬 Code graph analysis (1)src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
⏰ 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). (6)
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.28)At least one path must be specified to analyse. 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. Comment |
…ent types directly
…t to KafkaEvent and AsyncQueueEvent
…eEvent\FailToConsume to AsyncQueueEvent\FailedHandle
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
91-110: 两处风险：请求体大小的类型安全与敏感数据采集（头与 options/config）
- 类型安全：
strlen($options['body'] ?? '')在 body 为资源/StreamInterface/数组时会报错。应优先使用$request->getBody()->getSize()。- 隐私/合规：
http.request.headers、http.response.headers及http.guzzle.options/http.guzzle.config可能包含 Authorization/Cookie/密钥/证书路径等敏感信息，与本 PR 的“减少敏感数据”目标相悖，且还会放大 JSON 序列化负担。建议最小化与脱敏（示例 diff 仅展示关键替换，保留其余字段不变）：
- 'http.request.body.size' => strlen($options['body'] ?? ''), + 'http.request.body.size' => $request->getBody()->getSize(), - 'http.request.headers' => $request->getHeaders(), + // 脱敏部分敏感头 + 'http.request.headers' => array_filter( + $request->getHeaders(), + static fn($k) => !in_array(strtolower($k), [ + 'authorization','proxy-authorization','cookie','set-cookie','x-api-key','x-auth-token' + ]), + ARRAY_FILTER_USE_KEY + ), - 'http.guzzle.config' => $guzzleConfig, - 'http.guzzle.options' => $options ?? [], + // 避免采集可能含凭据/闭包的配置与原始 options + // 如确需调试，可通过 Feature 开关仅采集白名单键的浅拷贝与之对称，响应头也建议过滤 Set-Cookie 等：
- 'http.response.headers' => $response->getHeaders(), + 'http.response.headers' => array_filter( + $response->getHeaders(), + static fn($k) => !in_array(strtolower($k), ['set-cookie']), + ARRAY_FILTER_USE_KEY + ),如需，我可以按现有 Feature 开关（isTracingExtraTagEnabled）补一版白名单/脱敏实现。
Also applies to: 111-121, 122-135
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
419-425: 状态覆盖问题：存在异常时会被 “成功退出码” 覆盖为 ok当前先基于异常置为 internalError，随后无条件按 exitCode 赋值；当 exitCode==SUCCESS 且存在异常时，最终状态会被覆盖为 ok。应按“异常优先”处理。
- if (method_exists($event, 'getThrowable') && $event->getThrowable()) { - $transaction->setStatus(SpanStatus::internalError()); - } - - $transaction->setStatus( - $exitCode == SymfonyCommand::SUCCESS ? SpanStatus::ok() : SpanStatus::internalError() - ); + if (method_exists($event, 'getThrowable') && $event->getThrowable()) { + $transaction->setStatus(SpanStatus::internalError()); + } else { + $transaction->setStatus( + $exitCode == SymfonyCommand::SUCCESS ? SpanStatus::ok() : SpanStatus::internalError() + ); + }
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
112-118: 异常仅标记 internalError 的方向正确；建议确认协程事务是否在执行期处于激活态当前在协程中 startTransaction 后，仅在 defer 中 setSpan($coTransaction)。为确保协程内产生的子 span 都能正确挂载到该事务，建议在开始处也显式 set 一次（若 startTransaction 已内部 setSpan，可忽略）。
可按下述方式在创建 $coTransaction 后立即激活（保持 defer 中的 setSpan 不变也无害）：
$coTransaction = startTransaction( continueTrace($parent->toTraceparent(), $parent->toBaggage()) ->setName('coroutine') ->setOp('coroutine.execute') ->setDescription($callingOnFunction) ->setOrigin('auto.coroutine') ); + // 确保协程执行阶段该事务为当前激活 span + SentrySdk::getCurrentHub()->setSpan($coTransaction);src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
354-359: LGTM：向 RPC/HTTP 回传 traceId 便于跨边界排障将 traceId 写入 RpcContext 或响应头有助于排查。可选：若上游已注入同名头，考虑仅在缺失时设置以保持幂等。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(1 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(7 hunks)src/sentry/src/Tracing/Tracer.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/*/src/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use the namespace pattern FriendsOfHyperf{ComponentName} in all component PHP source files
Files:
src/sentry/src/Tracing/Aspect/CoroutineAspect.phpsrc/sentry/src/Tracing/Tracer.phpsrc/sentry/src/Tracing/Listener/EventHandleListener.phpsrc/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
{src,tests}/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to PSR-12 coding standards across PHP code
Files:
src/sentry/src/Tracing/Aspect/CoroutineAspect.phpsrc/sentry/src/Tracing/Tracer.phpsrc/sentry/src/Tracing/Listener/EventHandleListener.phpsrc/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
src/*/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
New components must follow the standard component structure under src/{component-name}/ including .gitattributes, .github, LICENSE, README.md, composer.json, and a src/ subdirectory
Files:
src/sentry/src/Tracing/Aspect/CoroutineAspect.phpsrc/sentry/src/Tracing/Tracer.phpsrc/sentry/src/Tracing/Listener/EventHandleListener.phpsrc/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
src/exception-event/src/Event/ExceptionDispatched.php (1)
getThrowable(30-33)
⏰ 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). (12)
- 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.2 with Swoole 6.0.2
- 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.3 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 6.0.2
- 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 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
🔇 Additional comments (10)
src/sentry/src/Tracing/Tracer.php (1)
84-89: LGTM：异常时仅设置 internalError，保持最小采集使用空合运算符安全地更新当前 span 状态，符合本 PR 的隐私目标与一致性处理。
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (3)
136-139: LGTM：二进制响应体统一占位非文本或不可回溯响应体以 “[binary omitted]” 占位，避免泄露内容且语义清晰。
145-147: LGTM：底层 Handler 报错时将状态置为 internalError与全局策略一致，简单明确。
142-143: 确认 setHttpStatus 方法已在当前 SDK 版本中提供根目录及
src/sentry/composer.json中锁定了sentry/sentry为^4.16.0，在vendor/sentry/sentry/src/Tracing/Span.php:289处已定义public function setHttpStatus(int $statusCode)，无需额外兼容性处理。src/sentry/src/Tracing/Listener/EventHandleListener.php (6)
361-362: 关于 setHttpStatus 的 SDK 版本兼容性请核实此处同样依赖
$span->setHttpStatus()。为避免生产报错，请确认已升级到包含该 API 的 sentry/sentry 版本。可复用我在 Guzzle 评论内的脚本。
455-457: LGTM：Redis 命令异常仅标记 internalError，避免泄露异常细节与全局策略一致。
524-526: LGTM：Crontab 失败仅标记 internalError简洁一致。
596-598: LGTM：AMQP 消费异常仅标记 internalError符合本次收敛策略。
660-662: LGTM：Kafka 消费异常仅标记 internalError与其他队列分支保持一致。
711-713: LGTM：异步队列异常仅标记 internalError一致性良好。
…ve exception details (#941) * refactor(sentry): simplify tracing data collection and remove sensitive exception details - Remove detailed exception information (class, code, message) from tracing spans for security - Simplify HTTP status handling by using setHttpStatus() method instead of manual status/tag setting - Remove coroutine ID tracking from HTTP client spans to reduce data overhead - Standardize exception handling across all tracing components to only set status without exposing details - Clean up unused imports in GuzzleHttpClientAspect This improves security by preventing sensitive exception data from being sent to Sentry while maintaining essential tracing functionality and error status tracking. * refactor(EventHandleListener): simplify error handling by checking event types directly * refactor(EventHandleListener): update event type checks from AmqpEvent to KafkaEvent and AsyncQueueEvent * refactor(EventHandleListener): update event type check from AsyncQueueEvent\FailToConsume to AsyncQueueEvent\FailedHandle * refactor(EventHandleListener): add method_exists check for getThrowable before usage --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
This PR refactors the Sentry tracing components to improve security and reduce data overhead by removing sensitive exception details from tracing spans while maintaining essential error tracking functionality.
Key Changes
setHttpStatus()methodFiles Modified
src/sentry/src/Tracing/Aspect/CoroutineAspect.php: Simplified exception handlingsrc/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php: Removed sensitive data collection and unused importssrc/sentry/src/Tracing/Listener/EventHandleListener.php: Standardized error handling across all event listenerssrc/sentry/src/Tracing/Tracer.php: Simplified exception handling in trace methodBenefits
Test Plan
Summary by CodeRabbit