refactor(sentry): improve span lifecycle management and code clarity #914
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Remove manual timestamp passing to span.finish() method, letting Sentry handle timing automatically - Improve span parent/child relationship management in TraceAnnotationAspect - Clean up commented code and improve readability in DbAspect - Simplify transaction creation flow in SpanStarter with better structure - Add proper comments for span restoration and transaction finishing - Ensure consistent span finishing patterns across all aspects and listeners This refactoring improves the reliability of span timing and makes the code more maintainable.
|
Warning Rate limit exceeded@huangdijia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 40 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 (22)
Walkthrough统一将多处 span/transaction 的 finish 调用改为无参 finish()；在事件监听器中新增数据库事务事件处理；条件化添加 SQL bindings；重构 SpanStarter 的上下文与启动流程；为 Switcher 的 is* 方法引入可配置默认值；若干 Aspect 调整控制流与早返回。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as 客户代码
participant TA as TraceAnnotationAspect
participant H as Hub
participant S as Span
C->>TA: 调用受注解方法
TA->>H: 捕获 parent span
alt parent 存在且 sampled
TA->>S: start span
TA->>C: 执行并早返回结果
end
rect rgb(245,245,255)
Note right of TA: finally（无论）
TA->>S: finish()
TA->>H: 恢复 parent span
end
sequenceDiagram
autonumber
participant L as EventHandleListener
participant Hub as Hub
participant Tx as Transaction
participant Sp as Span
Note over L,Tx: 新增事务事件处理序列
L->>Tx: TransactionBeginning
alt 当前 span 存在且开关允许
L->>Sp: 创建事务 span
end
L->>Tx: TransactionCommitted / RolledBack
L->>Sp: finish()（无参）
sequenceDiagram
autonumber
participant SS as SpanStarter
participant SC as SpanContext
participant TC as TransactionContext
participant Hub as Hub
SS->>SC: 预构建 SpanContext（链式设置）
SS->>TC: 继续/构造 TransactionContext 并应用 options
SS->>Hub: startTransaction(TC)
SS->>Hub: setSpan(transaction)
SS->>Hub: startChild(SC)
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
254-256: 避免记录敏感请求头（合规/隐私风险）当前会把所有请求头写入 span data，可能包含 Authorization/Cookie/X-API-Key 等敏感信息，存在泄露风险。建议默认脱敏常见敏感头，仅在明确配置允许时放开。
可在此处做白/黑名单过滤，例如：
- foreach ($request->getHeaders() as $key => $value) { - $data['http.request.header.' . $key] = implode(', ', $value); - } + $sensitive = ['authorization','proxy-authorization','cookie','set-cookie','x-api-key','x-auth-token']; + foreach ($request->getHeaders() as $key => $value) { + $lower = strtolower($key); + $data['http.request.header.' . $key] = in_array($lower, $sensitive, true) + ? '[REDACTED]' + : implode(', ', $value); + }如需，我可以加一个 sentry.sanitize_headers 配置项支持自定义。
🧹 Nitpick comments (1)
src/sentry/src/Tracing/SpanStarter.php (1)
90-107: 变参选项键名依赖命名实参，请确认一致性这里通过 ...$options 捕获并用 $key 匹配 'name'/'op' 等键。需要调用方始终以命名实参调用（例如 name: 'xxx'）。若存在位置实参或 PHP 版本行为差异，选项将不会生效。
为稳妥，可将签名改为 array $options = [] 并在调用侧显式传关联数组，或为 continueTrace 定义具名参数而非变参。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/DbAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php(2 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(6 hunks)src/sentry/src/Tracing/SpanStarter.php(2 hunks)
💤 Files with no reviewable changes (1)
- src/sentry/src/Tracing/Aspect/DbAspect.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
⏰ 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). (9)
- 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 6.0.2
- GitHub Check: Test on PHP 8.2 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 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
🔇 Additional comments (7)
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)
67-67: 统一改为无参 finish() 👍与本 PR 目标一致，交由 SDK 自行记录结束时间，finally 中调用可确保异常路径也能收尾。
src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (2)
66-74: 在 try 内提前 return，语义更直接成功路径在 try 内直接返回，配合 finally 可读性更好、不会漏收尾。
87-91: 先 finish 子 span，再恢复父 span 的顺序是正确的避免后续逻辑继续把已结束的子 span 作为 current 使用。
请确认调用方没有依赖方法末尾（finally 之后）的 return 语义变更；当前实现仅在 try 内返回结果。
src/sentry/src/Tracing/Listener/EventHandleListener.php (2)
270-279: defer 顺序调整合理先结束子 span，再把 transaction 设回 Hub，最后结束 transaction，层级结束次序正确。
379-380: 标准化为无参 $transaction->finish()与其余组件一致，减少手动时间戳带来的偏差。
Also applies to: 485-486, 563-564, 635-636, 694-695
src/sentry/src/Tracing/SpanStarter.php (2)
92-94: 整体流程更扁平化，易读先构建 TransactionContext 再 startTransaction，最后 setSpan 到 Hub，符合预期。
Also applies to: 109-113
42-47: SpanContext::make() 可能不存在（运行时致命）Sentry PHP SDK 的 SpanContext 通常通过构造函数实例化；静态 make() 并非标准 API。此处调用很可能触发致命错误。
建议改为直接 new，或使用框架的 make() 全局函数：
- $spanContext = SpanContext::make()->setOp($op) + $spanContext = (new SpanContext())->setOp($op) ->setDescription($description) ->setOrigin($origin) ->setStatus(SpanStatus::ok()) ->setStartTimestamp(microtime(true));Likely an incorrect or invalid review comment.
…t and SQL binding options
…nd EventHandleListener
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
469-471: 移除多余的乘以 1000 操作
Hyperf\Redis\Event\CommandExecuted::$time 已经是以毫秒为单位（由(microtime(true) - $start) * 1000计算并 round），不需要再乘以 1000，否则会导致指标被放大 1000 倍。请在src/sentry/src/Tracing/Listener/EventHandleListener.php的第 469–471 行将'duration' => $event->time * 1000,修改为
'duration' => $event->time,
🧹 Nitpick comments (2)
src/sentry/publish/sentry.php (1)
101-111: 缺少 sql_transactions 的可配置开关，无法在发布配置中显式关闭/开启监听器里使用了 isTracingSpanEnable('sql_transactions')，但发布配置的 tracing.spans 未提供该项；当前将退回到默认值，用户不易发现和控制。
建议补齐开关，保持与其它 spans 一致：
'spans' => [ 'cache' => env('SENTRY_TRACING_SPANS_CACHE', true), 'coroutine' => env('SENTRY_TRACING_SPANS_COROUTINE', true), 'db' => env('SENTRY_TRACING_SPANS_DB', true), 'elasticsearch' => env('SENTRY_TRACING_SPANS_ELASTICSEARCH', true), 'guzzle' => env('SENTRY_TRACING_SPANS_GUZZLE', true), 'rpc' => env('SENTRY_TRACING_SPANS_RPC', true), 'grpc' => env('SENTRY_TRACING_SPANS_GRPC', true), 'redis' => env('SENTRY_TRACING_SPANS_REDIS', true), 'sql_queries' => env('SENTRY_TRACING_SPANS_SQL_QUERIES', true), + 'sql_transactions' => env('SENTRY_TRACING_SPANS_SQL_TRANSACTIONS', true), ],src/sentry/src/Switcher.php (1)
25-33: 布尔值读取建议做更稳健的归一化，避免 'false' 字符串被当作 true不少环境变量/配置可能以字符串形式出现，(bool) 'false' 会变为 true。建议统一用 FILTER_VALIDATE_BOOLEAN 做解析。
最小改动示例（新增一个私有归一化方法，并替换当前位置的 (bool) 强转）：
class Switcher { public function __construct(protected ConfigInterface $config) { } + private function toBool(mixed $value, bool $default): bool + { + if ($value === null) { + return $default; + } + if (is_bool($value)) { + return $value; + } + if (is_string($value)) { + $v = filter_var($value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); + return $v === null ? $default : $v; + } + return (bool) $value; + } + public function isEnable(string $key, bool $default = true): bool { - return (bool) $this->config->get('sentry.enable.' . $key, $default); + return $this->toBool($this->config->get('sentry.enable.' . $key, $default), $default); } public function isBreadcrumbEnable(string $key, bool $default = true): bool { - return (bool) $this->config->get('sentry.breadcrumbs.' . $key, $default); + return $this->toBool($this->config->get('sentry.breadcrumbs.' . $key, $default), $default); } public function isTracingEnable(string $key, bool $default = true): bool { if (! $this->config->get('sentry.enable_tracing', true)) { return false; } - return (bool) $this->config->get('sentry.tracing.enable.' . $key, $default); + return $this->toBool($this->config->get('sentry.tracing.enable.' . $key, $default), $default); } public function isTracingSpanEnable(string $key, bool $default = true): bool { if (! SentrySdk::getCurrentHub()->getSpan()) { return false; } - return (bool) $this->config->get('sentry.tracing.spans.' . $key, $default); + return $this->toBool($this->config->get('sentry.tracing.spans.' . $key, $default), $default); } public function isTracingExtraTagEnable(string $key, bool $default = false): bool { - return (bool) ($this->config->get('sentry.tracing.extra_tags', [])[$key] ?? $default); + $val = ($this->config->get('sentry.tracing.extra_tags', [])[$key] ?? $default); + return $this->toBool($val, $default); }Also applies to: 35-43, 44-51, 53-56
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sentry/publish/sentry.php(1 hunks)src/sentry/src/Switcher.php(1 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sentry/src/Switcher.php (1)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
src/sentry/src/Tracing/Listener/EventHandleListener.php (3)
src/sentry/src/Switcher.php (2)
isTracingExtraTagEnable(53-56)isTracingSpanEnable(44-51)src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-52)src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
🪛 PHPMD (2.15.0)
src/sentry/src/Tracing/Listener/EventHandleListener.php
235-235: Avoid unused parameters such as '$event'. (undefined)
(UnusedFormalParameter)
249-249: Avoid unused parameters such as '$event'. (undefined)
(UnusedFormalParameter)
⏰ 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 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.2 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.1 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.7
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
🔇 Additional comments (7)
src/sentry/src/Switcher.php (1)
25-33: 为开关方法增加 $default 参数：LGTM消除了硬编码默认值，更易于按需调整，且对现有调用保持向后兼容（末位新增参数、具默认值）。
Also applies to: 35-43, 44-51, 53-56
src/sentry/src/Tracing/Listener/EventHandleListener.php (6)
29-29: 使用 DbEvent 命名空间别名聚合数据库事件：LGTM
76-80: 新增监听事务生命周期事件：LGTM
121-125: process() 的事件分发映射与 listen() 同步：LGTM
325-334: 请求收尾 defer 顺序清晰且与注释一致：LGTM
434-435: 统一使用 finish()（不手动传入 timestamp）：LGTM与 PR 目标一致，避免时钟漂移与重复计算。
Also applies to: 540-541, 618-619, 690-691, 749-750
164-212: 禁止默认上报 SQL 绑定；移除逐项 db.parameter. 并确认 $event->time 单位*
- 风险：逐项写入 db.parameter.*（行 186-188）且同时上报 db.sql.bindings（行 199-201）会重复、增大体积并可能泄露 PII/密钥。
- 建议：删除逐项 db.parameter.*；将 bindings 上报默认设置为关闭，并仅在明确开启时对绑定做脱敏/截断/序列化保护（避免对象/资源直接上报或 setData 序列化异常）。
- 时间单位：Laravel 的 QueryExecuted::$time 为毫秒。(typeerror.org) Hyperf 文档确认存在 Hyperf\Database\Events\QueryExecuted 事件，但文档未显式标注单位，请在仓库源码或你使用的 hyperf/database 版本中确认 $event->time 是否为毫秒；若为毫秒，当前用 microtime(true) - $event->time / 1000 推导为秒的逻辑是正确的。(github.com)
建议最小改动（保持行为最小化）：
@@ - foreach ($event->bindings as $key => $value) { - $data['db.parameter.' . $key] = $value; - } @@ - if ($this->switcher->isTracingExtraTagEnable('db.sql.bindings', true)) { + if ($this->switcher->isTracingExtraTagEnable('db.sql.bindings', false)) { $data += ['db.sql.bindings' => $event->bindings]; }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
203-212: 确认 Hyperf\QueryExecuted::time 单位并添加注释Hyperf\QueryExecuted::$time 单位为毫秒；请保留除以 1000 转为秒的写法，并在 startTimestamp 与 finish 调用处添加注释说明，确保后续维护者明确单位。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sentry/src/Tracing/Aspect/DbAspect.php(1 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sentry/src/Tracing/Aspect/DbAspect.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (3)
src/sentry/src/Switcher.php (2)
isTracingExtraTagEnable(53-56)isTracingSpanEnable(44-51)src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-52)src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
🪛 PHPMD (2.15.0)
src/sentry/src/Tracing/Listener/EventHandleListener.php
235-235: Avoid unused parameters such as '$event'. (undefined)
(UnusedFormalParameter)
249-249: Avoid unused parameters such as '$event'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (4)
src/sentry/src/Tracing/Listener/EventHandleListener.php (4)
29-29: 统一使用 DbEvent 并注册事务事件：方向正确
- 引入
use Hyperf\Database\Events as DbEvent;并在 listen()/process() 中切换为 DbEvent 映射，覆盖了 BEGIN/COMMIT/ROLLBACK，事件矩阵更完整、清晰。Also applies to: 76-80, 121-125
164-172:handleDbQueryExecuted签名切换到 DbEvent：对齐生态
- 将参数类型改为
DbEvent\QueryExecuted与上方事件映射一致，避免歧义。
326-333: finish() 改为无参 + 明确顺序注释：一致性更好
- 延迟回调先结束 span，再恢复 transaction 并 finish；其它场景也统一
finish()无参，符合本次 PR 的一致性目标。若希望我补一组回归用例（覆盖：事务嵌套、跨协程、DB/Redis/Kafka/AMQP/AsyncQueue 的 finish 顺序），请告知。
Also applies to: 434-435, 540-541, 618-619, 690-691, 749-750
214-233: 事务 span 引用丢失：COMMIT/ROLLBACK 可能 finish 到错误的 span（支持嵌套，按连接名入栈/出栈）
- BEGIN 未存放事务 span 引用；COMMIT/ROLLBACK 通过
SentrySdk::getCurrentHub()->getSpan()取得的是“当前”span，可能是其它 DB/业务 span，导致误设状态/误 finish。- 建议使用 Context 以连接名维护 stack，BEGIN push，COMMIT/ROLLBACK pop，对应 finish；天然适配嵌套事务。
- 同时可消除静态分析“未使用参数 $event”的告警。
@@ - private function handleDbTransactionBeginning(DbEvent\TransactionBeginning $event): void + private function handleDbTransactionBeginning(DbEvent\TransactionBeginning $event): void { if (! $this->switcher->isTracingSpanEnable('sql_transactions')) { return; } if (! SentrySdk::getCurrentHub()->getSpan()) { return; } - - $this->startSpan( + $span = $this->startSpan( op: 'db.transaction', description: 'BEGIN', origin: 'auto.db' - )->setData([ + ); + $span?->setData([ 'coroutine.id' => Coroutine::id(), 'db.system' => $event->connection->getDriverName(), 'db.name' => $event->connection->getDatabaseName(), 'db.pool.name' => $event->connectionName, ]); + if ($span) { + $key = __CLASS__ . '.db.tx.' . $event->connectionName; + $stack = (array) Context::get($key, []); + $stack[] = $span; + Context::set($key, $stack); + } } @@ - private function handleDbTransactionCommitted(DbEvent\TransactionCommitted $event): void + private function handleDbTransactionCommitted(DbEvent\TransactionCommitted $event): void { if (! $this->switcher->isTracingSpanEnable('sql_transactions')) { return; } - if (! $span = SentrySdk::getCurrentHub()->getSpan()) { - return; - } + $key = __CLASS__ . '.db.tx.' . $event->connectionName; + $stack = (array) Context::get($key, []); + $span = array_pop($stack); + Context::set($key, $stack); + if (! $span) return; $span->setStatus(SpanStatus::ok()) ->setDescription('COMMIT') ->finish(); } @@ - private function handleDbTransactionRolledBack(DbEvent\TransactionRolledBack $event): void + private function handleDbTransactionRolledBack(DbEvent\TransactionRolledBack $event): void { if (! $this->switcher->isTracingSpanEnable('sql_transactions')) { return; } - if (! $span = SentrySdk::getCurrentHub()->getSpan()) { - return; - } + $key = __CLASS__ . '.db.tx.' . $event->connectionName; + $stack = (array) Context::get($key, []); + $span = array_pop($stack); + Context::set($key, $stack); + if (! $span) return; $span->setStatus(SpanStatus::internalError()) ->setDescription('ROLLBACK') ->finish(); }Also applies to: 235-247, 249-261
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
125-127: rpc.context 可能包含敏感信息，应受开关控制。当前无条件写入
$span的rpc.context，可能泄露鉴权上下文/PII。- if ($this->container->has(Rpc\Context::class)) { - $span?->setData(['rpc.context' => $this->container->get(Rpc\Context::class)->getData()]); - } + if ($this->container->has(Rpc\Context::class) + && $this->switcher->isTracingExtraTagEnable('rpc.context')) { + $span?->setData(['rpc.context' => $this->container->get(Rpc\Context::class)->getData()]); + }
🧹 Nitpick comments (6)
src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
101-101: 统一 op 名称为 queue.publish 很好；建议补充批量规模数据。为批量发布补充
messaging.batch.message.count有助于聚合与排障。if (! $span) { return $proceedingJoinPoint->process(); } + // 记录批量大小 + $span->setData(['messaging.batch.message.count' => count($messages)]); + foreach ($messages as $message) {src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (2)
73-89: 建议：无新 span 时仍传播父级 trace（分布式追踪更稳健）。若未创建 span，但存在父级（Hub 当前 span），可用父级头部继续传播。
use Psr\Http\Message\RequestInterface; use Sentry\Tracing\SpanStatus; use Throwable; +use Sentry\SentrySdk; @@ - // Inject trace context && Start span + // Start span（如未采样可能返回 null） $span = $this->startSpan( @@ - if (! $span) { - return $proceedingJoinPoint->process(); - } + if (! $span) { + // 无新 span，尝试用父级继续传播 + if ($parent = SentrySdk::getCurrentHub()->getSpan()) { + $options['headers'] = array_replace($options['headers'] ?? [], [ + 'sentry-trace' => $parent->toTraceparent(), + 'baggage' => $parent->toBaggage(), + 'traceparent' => $parent->toW3CTraceparent(), + ]); + $proceedingJoinPoint->arguments['keys']['options']['headers'] = $options['headers']; + } + return $proceedingJoinPoint->process(); + }
103-104: 健壮性：计算请求体大小时处理 Stream/资源类型。
$options['body']可能是 PSR-7 Stream/资源，strlen()会报错或造成大内存复制。- 'http.request.body.size' => strlen($options['body'] ?? ''), + 'http.request.body.size' => (function ($body) { + if (is_string($body)) return strlen($body); + if ($body instanceof \Psr\Http\Message\StreamInterface) return $body->getSize(); + return null; + })($options['body'] ?? null),src/sentry/src/Listener/EventHandleListener.php (2)
115-119: 事件映射不一致：监听了 FailToHandle，但未在 process() 中处理。要么从
listen()移除，要么在process()中补齐分支（建议复用handleCommandFinished以统一收尾）。// Command events CommandEvent\BeforeHandle::class, - CommandEvent\FailToHandle::class, CommandEvent\AfterExecute::class,或：
// Command events CommandEvent\BeforeHandle::class => $this->handleCommandStarting($event), + CommandEvent\FailToHandle::class => $this->handleCommandFinished($event), CommandEvent\AfterExecute::class => $this->handleCommandFinished($event),Also applies to: 165-167
423-440: 优先使用公开 API；无则回退到属性读取，并为 command input 上报加开关保护经检索 vendor/hyperf/command/src/Command.php，Hyperf\Command 未提供 getExitCode()/getInput() 公共方法，当前通过闭包反射读取 protected 属性是必要。建议改为：优先调用公开方法（若存在），否则回退到闭包访问；并在上报 input 前用现有开关控制是否包含 input（防止泄露）。位置：src/sentry/src/Listener/EventHandleListener.php — handleCommandFinished()
- Integration::addBreadcrumb(new Breadcrumb( + $command = $event->getCommand(); + $exitCode = method_exists($command, 'getExitCode') + ? $command->getExitCode() + : (fn () => $this->exitCode ?? null)->call($command); + $data = ['exit' => $exitCode]; + if ($this->switcher->isEnable('command_input')) { + $data['input'] = method_exists($command, 'getInput') + ? $command->getInput() + : $this->extractConsoleCommandInput((fn () => $this->input)->call($command)); + } + Integration::addBreadcrumb(new Breadcrumb( Breadcrumb::LEVEL_INFO, Breadcrumb::TYPE_DEFAULT, 'command', 'Finished command: ' . $event->getCommand()->getName(), - [ - 'exit' => (fn () => $this->exitCode)->call($event->getCommand()), - 'input' => $this->extractConsoleCommandInput((fn () => $this->input)->call($event->getCommand())), - ] + $data ));src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
65-66: 过早修改请求参数在创建 span 之前就修改了
$proceedingJoinPoint的参数，这与其他 Aspect（如 GuzzleHttpClientAspect）的实践不一致。建议在确认 span 创建成功后再注入追踪头。将追踪头注入移到 span 创建之后：
- // Inject tracing headers - $proceedingJoinPoint->arguments['keys']['options'] = $options; - // Start gRPC client span $span = $this->startSpan( op: 'grpc.client', description: $method, origin: 'auto.grpc', )?->setData($data); + + // Inject tracing headers only if span is created + if ($span) { + $proceedingJoinPoint->arguments['keys']['options'] = $options; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/sentry/src/Listener/EventHandleListener.php(3 hunks)src/sentry/src/Tracing/Aspect/CoroutineAspect.php(3 hunks)src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/GrpcAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/RedisAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/RpcAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php(3 hunks)
💤 Files with no reviewable changes (1)
- src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php
✅ Files skipped from review due to trivial changes (1)
- src/sentry/src/Tracing/Aspect/CoroutineAspect.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/sentry/src/Listener/EventHandleListener.php (1)
src/sentry/src/Integration.php (2)
Integration(28-158)configureScope(79-88)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
process(37-112)
src/sentry/src/Tracing/Aspect/GrpcAspect.php (3)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
process(46-184)src/sentry/src/Switcher.php (1)
isTracingExtraTagEnable(53-56)
⏰ 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 5.1.7
- GitHub Check: Test on PHP 8.2 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.3 with Swoole 5.1.7
- 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.1 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.0.2
🔇 Additional comments (9)
src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
136-137: 在 try 内早返回，语义更清晰。
finally仍会执行，$span?->finish()不会遗漏。src/sentry/src/Tracing/Aspect/RedisAspect.php (2)
93-98: 使用 null-safe 写入与在 try 内早返回，稳健性提升。避免空 span 访问与重复 return，改动合理。
106-108: 异常栈可选记录处理到位。使用
?->防止空指针，符合其它 Aspect 的一致性。src/sentry/src/Listener/EventHandleListener.php (1)
403-412: 谨慎：只对 ArgvInput 返回字符串，默认不要记录命令行参数extractConsoleCommandInput 目前只在 ArgvInput 返回可序列化字符串，其他 Input 实现返回 null — 这是保守且安全的做法。已在 vendor 中检索：未发现 Hyperf 的命令事件暴露类似 getInput() 的通用接口（事件只携带 Command 对象），因此调用处必须做好开关与脱敏控制。建议动作：
- 保持默认“不记录”或通过配置开关显式开启记录。
- 若要记录：只记录白名单参数并对可能的敏感字段（password/token/secret/key 等）进行脱敏或屏蔽；限制记录长度与存储速率。
- 请检查并确认项目中实际的命令实现（vendor/hyperf/command/src/Command.php 及 app/Command 下的自定义命令）如何通过 $this->input / argument() / option() 暴露参数，并在这些位置加入脱敏/白名单逻辑或在事件监听处跳过记录。
src/sentry/src/Tracing/Aspect/GrpcAspect.php (5)
43-49: 提取方法变量提高代码可读性将方法名提取到变量中是一个好的做法，让代码更清晰。
53-56: 添加了必要的 span 采样检查这是一个重要的改进，在没有父 span 或未被采样时跳过追踪，避免了不必要的开销和潜在的空指针问题。
81-91: 使用了空安全操作符改进代码健壮性正确使用了
?->操作符来防止空指针异常，这是一个很好的防御性编程实践。
93-93: 添加了明确的返回语句在 try 块中添加显式的 return 语句使控制流更清晰，这是一个好的改进。
95-106: 异常处理中正确使用了空安全操作符在异常处理路径中也使用了
?->操作符，保持了代码的一致性和健壮性。
… and command events
…Aspect and add nullsafe operator in EventHandleListener
…uzzleHttpClientAspect
…eprecate old methods fix(aspects): update tracing span enable methods for consistency across all aspects fix(listener): standardize tracing enable checks in EventHandleListener for various events
…ptionIgnored with documentation reference
huangdijia
added a commit
that referenced
this pull request
Sep 20, 2025
…914) * refactor(sentry): improve span lifecycle management and code clarity - Remove manual timestamp passing to span.finish() method, letting Sentry handle timing automatically - Improve span parent/child relationship management in TraceAnnotationAspect - Clean up commented code and improve readability in DbAspect - Simplify transaction creation flow in SpanStarter with better structure - Add proper comments for span restoration and transaction finishing - Ensure consistent span finishing patterns across all aspects and listeners This refactoring improves the reliability of span timing and makes the code more maintainable. * feat(sentry): enhance database event handling with transaction support and SQL binding options * feat(sentry): add conditional handling for SQL bindings in DbAspect and EventHandleListener * fix(sentry): correct data assignment for SQL bindings in DbAspect * fix(sentry): remove redundant line in SQL bindings data assignment in DbAspect * fix(sentry): improve conditional checks and data assignment in tracing aspects * fix(sentry): inject additional trace context headers in GuzzleHttpClientAspect * fix(sentry): rename span name for batch sending in KafkaProducerAspect * fix(sentry): simplify span result handling in RedisAspect * fix(sentry): ensure result is returned in RpcAspect processing * fix(sentry): enhance command event handling with breadcrumbs and scope configuration * fix(sentry): add configuration options for breadcrumbs in async queue and command events * fix(sentry): improve header injection for distributed tracing in GrpcAspect and add nullsafe operator in EventHandleListener * fix(sentry): ensure span is valid before injecting trace context in GuzzleHttpClientAspect * fix(sentry): enhance command breadcrumb logging with input and exit code details * fix(sentry): add command input option for breadcrumbs configuration * fix(switcher): rename breadcrumb enable methods for consistency and deprecate old methods fix(aspects): update tracing span enable methods for consistency across all aspects fix(listener): standardize tracing enable checks in EventHandleListener for various events * fix(switcher): deprecate isExceptionIgnored method in favor of isExceptionIgnored with documentation reference --------- Co-Authored-By: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
This was referenced Sep 22, 2025
This was referenced Sep 25, 2025
This was referenced Oct 12, 2025
This was referenced Nov 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the Sentry tracing implementation to improve span lifecycle management and code clarity, along with additional enhancements from linter improvements.
Key Changes
span.finish()method, allowing Sentry to handle timing automaticallyTraceAnnotationAspectSpanStarterwith better structureFiles Modified
src/sentry/src/Tracing/Aspect/CoordinatorAspect.phpsrc/sentry/src/Tracing/Aspect/DbAspect.phpsrc/sentry/src/Tracing/Aspect/TraceAnnotationAspect.phpsrc/sentry/src/Tracing/Listener/EventHandleListener.phpsrc/sentry/src/Tracing/SpanStarter.phpKey Improvements in EventHandleListener
Benefits
Test Plan