-
-
Notifications
You must be signed in to change notification settings - Fork 27
fix(tracing): use routing key for AMQP destination name instead of exchange #903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…change - Update AmqpProducerAspect to use routing key for destination name - Update TracingAmqpListener to use routing key for destination name - Improves tracing accuracy by using more specific routing information
Walkthrough将 AMQP 追踪中 messaging.destination.name 的取值从基于 exchange/默认值,调整为基于 routingKey(标量直取,数组使用逗号连接,空为"")。改动同时应用于消息生产切面与监听器的事务结束逻辑,其他 span 与 AMQP 字段保持不变。 Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
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.17)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.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Listener/TracingAmqpListener.php (1)
112-112: routingKey 作为 destination.name 的回退实现 OK,但建议过滤空值并确认空字符串是否符合观测规范当前
(array)$message->getRoutingKey()在null时会变成空数组,implode返回空字符串。这与 PR 目标一致,但可能在可观测维度上产生“空值”标签,影响聚合/看板。建议过滤null/空串,并确认空字符串是否被 Sentry 接受且不会造成查询困扰。可在一行内完成过滤与字符串化:
- 'messaging.destination.name' => $carrier?->get('destination_name') ?: implode(', ', (array) $message->getRoutingKey()), + 'messaging.destination.name' => $carrier?->get('destination_name') + ?: implode(', ', array_map('strval', array_filter((array) $message->getRoutingKey(), static fn($v) => $v !== null && $v !== ''))),需要你验证:
- 目标环境中是否允许
messaging.destination.name为空字符串;若不允许,是否应改为占位值如"(empty)"。- 多 routingKey 场景使用
", "连接是否会引入高基数字段,影响事件卡片聚合。src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php (1)
88-88: destination_name 从 exchange 切换为 routingKey:建议过滤空值并标准化输出,避免携带空元素为稳健起见,过滤
null/空串并统一字符串化,避免出现", , "或"0"/false等非预期片段;同时与 Listener 的实现保持一致。- $destinationName = implode(', ', (array) $routingKey); + $destinationName = implode( + ', ', + array_map('strval', array_filter((array) $routingKey, static fn($v) => $v !== null && $v !== '')) + );请同时确认:
- 去除 exchange 作为回退是否需要在变更日志中标注潜在看板维度变更(基数可能上升,且维度名称值集合变化)。
- 生产端写入的
destination_name与消费端回读并回退到routingKey的逻辑在所有路径下保持一致(含注解覆写与多 key)。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php(1 hunks)src/sentry/src/Tracing/Listener/TracingAmqpListener.php(1 hunks)
⏰ 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.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.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.1 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 5.1.7
…change (#903) - Update AmqpProducerAspect to use routing key for destination name - Update TracingAmqpListener to use routing key for destination name - Improves tracing accuracy by using more specific routing information Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
Test plan
Summary by CodeRabbit