refactor(sentry): replace try-finally with defer pattern in CoroutineAspect #938
Conversation
…Aspect Replace the try-finally block with defer functions for span finishing in the CoroutineAspect class. This improves code clarity and ensures proper cleanup of Sentry spans and transactions when coroutines are created. Changes: - Move span finishing logic to defer function at coroutine creation time - Remove try-finally block that was wrapping proceedingJoinPoint.process() - Ensure parent span and transaction are properly finished using defer - Maintain same cleanup behavior with cleaner code structure
Walkthrough移除 CoroutineAspect 在 finally 中将原始事务重新附回的恢复步骤；并在 CoHttpTransport 的 worker 退出路径将睡眠时长由 1 ms 调整为 100 ms，延长退出等待期与通道耗尽时的暂停。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as 调用方
participant A as CoroutineAspect
participant T as Tracing (Span/Transaction)
participant R as 协程执行体
C->>A: 拦截并执行
A->>T: 创建/切换到父 Span
A->>A: 注册 defer 清理（结束父 Span）
A->>R: 创建协程并传入上下文
A-->>C: 返回 proceed() 结果
Note over A,R: 协程结束时触发 defer 执行清理；不再将原始事务重新附回
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 (2)
💤 Files with no reviewable changes (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:
⏰ 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). (7)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php (2)
108-111: 子协程 transaction 的 setSpan 时机与收尾建议
- 在 defer（协程结束时）才
setSpan($coTransaction)意义不大，建议在创建后立刻设置为当前，使子协程内后续 span 正确归属。- 可在创建后补充协程 id 作为数据字段，便于排查。
示例修正：
- defer(function () use ($coTransaction) { - SentrySdk::getCurrentHub()->setSpan($coTransaction); - $coTransaction->finish(); - }); + // 让子协程内新建的 span 正确挂在该 transaction 下 + SentrySdk::getCurrentHub()->setSpan($coTransaction); + // 可选：标注子协程 id + $coTransaction->setData(['coroutine.id' => Co::id()]); + defer(function () use ($coTransaction) { + $coTransaction->finish(); + });请确认
FriendsOfHyperf\Sentry\startTransaction()会将返回的 transaction 设为当前 span；若不会，上述前移 setSpan 更为必要。
114-133: 异常处理与事件刷新
- 目前只在 transaction 上打标，不执行
captureException()，可能错过事件上报；建议捕获后同时上报。- 如在长驻进程中依赖及时发送，考虑在子协程 defer 中调用集成的 flush（如有）。
最小增量修改示例：
try { $callable(); } catch (Throwable $exception) { + // 确保异常事件被上报 + SentrySdk::getCurrentHub()->captureException($exception); $coTransaction->setStatus(SpanStatus::internalError()) @@ throw $exception; }可选：若项目中有
Integration::flushEvents()（或等效Sentry\flush()），可在子协程的 defer 中追加一次 flush，以降低缓冲残留。如需，我可以基于当前仓库实际集成方式补上最小可行的 flush 调用。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php(2 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.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.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.php
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php (2)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)src/sentry/src/Aspect/CoroutineAspect.php (1)
process(37-64)
⏰ 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). (11)
- GitHub Check: Test on PHP 8.1 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.2 with Swoole 6.0.2
- 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.3 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.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
🔇 Additional comments (1)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
90-99: 跨协程上下文传递 Hub：行为已验证
Tracer::startTransaction()内部已经调用$hub->setSpan($transaction)，会将新创建的 transaction 设为当前。- 在调用
startTransaction()之前，仅通过Co::getContextFor()同步协程上下文，不存在对当前 Hub 的依赖。Integration::flushEvents()已在 CoroutineAspect 与 EventHandleListener 中使用，保证协程结束时事件正常上报。
无需恢复对 Hub 的额外传递，若有特殊场景仍可在子协程入口显式执行setSpan($coTransaction)。
huangdijia
deleted the
refactor/sentry-coroutine-aspect-defer-pattern
branch
…Aspect (#938) * refactor(sentry): replace try-finally with defer pattern in CoroutineAspect Replace the try-finally block with defer functions for span finishing in the CoroutineAspect class. This improves code clarity and ensures proper cleanup of Sentry spans and transactions when coroutines are created. Changes: - Move span finishing logic to defer function at coroutine creation time - Remove try-finally block that was wrapping proceedingJoinPoint.process() - Ensure parent span and transaction are properly finished using defer - Maintain same cleanup behavior with cleaner code structure * refactor(sentry): simplify span finishing logic in CoroutineAspect * refactor(sentry): increase sleep duration to reduce busy-waiting in worker watcher * refactor(sentry): move span finishing logic to finally block in CoroutineAspect --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
Changes
proceedingJoinPoint.process()Test plan
Summary by CodeRabbit
Bug Fixes
Refactor