-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat: add feature flag support for tracing aspects #1029
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
feat: add feature flag support for tracing aspects #1029
Conversation
Add Feature dependency injection to tracing aspects to allow conditional span creation based on feature flags. This enables fine-grained control over which types of operations are traced (db, redis, elasticsearch, rpc). Changes: - DbConnectionAspect: Check db tracing feature flag before caching server info - RedisConnectionAspect: Check redis tracing feature flag before caching slot info - ElasticsearchRequestAspect: Check elasticsearch tracing feature flag before processing - RpcEndpointAspect: Check rpc tracing feature flag before storing server address This allows users to selectively enable/disable tracing for specific operation types without modifying code.
概述针对四个追踪方面类(DbConnection、Elasticsearch、Redis、RpcEndpoint)进行了一致的重构,通过构造函数促进添加 Feature 依赖注入,并在各自的 变更
代码审查工作量估算🎯 2 (简单) | ⏱️ ~10 分钟 需要重点关注的区域:
相关 PR
建议审阅人
诗歌
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.32)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 |
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 (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
41-45: 基于 tracing span 的 Redis 开关逻辑合理,静态分析的$result告警可视为噪音
- 先执行
$proceedingJoinPoint->process(),再在 tap 闭包里使用Feature::isTracingSpanEnabled('redis')做短路,只影响上下文填充,不会改变原方法返回值,这个位置选择是安全且清晰的。- 闭包形参
$result当前未使用,但这是tap模式下很常见的写法,用于匹配回调签名。PHPMD 提示“UnusedFormalParameter”在这里属于可接受的告警,如果后续不打算在这里使用 Redis 调用结果,可以选择忽略该告警,或在静态分析配置里对这类模式放宽规则。总体而言,这段开关逻辑实现是正确且易于维护的。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/ElasticsearchRequestAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/RpcEndpointAspect.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/sentry/src/Tracing/Aspect/ElasticsearchRequestAspect.php (3)
src/sentry/src/Feature.php (2)
Feature(18-75)isTracingSpanEnabled(39-46)src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (2)
__construct(56-58)process(60-89)src/sentry/src/Function.php (1)
feature(24-27)
src/sentry/src/Tracing/Aspect/RpcEndpointAspect.php (3)
src/sentry/src/Feature.php (2)
Feature(18-75)isTracingSpanEnabled(39-46)src/sentry/src/Tracing/Aspect/RpcAspect.php (2)
__construct(43-47)process(49-60)src/sentry/src/Function.php (1)
feature(24-27)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (5)
src/sentry/src/Feature.php (2)
Feature(18-75)isTracingSpanEnabled(39-46)src/sentry/src/Tracing/Aspect/ElasticsearchRequestAspect.php (2)
__construct(30-32)process(34-66)src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (2)
__construct(34-37)process(39-66)src/sentry/src/Tracing/Aspect/RpcEndpointAspect.php (2)
__construct(30-32)process(34-65)src/sentry/src/Function.php (1)
feature(24-27)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (3)
src/sentry/src/Feature.php (2)
Feature(18-75)isTracingSpanEnabled(39-46)src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (2)
__construct(33-36)process(38-67)src/sentry/src/Function.php (1)
feature(24-27)
🪛 PHPMD (2.15.0)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php
41-41: Avoid unused parameters such as '$result'. (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). (18)
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
🔇 Additional comments (10)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (2)
15-15: 引入 Feature 依赖用于 Redis tracing 开关是合理的这里新增对
Feature的引用,与DbConnectionAspect等保持一致,有利于统一通过配置开关控制各类 tracing span,没看到问题。
34-37: 构造函数新增 Feature 依赖,确认是否存在手动实例化场景通过构造函数注入
Feature是很自然的依赖注入方式,同时仍在构造函数里初始化WeakMap,逻辑正确。不过这会改变构造签名,如果项目中有地方手动new RedisConnectionAspect()(而不是交给容器创建),会出现 BC 问题,建议确认一下。src/sentry/src/Tracing/Aspect/RpcEndpointAspect.php (3)
15-15: LGTM! 导入语句正确Feature 类的导入是必要的,且位置适当。
30-32: LGTM! 依赖注入实现正确构造函数使用属性提升语法注入 Feature 依赖,与 RpcAspect 的实现模式保持一致。
37-39: LGTM! 特性开关逻辑正确特性标志检查正确放置在
tap回调内部,在获取结果之后、设置上下文之前进行判断。当rpc特性被禁用时,及时返回避免设置追踪上下文,同时不影响原始方法调用的结果返回。这与 RpcAspect 中的特性开关模式保持一致。src/sentry/src/Tracing/Aspect/ElasticsearchRequestAspect.php (2)
30-32: 依赖注入实现正确。构造函数正确地注入了
Feature依赖,与ElasticsearchAspect中的模式保持一致。
36-38: 特性标志检查实现正确,无需修改。验证结果显示
ElasticsearchAspect.php也在同一行(第 62-64 行)执行了相同的特性标志检查。两个 Aspect 都遵循一致的模式:检查isTracingSpanEnabled('elasticsearch')标志,若禁用则提前返回。这确保了当ElasticsearchAspect.php在第 75 行调用Context::get(Constants::TRACE_ELASTICSEARCH_REQUEST_DATA, [])时,特性必然已启用,且ElasticsearchRequestAspect已设置了上下文数据。默认空数组提供了额外的防御性保护。src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (3)
15-15: 导入语句正确。添加 Feature 类的导入以支持构造函数依赖注入。
33-36: 依赖注入实现正确。通过构造函数提升属性注入 Feature 依赖,与其他追踪切面(RedisConnectionAspect、RpcEndpointAspect、ElasticsearchRequestAspect)的模式保持一致。WeakMap 初始化保持不变。
41-43: 功能开关检查实现正确,位置合理。在 tap 回调内部检查功能开关是正确的设计:
- 数据库操作始终执行(第 40 行),不受追踪开关影响
- 功能开关仅控制是否收集追踪元数据(缓存和上下文设置)
- 与 RedisConnectionAspect 和 RpcEndpointAspect 的模式保持一致
当功能禁用时,跳过服务器信息缓存(第 45-54 行)和上下文设置(第 56-65 行),有效减少追踪开销。
Add Feature dependency injection to tracing aspects to allow conditional span creation based on feature flags. This enables fine-grained control over which types of operations are traced (db, redis, elasticsearch, rpc). Changes: - DbConnectionAspect: Check db tracing feature flag before caching server info - RedisConnectionAspect: Check redis tracing feature flag before caching slot info - ElasticsearchRequestAspect: Check elasticsearch tracing feature flag before processing - RpcEndpointAspect: Check rpc tracing feature flag before storing server address This allows users to selectively enable/disable tracing for specific operation types without modifying code. Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
This PR adds feature flag support to tracing aspects, enabling fine-grained control over which types of operations are traced. Each aspect now checks the corresponding feature flag before performing tracing operations.
Changes
Featuredependency injection and check fordbtracing feature flag before caching server informationFeaturedependency injection and check forredistracing feature flag before caching slot/node informationFeaturedependency injection and check forelasticsearchtracing feature flag before processing requestsFeaturedependency injection and check forrpctracing feature flag before storing server addressBenefits
Test Plan
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.