refactor(sentry): centralize coroutine ID handling in tracing system #921
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 duplicate coroutine.id data from individual tracing aspects - Centralize coroutine ID handling in Tracer class for consistency - Add default transaction metadata (status, source, start timestamp) in Tracer - Remove unused Coroutine imports from aspect files - Improve code maintainability by reducing duplication across aspects This change ensures coroutine IDs are consistently added to all transactions and spans through the central Tracer class rather than duplicating the logic in each individual aspect.
Walkthrough本次变更在多数 Tracing Aspect 中去除对 Hyperf 协程 ID（coroutine.id）的采集；同时在 Tracer 中为事务与 trace 上下文统一注入 coroutine.id，并为事务设置缺省 startTimestamp、status、source。事件监听器清理了多个事件中对 coroutine.id 与部分 TransactionSource 的设置。Kafka 批量发布增加 message.count 字段。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as 调用方
participant Tracer as Tracer
participant Sdk as Sentry SDK
rect rgba(230,245,255,0.6)
Note over Tracer: 事务启动时设置默认值与上下文
Caller->>Tracer: startTransaction(context?)
Tracer->>Tracer: 若缺失：source=custom, status=ok, startTimestamp=now
Tracer->>Tracer: context.data += { coroutine.id }
Tracer->>Sdk: Sdk.startTransaction(context)
Sdk-->>Tracer: Transaction
Tracer-->>Caller: Transaction
end
rect rgba(240,255,240,0.6)
Note over Tracer: trace 执行包装统一注入 coroutine.id
Caller->>Tracer: trace(path, fn, context?)
Tracer->>Tracer: context.data += { coroutine.id }
Tracer->>Sdk: Sdk.startSpan(context)
Sdk-->>Tracer: Span
Tracer->>Tracer: 执行 fn()
Tracer->>Sdk: Span.finish()
Tracer-->>Caller: 返回 fn() 结果
end
Note over Caller,Sdk: 各具体 Aspect 不再各自写入 coroutine.id
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
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 |
huangdijia
added a commit
that referenced
this pull request
Sep 25, 2025
…921) - Remove duplicate coroutine.id data from individual tracing aspects - Centralize coroutine ID handling in Tracer class for consistency - Add default transaction metadata (status, source, start timestamp) in Tracer - Remove unused Coroutine imports from aspect files - Improve code maintainability by reducing duplication across aspects This change ensures coroutine IDs are consistently added to all transactions and spans through the central Tracer class rather than duplicating the logic in each individual aspect. Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
![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
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (1)
47-52: 可选：为 annotation.arguments 增加开关，避免不必要的负载或敏感数据泄露当前无条件记录所有参数，建议与 annotation.result 一致，受开关控制。
可按下述方式包裹（示例仅展示该段落变更）：
- if (in_array($methodName, ['__call', '__callStatic'])) { - $methodName = $proceedingJoinPoint->arguments['keys']['name'] ?? $proceedingJoinPoint->methodName; - $data['annotation.arguments'] = $proceedingJoinPoint->arguments['keys']['arguments'] ?? $proceedingJoinPoint->arguments['keys']; - } else { - $data['annotation.arguments'] = $proceedingJoinPoint->arguments['keys']; - } + if (in_array($methodName, ['__call', '__callStatic'])) { + $methodName = $proceedingJoinPoint->arguments['keys']['name'] ?? $proceedingJoinPoint->methodName; + if ($this->switcher->isTracingExtraTagEnabled('annotation.arguments')) { + $data['annotation.arguments'] = $proceedingJoinPoint->arguments['keys']['arguments'] ?? $proceedingJoinPoint->arguments['keys']; + } + } else { + if ($this->switcher->isTracingExtraTagEnabled('annotation.arguments')) { + $data['annotation.arguments'] = $proceedingJoinPoint->arguments['keys']; + } + }src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
125-126: 批量发布仅设置 message.count，建议补充基础语义字段以对齐单条发布为便于统一查询/看板，建议与 sendAsync 的数据结构保持一致（system/operation），同时保留 message.count。
可考虑：
- ->setData(['message.count' => count($messages)]) + ->setData([ + 'messaging.system' => 'kafka', + 'messaging.operation' => 'publish', + 'message.count' => count($messages), + ])如业务保证同批消息同一 topic，可一并添加 messaging.destination.name；否则可省略以避免歧义。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/CacheAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/CoordinatorAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/CoroutineAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/DbAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/FilesystemAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/GrpcAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/RedisAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/RpcAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php(1 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(0 hunks)src/sentry/src/Tracing/Tracer.php(3 hunks)
💤 Files with no reviewable changes (12)
- src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php
- src/sentry/src/Tracing/Aspect/RedisAspect.php
- src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php
- src/sentry/src/Tracing/Aspect/CacheAspect.php
- src/sentry/src/Tracing/Listener/EventHandleListener.php
- src/sentry/src/Tracing/Aspect/RpcAspect.php
- src/sentry/src/Tracing/Aspect/CoordinatorAspect.php
- src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
- src/sentry/src/Tracing/Aspect/DbAspect.php
- src/sentry/src/Tracing/Aspect/CoroutineAspect.php
- src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php
- src/sentry/src/Tracing/Aspect/GrpcAspect.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Tracer.php (1)
src/sentry/class_map/SentrySdk.php (1)
SentrySdk(24-65)
⏰ 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.2 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.3 with Swoole 6.0.2
- 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.2 with Swoole 5.1.7
🔇 Additional comments (7)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (1)
39-39: 去除协程 ID 合并，改用集中注入 —— LGTM保留原始 $data，遵循统一注入策略，改动合理。
src/sentry/src/Tracing/Tracer.php (5)
15-15: 引入 Hyperf\Engine\Coroutine 与 TransactionSource —— LGTM满足集中注入与事务来源默认化的需求。
Also applies to: 23-23
44-45: 在 TransactionContext 上集中注入 coroutine.id —— LGTM使用数组并集避免覆盖既有键，符合预期。
46-53: 为事务设置默认 startTimestamp/status —— LGTM仅在为空时赋值，避免干扰上游显式设置。
86-87: 在 SpanContext 上集中注入 coroutine.id —— LGTM与各 Aspect 的精简相匹配，避免重复。
54-56: 确认 sentry-php SDK 版本是否 ≥ v3.9.0，从 v3.9.0 起可直接使用TransactionContext::setSource()；若使用旧版 (<3.9.0)，请改为通过$transactionContext->getMetadata()->setSource(TransactionSource::custom())设置来源。src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (1)
44-44: 所有 span 调用均通过 FriendsOfHyperf\Sentry\trace() 注入 coroutine.id，未发现直接调用 Sentry\trace()。
This was referenced Sep 28, 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
Tracerclass to improve consistency and reduce code duplicationcoroutine.iddata setting from individual tracing aspectsTracerCoroutineimports from aspect filesChanges
Core Changes
Aspect Files Updated
coroutine.iddata setting from all tracing aspects:AmqpProducerAspect.phpAsyncQueueJobMessageAspect.phpCacheAspect.phpCoordinatorAspect.phpCoroutineAspect.phpDbAspect.phpElasticsearchAspect.phpFilesystemAspect.phpGrpcAspect.phpGuzzleHttpClientAspect.phpKafkaProducerAspect.phpRedisAspect.phpRpcAspect.phpTraceAnnotationAspect.phpEvent Listener Updates
Benefits
Test Plan
Summary by CodeRabbit