-
-
Notifications
You must be signed in to change notification settings - Fork 27
refactor(sentry): replace Switcher class with Feature class #923
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
- Introduce new Feature class with the same functionality as Switcher - Update all aspect classes and listeners to use Feature instead of Switcher - Convert Switcher to extend Feature for backward compatibility with deprecation warnings - Add appropriate @deprecated annotations for all Switcher methods - Update test to use Feature class instead of Switcher - This change improves code clarity while maintaining backward compatibility Breaking change: Switcher class is deprecated and will be removed in v3.2
Walkthrough将所有依赖 Switcher 的类与测试替换为依赖新引入的 Feature 类。新增 Feature 并调整 Switcher 继承自 Feature(标记为弃用)。更新各 Aspect、Listener、Tracer 中的构造函数与方法调用为 Feature 风格的开关检查;测试同步更新。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant A as Aspect/Listener/Tracer
participant F as Feature
participant C as ConfigInterface
participant S as SentrySdk
A->>F: isEnabled/isBreadcrumbEnabled/isTracing* (key)
F->>C: 读取配置项(sentry.*)
Note right of F: 若为 isTracingSpanEnabled<br/>检查是否存在活跃 Span
F->>S: 获取当前 Hub/Span(仅 tracing 场景)
F-->>A: 返回布尔结果
alt 结果为真
A->>A: 执行既有逻辑(记录面包屑/启动Span/附加标签)
else 结果为假
A-->>A: 跳过与 Sentry 相关处理
end
Estimated code review effort🎯 3 (中等) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewersPoem
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 (24)
🚧 Files skipped from review as they are similar to previous changes (11)
🧰 Additional context used🧬 Code graph analysis (13)src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php (2)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
src/sentry/src/Aspect/FilesystemAspect.php (1)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (1)
src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (1)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
tests/Sentry/FeatureTest.php (1)
src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
src/sentry/src/Aspect/LoggerAspect.php (4)
🪛 PHPMD (2.15.0)src/sentry/src/Aspect/LoggerAspect.php41-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). (11)
🔇 Additional comments (23)
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 |
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/sentry/src/Aspect/GuzzleHttpClientAspect.php (1)
62-80: 敏感信息泄露风险:记录了完整 URL 与原始请求/响应头
- http.request.full_url 包含原始 query,可能携带 token/secret。
- http.request.headers、http.response.headers 未脱敏,可能泄露 Authorization/Cookie 等敏感头。
建议最小化修复,脱敏敏感头并移除 URL query:
- $uri = $request->getUri()->__toString(); - $data = [ + $uriWithoutQuery = (string) $request->getUri()->withQuery(''); + $sensitiveHeaders = ['authorization', 'proxy-authorization', 'cookie', 'set-cookie', 'x-api-key', 'x-auth-token']; + $requestHeaders = array_change_key_case($request->getHeaders(), CASE_LOWER); + foreach ($requestHeaders as $name => &$values) { + if (in_array($name, $sensitiveHeaders, true)) { + $values = ['[REDACTED]']; + } + } + unset($values); + $responseHeaders = array_change_key_case($response?->getHeaders() ?? [], CASE_LOWER); + foreach ($responseHeaders as $name => &$values) { + if (in_array($name, ['set-cookie', 'authorization'], true)) { + $values = ['[REDACTED]']; + } + } + unset($values); + $data = [ 'config' => $guzzleConfig, // request 'http.request.method' => $request->getMethod(), - 'http.request.body.size' => strlen($options['body'] ?? ''), - 'http.request.full_url' => (string) $request->getUri(), + 'http.request.body.size' => isset($options['body']) ? strlen((string) $options['body']) : 0, + 'http.request.full_url' => $uriWithoutQuery, 'http.request.path' => $request->getUri()->getPath(), 'http.request.scheme' => $request->getUri()->getScheme(), 'http.request.host' => $request->getUri()->getHost(), 'http.request.port' => $request->getUri()->getPort(), 'http.request.user_agent' => $request->getHeaderLine('User-Agent'), // updated key for consistency - 'http.request.headers' => $request->getHeaders(), + 'http.request.headers' => $requestHeaders, // response 'http.response.status_code' => $response?->getStatusCode(), 'http.response.body.size' => $response?->getBody()->getSize() ?? 0, 'http.response.reason' => $response?->getReasonPhrase(), - 'http.response.headers' => $response?->getHeaders(), + 'http.response.headers' => $responseHeaders, 'duration' => $stats->getTransferTime() * 1000, ]; @@ - Integration::addBreadcrumb(new Breadcrumb( + Integration::addBreadcrumb(new Breadcrumb( Breadcrumb::LEVEL_INFO, Breadcrumb::TYPE_DEFAULT, 'guzzle', - $uri, + $uriWithoutQuery, $data ));此外,如需保留部分 query,可引入白名单方式仅上报非敏感参数。
Also applies to: 82-88
src/sentry/src/Aspect/FilesystemAspect.php (1)
76-92: match 语句存在语法问题(可能导致解析失败)分支列表里 'mimeType', 前多余逗号,导致 => 前出现尾随逗号。应去掉该逗号,维持多条件合并的合法语法。
建议修复:
$description = match ($method) { 'move', 'copy' => sprintf( 'from "%s" to "%s"', $arguments['source'] ?? '', $arguments['destination'] ?? '' ), 'write', 'writeStream', 'read', 'readStream', 'setVisibility', 'visibility', 'delete', 'deleteDirectory', 'createDirectory', 'fileExists', 'directoryExists', 'listContents', 'lastModified', 'fileSize', - 'mimeType', => $arguments['path'] ?? '', + 'mimeType' => $arguments['path'] ?? '', default => '', };src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
96-101: 潜在运行时错误:json_encode 失败时 strlen 参数类型不匹配在 item_size 计算中,json_encode 可能返回 false,strlen(false) 将抛 TypeError(PHP 8+)。
建议为 json_encode 结果添加安全转换:
->setData([ 'cache.key' => $keys, 'cache.ttl' => $arguments['ttl'] ?? null, 'item_size' => match (true) { - isset($arguments['value']) => strlen(json_encode($arguments['value'])), - isset($arguments['values']) && is_array($arguments['values']) => strlen(json_encode(array_values($arguments['values']))), + isset($arguments['value']) => strlen((string) (json_encode($arguments['value']) ?: '')), + isset($arguments['values']) && is_array($arguments['values']) => strlen((string) (json_encode(array_values($arguments['values'])) ?: '')), default => 0, }, ])src/sentry/src/Switcher.php (1)
62-76: 缺少 Feature::isExceptionIgnored 导致迁移后致命错误根据 PR 说明,使用方会把类型提示从 Switcher 切到 Feature,方法调用保持不变。但当前 Feature 并未实现
isExceptionIgnored()(见src/sentry/src/Feature.php),因此一旦代码改为注入 Feature,调用该方法会直接触发 “Call to undefined method Feature::isExceptionIgnored()” 的致命错误。请把该实现上移到 Feature(并在 Feature 引入use Throwable;),Switcher 只需继承即可,从而保证迁移安全。建议补充的实现:
+use Throwable; + class Feature { @@ + public function isExceptionIgnored(string|Throwable $exception): bool + { + $ignoreExceptions = (array) $this->config->get('sentry.ignore_exceptions', []); + + foreach ($ignoreExceptions as $ignoreException) { + if (is_a($exception, $ignoreException, true)) { + return true; + } + } + + return false; + }
🧹 Nitpick comments (1)
src/sentry/src/Feature.php (1)
67-70: 命名可读性微调建议isDisableCoroutineTracing 命名略显别扭,建议新增更自然的等价别名 isCoroutineTracingDisabled(保留现有方法并标注 @deprecated 以兼容既有代码)。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/sentry/src/Aspect/CacheAspect.php(2 hunks)src/sentry/src/Aspect/FilesystemAspect.php(2 hunks)src/sentry/src/Aspect/GuzzleHttpClientAspect.php(2 hunks)src/sentry/src/Aspect/LoggerAspect.php(2 hunks)src/sentry/src/Aspect/RedisAspect.php(2 hunks)src/sentry/src/Crons/Listener/EventHandleListener.php(2 hunks)src/sentry/src/Feature.php(1 hunks)src/sentry/src/Listener/EventHandleListener.php(2 hunks)src/sentry/src/Switcher.php(2 hunks)src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/CacheAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/CoordinatorAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/CoroutineAspect.php(3 hunks)src/sentry/src/Tracing/Aspect/DbAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/GrpcAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/RedisAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/RpcAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php(2 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(2 hunks)src/sentry/src/Tracing/Tracer.php(2 hunks)tests/Sentry/SwitcherTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (25)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
tests/Sentry/SwitcherTest.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Aspect/LoggerAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Aspect/CacheAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Listener/EventHandleListener.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php (2)
src/sentry/src/Feature.php (1)
Feature(18-71)src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
__construct(38-40)
src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/DbAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (2)
src/sentry/src/Feature.php (1)
Feature(18-71)src/sentry/src/Aspect/LoggerAspect.php (1)
__construct(35-37)
src/sentry/src/Aspect/GuzzleHttpClientAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Aspect/FilesystemAspect.php (1)
src/sentry/src/Feature.php (2)
Feature(18-71)__construct(20-22)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Tracer.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Aspect/RedisAspect.php (3)
src/sentry/src/Feature.php (1)
Feature(18-71)src/sentry/src/Aspect/CacheAspect.php (1)
__construct(35-37)src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
__construct(41-45)
src/sentry/src/Feature.php (1)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
src/sentry/src/Feature.php (2)
Feature(18-71)isDisableCoroutineTracing(67-70)
src/sentry/src/Crons/Listener/EventHandleListener.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
src/sentry/src/Feature.php (1)
Feature(18-71)
src/sentry/src/Switcher.php (1)
src/sentry/src/Feature.php (5)
Feature(18-71)isEnabled(24-27)isBreadcrumbEnabled(29-32)isTracingEnabled(34-41)isTracingSpanEnabled(43-50)
⏰ 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.3 with Swoole 6.0.2
- 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
- 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.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 5.1.7
🔇 Additional comments (24)
src/sentry/src/Feature.php (1)
24-61: 整体实现稳健,接口清晰且默认值合理
- 配置读取与分级开关设计清晰(全局开关优先于子开关)。
- isTracingSpanEnabled 在无当前 Span 时短路,避免空指针,👍
src/sentry/src/Aspect/CacheAspect.php (1)
15-15: 从 Switcher 切换到 Feature 的依赖注入变更正确类型与 use 更新一致,运行时行为不变。
Also applies to: 35-35
src/sentry/src/Aspect/GuzzleHttpClientAspect.php (1)
14-14: 依赖注入从 Switcher → Feature 的迁移正确构造参数与 use 一致更新,无行为回归。
Also applies to: 32-32
src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php (1)
16-16: 从 Switcher → Feature 的迁移正确类型与用法一致,追踪开关行为未改变。
Also applies to: 43-44
src/sentry/src/Aspect/FilesystemAspect.php (1)
14-14: 从 Switcher → Feature 的替换正确构造函数与 use 更新一致。
Also applies to: 44-44
src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
14-14: 依赖注入类型替换正确与 Feature 的 API 对齐,无行为变化。
Also applies to: 39-39
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)
14-14: 从 Switcher → Feature 的替换正确切换到 isTracingSpanEnabled('coordinator') 的用法一致,逻辑无回归。
Also applies to: 29-29
src/sentry/src/Aspect/LoggerAspect.php (1)
16-16: 依赖注入替换正确日志面包屑开关逻辑保持不变。
Also applies to: 35-35
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
14-70: 替换 Feature 类型无行为差异构造函数改为注入
Feature与新类保持一致,后续逻辑保持不变。tests/Sentry/SwitcherTest.php (1)
14-39: 测试对 Feature 的覆盖保持完整用例切换到
Feature后依旧验证相同的配置读取路径,覆盖面未受影响。src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
15-128: Kafka 追踪开关迁移到 Feature 合理构造参数与注入点同步更新为
Feature,其余追踪逻辑维持原状。src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
14-72: gRPC 追踪依赖调整一致依赖改为
Feature后仍按原机制判断 span 开关,行为保持稳定。src/sentry/src/Tracing/Tracer.php (1)
14-116: Tracer 改注入 Feature 符合整体重构更换依赖类型后仍能沿用相同的追踪配置与异常标记逻辑。
src/sentry/src/Crons/Listener/EventHandleListener.php (1)
15-155: Crons 监听器依赖更新与 Feature 匹配构造函数改为
Feature后,定时任务开关判断与之前保持一致。src/sentry/src/Listener/EventHandleListener.php (1)
14-624: 事件监听器使用 Feature 继续覆盖全部开关注入类型调换后,所有
isEnabled/isBreadcrumbEnabled调用无须改动,逻辑未受影响。src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php (1)
15-116: AMQP 生产者追踪改依赖 Feature 一致追踪开关继续通过相同接口读取,功能与重构目标对齐。
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
16-16: 依赖类型替换确认无误导入与构造函数参数同步改为
Feature,与新特性类保持一致,同时维持$switcher属性命名不会影响现有逻辑。Also applies to: 70-70
src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (1)
14-30: 注入类型更新保持一致性构造函数改用
Feature,确保注解切面使用新的特性开关实现,行为保持稳定。src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
15-45: RPC 切面依赖调整合理依赖类型替换为
Feature后其余逻辑无需改动,现有特性开关调用继续生效。src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
14-49: 协程切面同步至新特性类构造注入和静态方法调用统一指向
Feature,覆盖到协程追踪禁用判断,逻辑闭环完整。src/sentry/src/Aspect/RedisAspect.php (1)
14-34: Redis 面包屑切面迁移确认引入
Feature后依旧复用$switcher属性,面包屑开关判定无需额外调整。src/sentry/src/Tracing/Aspect/DbAspect.php (1)
14-38: 数据库切面依赖替换正确构造参数更新为
Feature与新类接口匹配,其余追踪逻辑保持原样。src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
14-44: Redis 追踪切面适配完成依赖改为
Feature后相关配置读取与特性开关判定无需改动,兼容新命名。src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
15-44: HTTP 客户端切面更新一致构造函数与属性导入同步指向
Feature,其余调用链保持稳定。
- Updated the constructor parameter name from `switcher` to `feature` in various aspects including CacheAspect, FilesystemAspect, GuzzleHttpClientAspect, LoggerAspect, RedisAspect, and others for consistency and clarity. - Adjusted the corresponding method calls to use the new `feature` property instead of `switcher`. - Removed the obsolete SwitcherTest file and replaced it with a new FeatureTest file to reflect the changes in the Feature class.
* refactor(sentry): replace Switcher class with Feature class - Introduce new Feature class with the same functionality as Switcher - Update all aspect classes and listeners to use Feature instead of Switcher - Convert Switcher to extend Feature for backward compatibility with deprecation warnings - Add appropriate @deprecated annotations for all Switcher methods - Update test to use Feature class instead of Switcher - This change improves code clarity while maintaining backward compatibility Breaking change: Switcher class is deprecated and will be removed in v3.2 * refactor: rename switcher to feature in Sentry components - Updated the constructor parameter name from `switcher` to `feature` in various aspects including CacheAspect, FilesystemAspect, GuzzleHttpClientAspect, LoggerAspect, RedisAspect, and others for consistency and clarity. - Adjusted the corresponding method calls to use the new `feature` property instead of `switcher`. - Removed the obsolete SwitcherTest file and replaced it with a new FeatureTest file to reflect the changes in the Feature class. --------- Co-Authored-By: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
This PR refactors the Sentry integration by introducing a new
Featureclass to replace the existingSwitcherclass. The changes improve code clarity and naming conventions while maintaining full backward compatibility.Changes Made:
Featureclass: Introduces a new class with the same functionality asSwitcherbut with a more descriptive nameSwitcherclass now extendsFeatureand maintains all existing methods with proper deprecation warningsFeatureclass instead ofSwitcherFeatureclassSwitcherare marked as deprecated with clear migration pathsBreaking Changes:
Switcherclass is deprecated as of v3.1 and will be removed in v3.2. Users should migrate to theFeatureclass.Migration Guide:
All method calls remain the same - only the class name changes.
Test Plan
FeatureclassSwitchercontinues to workFeatureclassSwitchercontinue to work through inheritanceSummary by CodeRabbit