refactor(sentry): standardize message ID generation using SentryUid in tracing aspects #943
Conversation
…n tracing aspects Replace uniqid() calls with SentryUid::generate() for consistent message ID generation across AMQP, AsyncQueue, and Kafka producer aspects. This ensures better integration with Sentry's internal ID generation patterns.
Walkthrough将 AMQP、Kafka 以及异步队列消息的 messageId 生成方式统一切换为使用 Sentry\Util\SentryUid::generate()，相应文件新增了 SentryUid 的 use 引用，除 ID 生成路径外未改动其他逻辑、数据字段或控制流。 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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. Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
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/KafkaProducerAspect.php (1)
107-116: sendBatchAsync：逐条消息的 message_id 也应字符串化与单发保持一致，降低 JSON/headers 序列化不确定性。
请修改为：
- 'message_id' => SentryUid::generate(), + 'message_id' => (string) SentryUid::generate(),
🧹 Nitpick comments (1)
src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
56-94: 行为变更提示：去除 kafka_ 前缀的潜在兼容性影响此前 uniqid('kafka_', true) 产出的带前缀 ID 可能被日志解析、监控告警或临时脚本所依赖；改为 SentryUid 可能影响检索规则与可观测面板。请在发布说明中标注并在测试计划中验证相关告警/仪表盘。
可选：抽取一个私有方法 generateMessageId(): string 以集中控制格式，后续如需再调整仅改一处。
Also applies to: 96-128
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php(3 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/AsyncQueueJobMessageAspect.phpsrc/sentry/src/Tracing/Aspect/AmqpProducerAspect.phpsrc/sentry/src/Tracing/Aspect/KafkaProducerAspect.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/AsyncQueueJobMessageAspect.phpsrc/sentry/src/Tracing/Aspect/AmqpProducerAspect.phpsrc/sentry/src/Tracing/Aspect/KafkaProducerAspect.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/AsyncQueueJobMessageAspect.phpsrc/sentry/src/Tracing/Aspect/AmqpProducerAspect.phpsrc/sentry/src/Tracing/Aspect/KafkaProducerAspect.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). (12)
- 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.1 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 6.0.2
- 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.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 (6)
src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php (3)
23-23: 引入 SentryUid 符合本次重构方向命名空间与 PSR-12 用法正确，无意见。
63-71: 补充验证：push 场景中 job 恒存在与类型安全该分支假设 push 时一定有 $job；若存在自定义驱动/调用缺省 job 的边缘场景，$job::class 与序列化长度计算可能出错。建议在本 PR 的测试计划中覆盖“缺省/异常 job”输入，确保与 Hyperf 行为一致。
Also applies to: 81-89, 95-114
80-80: 强制字符串化 messageId
将getId()与SentryUid::generate()的结果统一转为字符串：- $messageId = method_exists($job, 'getId') ? $job->getId() : SentryUid::generate(); + $messageId = method_exists($job, 'getId') + ? (string) $job->getId() + : (string) SentryUid::generate();避免 JSON 序列化时出现类型不一致或空对象问题。
src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
23-23: 引入 SentryUid OK与既有 use 分组/风格一致。
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php (2)
25-25: 引入 SentryUid OK风格与分组正确，符合组件命名空间约定（FriendsOfHyperf\Sentry）。基于编码规范。
81-115: 行为变更提示：去除 amqp_ 前缀的潜在兼容性影响若有依赖该前缀进行日志检索或路由调试，请同步更新相关规则；建议在测试计划中包含一次生产者链路的端到端验证（含消费端可见的 headers）。
| } | ||
|
|
||
| $messageId = uniqid('amqp_', true); | ||
| $messageId = SentryUid::generate(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
AMQP：显式字符串化 message_id
为避免 headers/Span data 中的类型不一致与潜在序列化问题，建议强转为 string。
请修改为：
- $messageId = SentryUid::generate();
+ $messageId = (string) SentryUid::generate();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $messageId = SentryUid::generate(); | |
| $messageId = (string) SentryUid::generate(); |
🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php around line 77, the
generated message ID is assigned without guaranteeing a string type; explicitly
cast the result of SentryUid::generate() to a string (e.g., (string)
SentryUid::generate()) and ensure any place that sets the AMQP header message_id
uses that string value so headers/Span data remain consistently string-typed for
serialization.
| protected function sendAsync(ProceedingJoinPoint $proceedingJoinPoint) | ||
| { | ||
| $messageId = uniqid('kafka_', true); | ||
| $messageId = SentryUid::generate(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
sendAsync：统一字符串化 message_id
避免类型/序列化差异，建议显式 (string)。
请修改为：
- $messageId = SentryUid::generate();
+ $messageId = (string) SentryUid::generate();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $messageId = SentryUid::generate(); | |
| $messageId = (string) SentryUid::generate(); |
🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php around line 58, the
generated message ID is assigned without ensuring it is a string; update the
assignment to explicitly cast the result of SentryUid::generate() to string so
message_id is always a string (e.g., $messageId = (string)
SentryUid::generate();), ensuring consistent typing/serialization for sendAsync.
huangdijia
deleted the
refactor/sentry-use-sentryuid-for-message-ids
branch
…n tracing aspects (#943) Replace uniqid() calls with SentryUid::generate() for consistent message ID generation across AMQP, AsyncQueue, and Kafka producer aspects. This ensures better integration with Sentry's internal ID generation patterns. Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
uniqid()calls withSentryUid::generate()in AMQP, AsyncQueue, and Kafka producer aspectsChanges Made
SentryUid::generate()SentryUid::generate()sendAsync()and batch methods to useSentryUid::generate()Test Plan
Summary by CodeRabbit