Skip to content

Conversation

@huangdijia
Copy link
Contributor

@huangdijia huangdijia commented Sep 20, 2025

Summary

This PR consolidates 11 separate Sentry event listener classes into a single unified EventListener class, significantly improving code maintainability and reducing duplication.

Changes Made

  • Removed 11 listener classes:

    • AmqpExceptionListener
    • AsyncQueueExceptionListener
    • CaptureExceptionListener
    • CommandExceptionListener
    • CrontabExceptionListener
    • DbQueryListener
    • KafkaExceptionListener
    • RedisCommandExecutedListener
    • RequestExceptionListener
    • SetRedisEventEnableListener
    • SetRequestLifecycleListener
  • Created unified EventListener class:

    • Centralized event handling using match statements
    • Consolidated all functionality from removed listeners
    • Improved code organization and maintainability
  • Updated ConfigProvider.php:

    • Replaced 11 individual listener registrations with single EventListener
    • Maintained all existing functionality

Benefits

  • Reduced code duplication: Eliminated repetitive exception handling logic
  • Improved maintainability: Single file to manage all Sentry event handling
  • Better performance: Reduced number of registered listeners
  • Preserved functionality: All existing event handling behavior maintained
  • Code reduction: 174 lines removed (672→498 lines)

Test Plan

  • Verify all Sentry events are still captured correctly
  • Test exception handling for all supported services (AMQP, AsyncQueue, Commands, etc.)
  • Confirm breadcrumbs are still generated for database and Redis operations
  • Validate request lifecycle events continue to work
  • Run existing Sentry integration tests

Summary by CodeRabbit

  • 新功能
    • 引入三类统一监听器,集中处理事件、定时任务监控与追踪,覆盖请求、数据库、Redis、命令、定时、异步队列、AMQP/Kafka 等场景;定时任务新增 check-in、时区解析与监控配置支持,改善任务状态上报与告警能力。
  • 重构
    • 移除大量分散的专用监听器,统一异常捕获、初始化与追踪流程,简化注册与配置,提高一致性。
  • 杂项
    • 若干追踪/切面内部数据、标签与错误标记微调;更新拼写词表配置。

- Replace 11 separate listener classes with single EventListener class
- Consolidate all event handling logic into centralized match statement
- Merge functionality from deleted listeners:
  - AmqpExceptionListener, AsyncQueueExceptionListener
  - CaptureExceptionListener, CommandExceptionListener
  - CrontabExceptionListener, DbQueryListener
  - KafkaExceptionListener, RedisCommandExecutedListener
  - RequestExceptionListener, SetRedisEventEnableListener
  - SetRequestLifecycleListener
- Improve code maintainability and reduce duplication
- Preserve all existing functionality and event handling behavior
@coderabbitai
Copy link

coderabbitai bot commented Sep 20, 2025

Warning

Rate limit exceeded

@huangdijia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 32 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between a9f85df and a2f1674.

📒 Files selected for processing (2)
  • src/sentry/src/Listener/EventHandleListener.php (1 hunks)
  • src/sentry/src/Tracing/Listener/EventHandleListener.php (1 hunks)

Walkthrough

将多个按子系统拆分的异常与追踪监听器删除,并用三个集中式监听器替代:Listener\EventHandleListenerCrons\Listener\EventHandleListenerTracing\Listener\EventHandleListener;同时更新配置注册以仅注册这三类监听器,若干 tracing/aspect 内部数据与 tag 表达式也被调整。

Changes

Cohort / File(s) Change Summary
配置注册
src/sentry/src/ConfigProvider.php
将原有多个专用监听器注册项替换为三项:Listener\EventHandleListener, Crons\Listener\EventHandleListener, Tracing\Listener\EventHandleListener
新增:统一事件监听器
src/sentry/src/Listener/EventHandleListener.php
新增集中式监听器,统一路由框架/DB/Redis/请求/命令/队列/消息/Crontab 等事件,包含 Sentry SDK 初始化、异常捕获、面包屑与 flush 等逻辑。
新增:统一追踪监听器
src/sentry/src/Tracing/Listener/EventHandleListener.php
新增集中式追踪监听器,统一管理 span/transaction 的启动、结束、数据填充与载荷传播(DB/HTTP/RPC/Redis/AMQP/Kafka/AsyncQueue/命令/Crontab)。
新增:Crons 专用监听器
src/sentry/src/Crons/Listener/EventHandleListener.php
新增 crontab 处理:支持 check-in(in_progress/ok/error)、监控配置合成与上下文持久化(check-in id 存取)。
移除:分散的异常监听器
src/sentry/src/Listener/AmqpExceptionListener.php, .../AsyncQueueExceptionListener.php, .../CommandExceptionListener.php, .../CrontabExceptionListener.php, .../KafkaExceptionListener.php, .../RequestExceptionListener.php
删除多个按子系统的异常监听器(负责 SDK setup、captureException、flush 的类被集中式监听器取代)。
移除:辅助监听器与基类
src/sentry/src/Listener/CaptureExceptionListener.php, .../DbQueryListener.php, .../RedisCommandExecutedListener.php, .../SetRedisEventEnableListener.php, .../SetRequestLifecycleListener.php
删除 CaptureExceptionListener 基类及若干用于面包屑、开启事件或注入生命周期的辅助监听器(其职责已迁移或合并)。
移除:分散追踪监听器
src/sentry/src/Tracing/Listener/TracingAmqpListener.php, .../TracingAsyncQueueListener.php, .../TracingCommandListener.php, .../TracingCrontabListener.php, .../TracingDbQueryListener.php, .../TracingKafkaListener.php, .../TracingRedisListener.php, .../TracingRequestListener.php
删除多个按子系统拆分的 tracing 监听器,功能由新的集中式 Tracing\EventHandleListener 承接。
Tracing/Aspect 与其它细微调整
src/sentry/src/Tracing/Aspect/*, .vscode/cspell.json
多处 aspect 内部数据填充方式改为直接 setData、将 error 标签由布尔改为字符串 'true'exception.code 强制为字符串;.vscode/cspell.json 添加词表项。
Tracing 细节变更
src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php, .../CoordinatorAspect.php, .../CoroutineAspect.php, .../DbAspect.php, .../ElasticsearchAspect.php, .../GrpcAspect.php, .../GuzzleHttpClientAspect.php, .../KafkaProducerAspect.php, .../RedisAspect.php, .../RpcAspect.php, .../TraceAnnotationAspect.php
一致性调整:改为直接对 span 调用 setData、部分参数从位置参数改为命名参数、异常标签/code 类型与额外 stack_trace 的条件判断改写等。

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Hyperf as Hyperf 事件
  participant L as Listener\EventHandleListener
  participant S as Sentry_SDK
  Hyperf->>L: 各类事件(请求/DB/队列/消息/命令/Crontab/Redis)
  rect rgba(230,240,255,0.6)
    L->>L: 按事件类别路由并检查 feature 开关
    alt 异常/失败事件
      L->>S: captureException(throwable)
    else 起始事件(Before)
      L->>S: setup/context 初始化(start span / scope)
    else 结束事件(After/Terminate)
      L->>S: flush() / update check-in / finish span
    end
  end
Loading
sequenceDiagram
  autonumber
  actor Hyperf as Hyperf 事件
  participant T as Tracing\Listener\EventHandleListener
  participant S as Sentry_SDK
  Hyperf->>T: DB/HTTP/RPC/Redis/AMQP/Kafka/AsyncQueue/命令/Crontab 事件
  alt 起始事件
    T->>S: startTransaction/span(op, description, data)
  else 结束或失败事件
    T->>S: set tags/data/status\nfinish()
  end
  note over T,S: 遵循配置开关、采样与载荷传播策略
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • xuanyanwow

Poem

小兔子穿代码野,耳尖点点听变更,
旧侦听散作风中尘,新三将合一心。
事件汇流如溪转,追踪脉络更分明,
轻跳仓栈赞此功,胡萝卜献与你同。 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.68% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed 标题“refactor(sentry): consolidate event listeners into unified EventListener” 简洁且直接地反映了该 PR 的主要意图:将多个 Sentry 监听器合并为统一的事件处理实现以减少重复并提高可维护性。根据变更摘要和目标,变更确实主要围绕合并监听器展开,因此标题与变更高度相关且能让审阅者快速把握核心改动。尽管实现细节上新增了 Listener、Crons 和 Tracing 三个 EventHandleListener,而不是仅有单一类,但这并不削弱标题传达的主要目的和含义。

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 (1)
src/sentry/src/ConfigProvider.php (1)

54-64: 移除遗留监听器以避免重复处理

仓库搜索显示旧监听器仍被注册/实现,需清理并同步文档与 autoload:

  • src/telescope/src/ConfigProvider.php — 注册了 DbQueryListener、RedisCommandExecutedListener、SetRequestLifecycleListener(约行 44–51)
  • src/telescope/src/Listener/DbQueryListener.php
  • src/telescope/src/Listener/RedisCommandExecutedListener.php
  • src/telescope/src/Listener/SetRequestLifecycleListener.php
  • docs/**/telescope.md(多语言)— 仍引用 SetRequestLifecycleListener
  • CHANGELOG-3.1.md — 提及 RedisCommandExecutedListener 的新增
  • vendor/composer/autoload_classmap.php、vendor/composer/autoload_static.php — 为生成的类映射,源码清理后需通过 composer dump-autoload 更新

动作:从 telescope 的 ConfigProvider 中移除或禁用上述注册,删除或归档对应 Listener 实现,更新 docs/CHANGELOG,运行 composer dump-autoload 并复测以确保不再重复注册。

🧹 Nitpick comments (7)
src/sentry/src/Listener/EventListener.php (7)

54-101: 建议补充监听 ExceptionDispatched 统一兜底异常

合并后未见对 FriendsOfHyperf\ExceptionEvent\Event\ExceptionDispatched 的监听,可能遗漏由异常组件分发的全局异常。建议加入该事件并在 process() 中捕获。

应用此补丁(需对应 use 引入):

@@
 use Hyperf\Server\Event;
+use FriendsOfHyperf\ExceptionEvent\Event\ExceptionDispatched;
@@
     public function listen(): array
     {
         return [
@@
-            KafkaEvent\AfterConsume::class,
+            KafkaEvent\AfterConsume::class,
+            // Global exception events
+            ExceptionDispatched::class,
         ];
     }
@@
-        match ($event::class) {
+        match ($event::class) {
@@
-            KafkaEvent\FailToConsume::class => $this->handleKafkaFail($event),
+            KafkaEvent\FailToConsume::class => $this->handleKafkaFail($event),
+
+            // Global exception events
+            ExceptionDispatched::class => $this->captureException($event->getThrowable()),
 
             default => null,
         };
     }

建议补充端到端用例覆盖 request/console/async-queue/AMQP/Kafka/crontab 下的异常是否均能上报。

Also applies to: 103-152


154-169: 重复 flush 影响吞吐量:captureException 已 flush,后续又调用 flushEvents

captureException()finally 会调用 flush(),而各个 *After/*Failed/*Terminated 处理器又调用 flushEvents(),导致双重 flush。在高并发下会放大 I/O 成本。建议只在生命周期末尾统一 flush,移除 captureException() 内的 flush。

     protected function captureException($throwable): void
     {
@@
-        } finally {
-            $hub->getClient()?->flush();
-        }
+        }
     }

若需兼容旧行为,可添加 captureException($t, bool $flush=false) 开关并在少量场景传 true

Also applies to: 176-179, 385-392, 400-412, 429-440, 457-468, 485-496


233-253: 按引用遍历后未释放引用,存在惯性引用隐患

foreach ($servers as &$server) 结束后建议 unset($server);,避免后续对 $server 的误用。

         foreach ($servers as &$server) {
             $callbacks = $server['callbacks'] ?? [];
@@
             }
         }
 
+        unset($server);
         $this->config->set('server.servers', $servers);

308-328: Redis 面包屑上下文健壮性与隐私

  • duration 直接使用 $event->time * 1000,若 time 缺失将触发告警。
  • parameters/result 可能包含敏感信息,建议提供更细粒度开关。
         Integration::addBreadcrumb(new Breadcrumb(
@@
-            [
-                'arguments' => $event->parameters,
-                'result' => $event->result,
-                'duration' => $event->time * 1000,
-            ]
+            array_filter([
+                'arguments' => $event->parameters ?? null,
+                'result' => $event->result ?? null,
+                'duration' => isset($event->time) ? $event->time * 1000 : null,
+            ], static fn($v) => $v !== null)
         ));

同时考虑新增 sentry.breadcrumbs.redis_arguments/result 开关以降低泄露风险。


103-152: 使用 instanceof 提升兼容性

match ($event::class) 要求严格类名匹配,若框架后续引入子类/代理类会失配。建议改为 match(true) + instanceof 或 if/elseif。

示例(节选):

-    match ($event::class) {
-        QueryExecuted::class => $this->handleDbQuery($event),
-        TransactionBeginning::class,
-        TransactionCommitted::class,
-        TransactionRolledBack::class => $this->handleDbTransaction($event),
+    match (true) {
+        $event instanceof QueryExecuted => $this->handleDbQuery($event),
+        $event instanceof TransactionBeginning,
+        $event instanceof TransactionCommitted,
+        $event instanceof TransactionRolledBack => $this->handleDbTransaction($event),
         ...
     };

181-186: 静态分析告警:多个处理器未使用 $event 参数

这些方法签名由接口决定,但参数未使用触发 PHPMD 告警。建议在方法上添加抑制注解以保持代码整洁。

示例(请在各未使用的方法上类比添加):

-    protected function handleBootApplication(object $event): void
+    /**
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
+     */
+    protected function handleBootApplication(object $event): void
     {
         $this->setupRequestLifecycle();
         $this->setupRedisEventEnable();
     }

Also applies to: 330-338, 349-357, 367-375, 376-384, 385-393, 414-422, 423-431, 442-450, 451-459, 470-478, 479-487


266-289: 确认 QueryExecuted::$time 单位并添加 SQL 参数掩码

  • Hyperf 的 QueryExecuted::$time 单位为毫秒,与 executionTimeMs 命名一致,无需额外换算 (tzf-foryou.xyz)
  • 建议对 sql_bindings 下的参数值进行掩码或过滤(如通过白/黑名单过滤 PII 字段),以防泄露敏感信息
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13ce1ac and d267cfc.

📒 Files selected for processing (13)
  • src/sentry/src/ConfigProvider.php (1 hunks)
  • src/sentry/src/Listener/AmqpExceptionListener.php (0 hunks)
  • src/sentry/src/Listener/AsyncQueueExceptionListener.php (0 hunks)
  • src/sentry/src/Listener/CaptureExceptionListener.php (0 hunks)
  • src/sentry/src/Listener/CommandExceptionListener.php (0 hunks)
  • src/sentry/src/Listener/CrontabExceptionListener.php (0 hunks)
  • src/sentry/src/Listener/DbQueryListener.php (0 hunks)
  • src/sentry/src/Listener/EventListener.php (1 hunks)
  • src/sentry/src/Listener/KafkaExceptionListener.php (0 hunks)
  • src/sentry/src/Listener/RedisCommandExecutedListener.php (0 hunks)
  • src/sentry/src/Listener/RequestExceptionListener.php (0 hunks)
  • src/sentry/src/Listener/SetRedisEventEnableListener.php (0 hunks)
  • src/sentry/src/Listener/SetRequestLifecycleListener.php (0 hunks)
💤 Files with no reviewable changes (11)
  • src/sentry/src/Listener/AmqpExceptionListener.php
  • src/sentry/src/Listener/CrontabExceptionListener.php
  • src/sentry/src/Listener/CommandExceptionListener.php
  • src/sentry/src/Listener/AsyncQueueExceptionListener.php
  • src/sentry/src/Listener/DbQueryListener.php
  • src/sentry/src/Listener/SetRequestLifecycleListener.php
  • src/sentry/src/Listener/KafkaExceptionListener.php
  • src/sentry/src/Listener/RedisCommandExecutedListener.php
  • src/sentry/src/Listener/SetRedisEventEnableListener.php
  • src/sentry/src/Listener/RequestExceptionListener.php
  • src/sentry/src/Listener/CaptureExceptionListener.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/sentry/src/ConfigProvider.php (1)
src/sentry/src/Listener/EventListener.php (1)
  • EventListener (43-497)
src/sentry/src/Listener/EventListener.php (4)
src/sentry/src/Integration.php (1)
  • Integration (28-158)
src/sentry/src/Switcher.php (3)
  • Switcher (19-85)
  • isTracingEnable (35-42)
  • isBreadcrumbEnable (30-33)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
src/exception-event/src/Event/ExceptionDispatched.php (1)
  • getThrowable (30-33)
🪛 PHPMD (2.15.0)
src/sentry/src/Listener/EventListener.php

181-181: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


261-261: Avoid unused local variables such as '$_'. (undefined)

(UnusedLocalVariable)


330-330: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


349-349: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


367-367: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


376-376: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


385-385: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


414-414: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


423-423: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


442-442: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


451-451: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


470-470: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


479-479: Avoid unused parameters such as '$event'. (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). (10)
  • 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.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 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
🔇 Additional comments (3)
src/sentry/src/Listener/EventListener.php (3)

171-175: 初始化一次化处理设计合理

使用 Context::getOrSet 保证 SDK 仅初始化一次,避免重复开销。


54-101: 统一监听列表清晰且覆盖面广

整合后的注册项完整,便于维护。

建议补充集成测试覆盖:每类事件至少一例,验证面包屑/异常是否如预期发送,并校验无重复 flush(结合上文改动)。如需我可生成基础用例骨架。


339-347: 不要直接访问 $event->exception — 优先使用 getThrowable() 并兜底检查 throwable/exception

仓库未包含 Hyperf 事件类实现;Hyperf 的请求生命周期确实有 RequestTerminated 事件,周边实现/监听器普遍通过 getThrowable()/throwable 暴露异常,直接访问 $event->exception 会导致未定义属性访问并静默丢失异常。 (github.com)

建议(简短):

  • 在 handleRequestTerminated 中按顺序读取异常并仅在存在时捕获:先 if (method_exists($event, 'getThrowable')) -> $event->getThrowable(); 再 elseif (property_exists($event, 'throwable')) -> $event->throwable; 再 elseif (property_exists($event, 'exception')) -> $event->exception; 最后调用 captureException 仅当为 Throwable 时;保留 flushEvents()。
  • 或改为监听通用的异常分发事件(ExceptionDispatched / exception-event)以避免不同 Hyperf 版本间签名差异。

位置:src/sentry/src/Listener/EventListener.php — handleRequestTerminated(约 lines 339–347)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

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/Crons/Listener/EventHandleListener.php (1)

98-105: captureCheckIn 建议加异常保护并存储字符串化的 checkInId

网络/SDK 异常会中断任务;且 Context 中存 EventId 对象可能引发类型不一致。建议 try/catch 并转为字符串后保存。

-            $checkInId = $hub->captureCheckIn(
-                slug: $slug,
-                status: CheckInStatus::inProgress(),
-                monitorConfig: $monitorConfig,
-            );
-
-            Context::set(Constants::CRON_CHECKIN_ID, $checkInId);
+            try {
+                $checkInId = $hub->captureCheckIn(
+                    slug: $slug,
+                    status: CheckInStatus::inProgress(),
+                    monitorConfig: $monitorConfig,
+                );
+                Context::set(Constants::CRON_CHECKIN_ID, (string) $checkInId);
+            } catch (\Throwable $e) {
+                $this->logger->error((string) $e);
+            }
🧹 Nitpick comments (6)
src/sentry/src/Crons/Listener/EventHandleListener.php (3)

63-72: Crontab 规则校验不严谨:应严格等于 5 段并按空白分割

当前仅在段数大于 5 时告警;少于 5 的非法规则会继续执行,可能导致 Sentry 报错。建议用空白分割并要求恰好 5 段。

-            $rule = $event->crontab->getRule();
-            $rules = explode(' ', $rule);
-
-            if (count($rules) > 5) {
+            $rule = trim($event->crontab->getRule());
+            $fields = preg_split('/\s+/', $rule, -1, PREG_SPLIT_NO_EMPTY);
+            if (count($fields) !== 5) {
                 $this->logger->warning(sprintf('Crontab rule %s is not supported by sentry', $rule));
                 return;
             }

107-119: 完成后清理 Context 中的 check-in ID,避免串任务污染

成功上报后未清理,下一次任务可能读取到旧 ID。

             $hub->captureCheckIn(
                 slug: $slug,
                 status: CheckInStatus::ok(),
                 checkInId: $checkInId,
             );
+            Context::set(Constants::CRON_CHECKIN_ID, null);

121-133: 失败上报同样需要异常保护与清理

与成功分支一致,增加 try/catch 并清理 Context。

-            $hub->captureCheckIn(
-                slug: $slug,
-                status: CheckInStatus::error(),
-                checkInId: $checkInId,
-            );
+            try {
+                $hub->captureCheckIn(
+                    slug: $slug,
+                    status: CheckInStatus::error(),
+                    checkInId: $checkInId,
+                );
+            } catch (\Throwable $e) {
+                $this->logger->error((string) $e);
+            } finally {
+                Context::set(Constants::CRON_CHECKIN_ID, null);
+            }
src/sentry/src/Listener/EventHandleListener.php (3)

269-271: 消除未使用变量警告(PHPMD)

遍历键名更简洁也可避免“未使用的局部变量”提示。

-        foreach ($this->config->get('redis', []) as $pool => $_) {
-            $this->config->set("redis.{$pool}.event.enable", true);
-        }
+        foreach (array_keys($this->config->get('redis', [])) as $pool) {
+            $this->config->set("redis.{$pool}.event.enable", true);
+        }

331-341: Redis 面包屑 result 可能体量过大

result 可能为大型数组/对象,易超限。建议可配置开关或截断。

-                'result' => $event->result,
+                // 可选:根据配置决定是否记录 result,或仅截断/摘要
+                'result' => is_scalar($event->result) ? $event->result : '[non-scalar result]',

185-193: 关于多处“未使用参数 $event”的静态检查噪音

这些方法必须保留签名。建议在类级或方法级加抑制注解(如 PHPMD)或在注释中标明预期未用。

Also applies to: 347-355, 372-380, 397-405, 409-417, 433-441, 446-454, 470-478, 483-491, 495-503, 507-515, 520-528, 532-540, 544-552

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d267cfc and d1567d0.

📒 Files selected for processing (12)
  • src/sentry/src/ConfigProvider.php (1 hunks)
  • src/sentry/src/Crons/Listener/EventHandleListener.php (1 hunks)
  • src/sentry/src/Listener/EventHandleListener.php (1 hunks)
  • src/sentry/src/Tracing/Listener/EventHandleListener.php (1 hunks)
  • src/sentry/src/Tracing/Listener/TracingAmqpListener.php (0 hunks)
  • src/sentry/src/Tracing/Listener/TracingAsyncQueueListener.php (0 hunks)
  • src/sentry/src/Tracing/Listener/TracingCommandListener.php (0 hunks)
  • src/sentry/src/Tracing/Listener/TracingCrontabListener.php (0 hunks)
  • src/sentry/src/Tracing/Listener/TracingDbQueryListener.php (0 hunks)
  • src/sentry/src/Tracing/Listener/TracingKafkaListener.php (0 hunks)
  • src/sentry/src/Tracing/Listener/TracingRedisListener.php (0 hunks)
  • src/sentry/src/Tracing/Listener/TracingRequestListener.php (0 hunks)
💤 Files with no reviewable changes (8)
  • src/sentry/src/Tracing/Listener/TracingRedisListener.php
  • src/sentry/src/Tracing/Listener/TracingDbQueryListener.php
  • src/sentry/src/Tracing/Listener/TracingAsyncQueueListener.php
  • src/sentry/src/Tracing/Listener/TracingCommandListener.php
  • src/sentry/src/Tracing/Listener/TracingCrontabListener.php
  • src/sentry/src/Tracing/Listener/TracingKafkaListener.php
  • src/sentry/src/Tracing/Listener/TracingAmqpListener.php
  • src/sentry/src/Tracing/Listener/TracingRequestListener.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/sentry/src/Tracing/Listener/EventHandleListener.php (5)
src/sentry/src/Switcher.php (4)
  • Switcher (19-85)
  • isTracingSpanEnable (44-51)
  • isTracingEnable (35-42)
  • isTracingExtraTagEnable (53-56)
src/sentry/src/Util/Carrier.php (3)
  • Carrier (21-111)
  • getSentryTrace (68-71)
  • getBaggage (73-76)
src/sentry/src/Util/SqlParser.php (1)
  • SqlParser (20-65)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
src/sentry/src/Tracing/SpanStarter.php (3)
  • startSpan (33-52)
  • startRequestTransaction (54-77)
  • continueTrace (84-115)
src/sentry/src/ConfigProvider.php (3)
src/sentry/src/Listener/EventHandleListener.php (1)
  • EventHandleListener (43-553)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
  • EventHandleListener (68-716)
src/sentry/src/Crons/Listener/EventHandleListener.php (1)
  • EventHandleListener (24-135)
src/sentry/src/Listener/EventHandleListener.php (3)
src/sentry/src/Integration.php (1)
  • Integration (28-158)
src/sentry/src/Switcher.php (3)
  • Switcher (19-85)
  • isTracingEnable (35-42)
  • isBreadcrumbEnable (30-33)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
🪛 GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
src/sentry/src/Tracing/Listener/EventHandleListener.php

[failure] 354-354:
Parameter #1 $tags of method Sentry\Tracing\Span::setTags() expects array<string, string>, array<string, int> given.

🪛 PHPMD (2.15.0)
src/sentry/src/Listener/EventHandleListener.php

188-188: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


269-269: Avoid unused local variables such as '$_'. (undefined)

(UnusedLocalVariable)


347-347: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


372-372: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


397-397: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


409-409: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


421-421: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


446-446: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


458-458: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


483-483: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


495-495: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


520-520: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


532-532: Avoid unused parameters such as '$event'. (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). (7)
  • GitHub Check: Test on PHP 8.2 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
  • 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
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
🔇 Additional comments (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)

497-505: AMQP headers 读取更稳健的方式(AMQPTable 兼容)

AMQPTable 不总是可数组访问;建议通过 API 获取并确保为字符串。
[suggest_minor_issue]

-            $applicationHeaders = $amqpMessage->has('application_headers') ? $amqpMessage->get('application_headers') : null;
-            if ($applicationHeaders && isset($applicationHeaders[Constants::TRACE_CARRIER])) {
-                $carrier = Carrier::fromJson($applicationHeaders[Constants::TRACE_CARRIER]);
+            $applicationHeaders = $amqpMessage->has('application_headers') ? $amqpMessage->get('application_headers') : null;
+            if ($applicationHeaders instanceof AMQPTable && $applicationHeaders->get(Constants::TRACE_CARRIER)) {
+                $carrier = Carrier::fromJson((string) $applicationHeaders->get(Constants::TRACE_CARRIER));
                 Context::set(Constants::TRACE_CARRIER, $carrier);
             }

Copy link
Contributor Author

@huangdijia huangdijia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

代码审查报告

1. 变更总结

  • 重构目标: 将多个独立的Sentry事件监听器合并为统一的EventHandleListener
  • 主要变更:
    • 删除了11个独立的监听器类
    • 创建了3个统一的EventHandleListener类
    • 简化了ConfigProvider.php中的监听器注册配置
    • 合并了异常捕获、面包屑记录、追踪等功能

2. 潜在风险

性能风险

  • 监听器臃肿: 单个EventHandleListener类承担了过多职责(553行代码),可能影响性能
  • 事件处理开销: 每个事件都需要经过大量的match语句判断,增加了处理开销
  • 内存占用: 统一监听器需要处理更多事件类型,可能增加内存消耗

维护性风险

  • 代码复杂度: 单一类处理多种事件类型,违反了单一职责原则
  • 调试困难: 当某个特定事件处理出现问题时,调试范围变大
  • 扩展性降低: 添加新的事件类型需要修改核心监听器类

功能风险

  • 错误处理: 某个事件处理失败可能影响其他事件的处理
  • 配置复杂: 不同事件类型的开关控制逻辑混合在一起,容易出错

3. 优化建议

架构优化

  • 策略模式: 考虑使用策略模式,为每种事件类型创建独立的处理策略
  • 事件分组: 将相关的事件处理逻辑分组到独立的trait或helper类中
  • 接口抽象: 定义统一的事件处理接口,提高代码的可测试性

代码优化

  • 方法拆分: 将大型的process方法拆分为更小的专用方法
  • 常量提取: 将硬编码的字符串提取为类常量
  • 性能优化: 考虑使用数组映射替代多层match语句

示例优化代码结构:

// 建议的优化结构
class EventHandleListener implements ListenerInterface
{
    private array $handlers = [];
    
    public function __construct()
    {
        $this->handlers = [
            CommandEvent\BeforeHandle::class => new CommandEventHandler(),
            AmqpEvent\BeforeConsume::class => new AmqpEventHandler(),
            // ...
        ];
    }
    
    public function process(object $event): void
    {
        $handler = $this->handlers[$event::class] ?? null;
        $handler?->handle($event);
    }
}

4. 合规性检查

代码风格 ✅

  • 遵循PSR-12代码规范
  • 使用了适当的类型声明和返回类型
  • 注释格式符合PhpDoc标准

命名规范 ✅

  • 类名、方法名遵循驼峰命名规范
  • 命名空间结构合理
  • 方法名语义明确

项目架构 ⚠️

  • 违反单一职责: EventHandleListener承担了过多责任
  • 模块边界模糊: 不同类型的事件处理逻辑混合在同一个类中
  • 依赖注入合理: 构造函数依赖注入使用得当

兼容性 ✅

  • 保持了与现有配置的兼容性
  • 监听的事件类型保持不变
  • 公共接口没有破坏性变更

总结评价

这次重构在简化配置减少文件数量方面取得了成效,但在代码质量可维护性方面存在一些问题。建议在当前基础上进一步优化,采用更合理的设计模式来平衡代码复用和职责分离。

Copy link

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (9)
src/sentry/src/Crons/Listener/EventHandleListener.php (1)

116-120: check-in ID 获取时的类型断言可能不安全

与上面相同的问题,建议使用更安全的类型检查。

src/sentry/src/Tracing/Listener/EventHandleListener.php (8)

306-311: 标签值必须为字符串类型

setTags 方法要求所有标签值必须是字符串类型,但这里传入了布尔值和整数。

             $transaction->setStatus(SpanStatus::internalError())
                 ->setTags([
-                    'error' => true,
-                    'exception.class' => $exception::class,
-                    'exception.code' => $exception->getCode(),
-                    'exception.message' => $exception->getMessage(),
+                    'error' => 'true',
+                    'exception.class' => $exception::class,
+                    'exception.code' => (string) $exception->getCode(),
+                    'exception.message' => $exception->getMessage(),
                 ]);

354-356: 命令退出码标签值必须为字符串

CI 测试失败表明 setTags 只接受字符串值。

         ])->setTags([
-            'command.exit_code' => $exitCode,
+            'command.exit_code' => (string) $exitCode,
         ]);

423-428: Redis span 标签值类型不一致

             $span->setStatus(SpanStatus::internalError())
                 ->setTags([
-                    'error' => true,
-                    'exception.class' => $exception::class,
-                    'exception.message' => $exception->getMessage(),
-                    'exception.code' => $exception->getCode(),
+                    'error' => 'true',
+                    'exception.class' => $exception::class,
+                    'exception.message' => $exception->getMessage(),
+                    'exception.code' => (string) $exception->getCode(),
                 ]);

472-477: Crontab 异常标签值类型错误

             $transaction->setStatus(SpanStatus::internalError())
                 ->setTags([
-                    'error' => true,
-                    'exception.class' => $exception::class,
-                    'exception.message' => $exception->getMessage(),
-                    'exception.code' => $exception->getCode(),
+                    'error' => 'true',
+                    'exception.class' => $exception::class,
+                    'exception.message' => $exception->getMessage(),
+                    'exception.code' => (string) $exception->getCode(),
                 ]);

550-555: AMQP 异常标签值需要字符串化

             $transaction->setStatus(SpanStatus::internalError())
                 ->setTags([
-                    'error' => true,
-                    'exception.class' => $exception::class,
-                    'exception.message' => $exception->getMessage(),
-                    'exception.code' => $exception->getCode(),
+                    'error' => 'true',
+                    'exception.class' => $exception::class,
+                    'exception.message' => $exception->getMessage(),
+                    'exception.code' => (string) $exception->getCode(),
                 ]);

621-627: Kafka 异常标签值需要字符串化

             $transaction->setStatus(SpanStatus::internalError())
                 ->setTags([
-                    'error' => true,
-                    'exception.class' => $exception::class,
-                    'exception.message' => $exception->getMessage(),
-                    'exception.code' => $exception->getCode(),
+                    'error' => 'true',
+                    'exception.class' => $exception::class,
+                    'exception.message' => $exception->getMessage(),
+                    'exception.code' => (string) $exception->getCode(),
                 ]);

681-686: AsyncQueue 异常标签值类型不一致

             $transaction->setStatus(SpanStatus::internalError())
                 ->setTags([
-                    'error' => true,
-                    'exception.class' => $exception::class,
-                    'exception.message' => $exception->getMessage(),
-                    'exception.code' => $exception->getCode(),
+                    'error' => 'true',
+                    'exception.class' => $exception::class,
+                    'exception.message' => $exception->getMessage(),
+                    'exception.code' => (string) $exception->getCode(),
                 ]);

360-365: 异常相关标签值需要转换为字符串

与其他位置相同的问题,所有标签值都需要是字符串。

             $transaction->setStatus(SpanStatus::internalError())
                 ->setTags([
-                    'error' => true,
-                    'exception.class' => $exception::class,
-                    'exception.message' => $exception->getMessage(),
-                    'exception.code' => $exception->getCode(),
+                    'error' => 'true',
+                    'exception.class' => $exception::class,
+                    'exception.message' => $exception->getMessage(),
+                    'exception.code' => (string) $exception->getCode(),
                 ]);
🧹 Nitpick comments (6)
src/sentry/src/Crons/Listener/EventHandleListener.php (3)

52-54: 考虑缓存 getOptions() 结果以避免重复调用

process 方法中获取了 options,但在后续的 handleCrontabTaskStarting 方法中可能会再次调用。建议将 options 传递给所有处理方法。

-        match ($event::class) {
-            Event\BeforeExecute::class => $this->handleCrontabTaskStarting($event, $options),
-            Event\AfterExecute::class => $this->handleCrontabTaskFinished($event),
-            Event\FailToExecute::class => $this->handleCrontabTaskFailed($event),
-            default => null,
-        };
+        match ($event::class) {
+            Event\BeforeExecute::class => $this->handleCrontabTaskStarting($event, $options),
+            Event\AfterExecute::class => $this->handleCrontabTaskFinished($event, $options),
+            Event\FailToExecute::class => $this->handleCrontabTaskFailed($event, $options),
+            default => null,
+        };

98-102: check-in ID 获取时的类型断言可能不安全

使用 @var string 注释断言 $checkInId 为字符串,但实际可能为 null。建议使用更安全的类型检查。

-        /** @var string $checkInId */
-        $checkInId = Context::get(Constants::CRON_CHECKIN_ID);
-        if (! $checkInId) {
+        $checkInId = Context::get(Constants::CRON_CHECKIN_ID);
+        if (! is_string($checkInId) || empty($checkInId)) {
             return;
         }

144-145: 考虑为 threshold 参数设置合理的默认值或验证范围

failure_issue_thresholdrecovery_threshold 参数直接转换为 int,但没有验证其有效性。负值或过大的值可能会导致意外行为。

-        $failureIssueThreshold = isset($options['failure_issue_threshold']) ? (int) $options['failure_issue_threshold'] : null;
-        $recoveryThreshold = isset($options['recovery_threshold']) ? (int) $options['recovery_threshold'] : null;
+        $failureIssueThreshold = null;
+        if (isset($options['failure_issue_threshold'])) {
+            $threshold = (int) $options['failure_issue_threshold'];
+            $failureIssueThreshold = $threshold > 0 ? $threshold : null;
+        }
+        $recoveryThreshold = null;
+        if (isset($options['recovery_threshold'])) {
+            $threshold = (int) $options['recovery_threshold'];
+            $recoveryThreshold = $threshold > 0 ? $threshold : null;
+        }
src/sentry/src/Tracing/Listener/EventHandleListener.php (3)

464-467: Crontab 标签值可能包含非字符串类型

isSingleton()isOnOneServer() 方法返回布尔值,需要转换为字符串。

         $transaction->setTags([
             'crontab.rule' => $crontab->getRule(),
             'crontab.type' => $crontab->getType(),
-            'crontab.options.is_single' => $crontab->isSingleton(),
-            'crontab.options.is_on_one_server' => $crontab->isOnOneServer(),
+            'crontab.options.is_single' => $crontab->isSingleton() ? 'true' : 'false',
+            'crontab.options.is_on_one_server' => $crontab->isOnOneServer() ? 'true' : 'false',
         ]);

352-353: 访问命令输入的私有属性可能会导致兼容性问题

与上面类似,使用闭包访问 input 私有属性也存在兼容性风险。

建议考虑使用公开的 API 方法,或者在命令类中提供获取这些信息的公共方法。


231-237: 考虑使源配置可扩展

当前的源配置硬编码了三种选择,可以考虑使用策略模式使其更易扩展。

这样可以让用户更容易添加自定义的源策略,而不需要修改核心代码。

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1567d0 and 93fca7f.

📒 Files selected for processing (2)
  • src/sentry/src/Crons/Listener/EventHandleListener.php (1 hunks)
  • src/sentry/src/Tracing/Listener/EventHandleListener.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sentry/src/Tracing/Listener/EventHandleListener.php (5)
src/sentry/src/Switcher.php (4)
  • Switcher (19-85)
  • isTracingSpanEnable (44-51)
  • isTracingEnable (35-42)
  • isTracingExtraTagEnable (53-56)
src/sentry/src/Util/Carrier.php (3)
  • Carrier (21-111)
  • getSentryTrace (68-71)
  • getBaggage (73-76)
src/sentry/src/Util/SqlParser.php (1)
  • SqlParser (20-65)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
src/sentry/src/Tracing/SpanStarter.php (3)
  • startSpan (33-52)
  • startRequestTransaction (54-77)
  • continueTrace (84-115)
src/sentry/src/Crons/Listener/EventHandleListener.php (2)
src/sentry/src/Switcher.php (2)
  • Switcher (19-85)
  • isCronsEnable (71-74)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
⏰ 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 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.2 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 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
  • GitHub Check: Test on PHP 8.3 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 6.0.2
🔇 Additional comments (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)

349-350: 不要通过闭包访问命令的私有属性 exitCode

仓库中未能定位 vendor/symfony/console 或 Symfony Command 的定义,无法验证该私有属性在目标 Symfony 版本中的存在或可访问性。改用稳定的公共 API 获取退出码(例如使用 $command->run($input, $output) 的返回值或命令公开常量),并为多个 Symfony 版本添加兼容性/回归测试。
位置:src/sentry/src/Tracing/Listener/EventHandleListener.php:349-350

Copy link
Contributor Author

@huangdijia huangdijia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

补充代码审查报告

基于最新的代码变更,补充以下审查意见:

架构设计分析

优点

  • 配置简化: 从22个监听器减少到3个,大幅简化了配置管理
  • 代码复用: 统一的异常处理、面包屑记录和事件刷新逻辑
  • 功能完整: 保持了所有原有的Sentry集成功能

架构问题

  • 单一职责违反: EventHandleListener承担了过多职责,违反SOLID原则
  • 类文件过大: 553行代码的单一类,影响可读性和维护性
  • 紧耦合: 将不同域的事件处理逻辑耦合在一起

性能影响评估

潜在性能问题

  • 事件路由开销: 每个事件都需要经过大量match语句判断
  • 方法调用链: 统一处理增加了方法调用层次
  • 内存占用: 单个监听器实例需要处理更多事件类型

性能优化建议

// 建议使用数组映射优化事件路由
private static array $eventHandlers = [
    CommandEvent\BeforeHandle::class => 'handleCommandStarting',
    AmqpEvent\BeforeConsume::class => 'handleAmqpMessageProcessing',
    // ...
];

public function process(object $event): void
{
    $handler = self::$eventHandlers[$event::class] ?? null;
    if ($handler) {
        $this->{$handler}($event);
    }
}

代码质量问题

方法复杂度

  • process方法包含过多分支逻辑
  • 部分处理方法参数不一致(如handleCrontabTaskStarting需要额外的$options参数)
  • 错误处理逻辑分散在各个方法中

建议的重构方案

// 使用策略模式重构
interface EventHandlerInterface
{
    public function handle(object $event): void;
    public function supports(string $eventClass): bool;
}

class EventHandleListener implements ListenerInterface
{
    private array $handlers;
    
    public function __construct(array $handlers)
    {
        $this->handlers = $handlers;
    }
    
    public function process(object $event): void
    {
        foreach ($this->handlers as $handler) {
            if ($handler->supports($event::class)) {
                $handler->handle($event);
                break;
            }
        }
    }
}

测试覆盖建议

必要的测试案例

  • 事件路由测试: 确保每种事件类型都能正确路由到对应处理方法
  • 异常处理测试: 验证某个事件处理失败不会影响其他事件
  • 性能测试: 对比重构前后的事件处理性能
  • 并发测试: 验证在高并发场景下的稳定性

向后兼容性

兼容性保证 ✅

  • 保持了所有公共接口
  • 配置项无破坏性变更
  • 事件监听行为一致

最终建议

短期改进

  1. 添加性能监控点,观察重构后的性能表现
  2. 增加单元测试覆盖关键事件处理逻辑
  3. 添加方法级别的文档注释

长期优化

  1. 考虑采用策略模式或责任链模式重构
  2. 将大型类拆分为功能相关的小类
  3. 引入事件处理中间件机制

总体而言,这次重构在简化配置方面取得了显著成效,但需要关注代码质量性能影响。建议在保持当前功能的基础上,逐步优化架构设计。

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (10)
src/sentry/src/Tracing/Aspect/DbAspect.php (1)

98-105: 避免泄露敏感数据 & 空值安全

当前无条件记录 db.sql.bindings 且逐项展开为 db.parameter.*,存在敏感数据/PII 泄露与日志放大风险;同时 bindings 未定义时会触发遍历空值的告警。建议:

  • 将绑定值记录置于开关保护并默认打码;
  • 遍历前使用空数组回退,或仅在开关开启时展开参数。

可参考如下最小变更:

-            'db.sql.bindings' => $arguments['arguments']['bindings'] ?? [],
+            'db.sql.bindings' => $this->switcher->isTracingExtraTagEnable('db.bindings')
+                ? ($arguments['arguments']['bindings'] ?? [])
+                : '[redacted]',
@@
-        foreach ($arguments['arguments']['bindings'] as $key => $value) {
-            $data['db.parameter.' . $key] = $value;
-        }
+        if ($this->switcher->isTracingExtraTagEnable('db.bindings')) {
+            foreach (($arguments['arguments']['bindings'] ?? []) as $key => $value) {
+                $data['db.parameter.' . $key] = $value;
+            }
+        }

Also applies to: 101-103

src/sentry/src/Tracing/Aspect/RedisAspect.php (1)

61-73: Redis 参数可能包含敏感信息,建议加开关与打码

db.redis.parameters 直接写入全部调用参数,可能泄露令牌/账号等敏感值并导致日志体积膨胀。建议受开关控制并默认打码:

-            'db.redis.parameters' => $arguments['arguments'],
+            'db.redis.parameters' => $this->switcher->isTracingExtraTagEnable('redis.parameters')
+                ? $arguments['arguments']
+                : '[redacted]',
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)

76-85: 谨慎记录请求参数,避免敏感信息与日志放大

arguments 直接 JSON 化写入 span data,可能包含搜索条件、用户标识等敏感字段。建议置于开关、默认打码:

-            'arguments' => json_encode($proceedingJoinPoint->arguments['keys'], JSON_UNESCAPED_UNICODE),
+            'arguments' => $this->switcher->isTracingExtraTagEnable('elasticsearch.arguments')
+                ? json_encode($proceedingJoinPoint->arguments['keys'], JSON_UNESCAPED_UNICODE)
+                : '[redacted]',
src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)

50-56: 潜在空指针:无父 span 时注入请求头会报错

$parent = SentrySdk::getCurrentHub()->getSpan(); 可能为 null,随后调用 $parent->toTraceparent() 等会触发致命错误。最小修复:

-        $parent = SentrySdk::getCurrentHub()->getSpan();
-        $options['headers'] = $options['headers'] ?? [] + [
-            'sentry-trace' => $parent->toTraceparent(),
-            'baggage' => $parent->toBaggage(),
-            'traceparent' => $parent->toW3CTraceparent(),
-        ];
+        if ($parent = SentrySdk::getCurrentHub()->getSpan()) {
+            $options['headers'] = $options['headers'] ?? [] + [
+                'sentry-trace' => $parent->toTraceparent(),
+                'baggage' => $parent->toBaggage(),
+                'traceparent' => $parent->toW3CTraceparent(),
+            ];
+        }

或进一步将注入基于后续创建的 $span 以保证 trace 关联一致。

src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (1)

46-65: 空安全与标签统一改动到位,但建议对参数记录加保护

  • 使用空安全操作符与字符串化异常标签,改动妥当,LGTM。
  • annotation.arguments 可能包含业务参数与 PII,建议与 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;
+            $args = $proceedingJoinPoint->arguments['keys']['arguments'] ?? $proceedingJoinPoint->arguments['keys'];
+        } else {
+            $args = $proceedingJoinPoint->arguments['keys'];
+        }
+        $data['annotation.arguments'] = $this->switcher->isTracingExtraTagEnable('annotation.arguments') ? $args : '[redacted]';

Also applies to: 70-86

src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php (1)

74-80: 潜在空对象致命错误:$job 可能为 null 时使用 $job::class

当缺少 job 参数时,$job::class 会抛致命错误。建议先构造安全的描述字符串。

应用如下补丁:

-        $span = $this->startSpan(
-            op: 'queue.publish',
-            description: $job::class,
-            origin: 'auto.queue'
-        );
+        $desc = is_object($job) ? $job::class : (is_string($job) ? $job : get_debug_type($job));
+        $span = $this->startSpan(
+            op: 'queue.publish',
+            description: $desc,
+            origin: 'auto.queue'
+        );
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (4)

73-89: 先判空再注入头:当前先使用 $span 后再判空,存在 NPE 风险

startSpan() 返回 null,则随后 toTraceparent() 等会致命错误。

应用如下补丁,将判空提前:

-        // Inject trace context && Start span
-        $span = $this->startSpan(
+        // Inject trace context && Start span
+        $span = $this->startSpan(
             op: 'http.client',
             description: $request->getMethod() . ' ' . (string) $request->getUri(),
             origin: 'auto.http.client',
         );
-        $options['headers'] = array_replace($options['headers'] ?? [], [
-            'sentry-trace' => $span->toTraceparent(),
-            'baggage' => $span->toBaggage(),
-            'traceparent' => $span->toW3CTraceparent(),
-        ]);
-        // Override the headers
-        $proceedingJoinPoint->arguments['keys']['options']['headers'] = $options['headers'];
-
-        if (! $span) {
-            return $proceedingJoinPoint->process();
-        }
+        if (! $span) {
+            return $proceedingJoinPoint->process();
+        }
+        $options['headers'] = array_replace($options['headers'] ?? [], [
+            'sentry-trace' => $span->toTraceparent(),
+            'baggage' => $span->toBaggage(),
+            'traceparent' => $span->toW3CTraceparent(),
+        ]);
+        // Override the headers
+        $proceedingJoinPoint->arguments['keys']['options']['headers'] = $options['headers'];

97-116: 请求体大小计算不稳健:strlen($options['body']) 可能不是字符串

Guzzle 常用 StreamInterface;应优先用 $request->getBody()->getSize()

应用如下补丁(仅替换该字段):

-                'http.request.body.size' => strlen($options['body'] ?? ''),
+                'http.request.body.size' => (int) ($request->getBody()->getSize() ?? 0),

109-115: 潜在敏感信息外泄:直接记录请求头与 options

Authorization/Cookie 等应脱敏或按开关控制再上报。

应用如下补丁(移除默认上报,改为受开关控制并脱敏):

-                'http.request.headers' => $request->getHeaders(),
-                // Other
+                // Other
                 'coroutine.id' => Coroutine::id(),
                 'http.system' => 'guzzle',
                 'http.guzzle.config' => $guzzleConfig,
-                'http.guzzle.options' => $options ?? [],
                 'duration' => $stats->getTransferTime() * 1000, // in milliseconds
             ]);
+
+            if ($this->switcher->isTracingExtraTagEnable('http.request.headers')) {
+                $span->setData(['http.request.headers' => $this->sanitizeHeaders($request->getHeaders())]);
+            }
+            if ($this->switcher->isTracingExtraTagEnable('http.guzzle.options')) {
+                $span->setData(['http.guzzle.options' => $this->sanitizeOptions($options ?? [])]);
+            }

并在类中新增脱敏辅助方法:

protected function sanitizeHeaders(array $headers): array
{
    $sensitive = ['authorization','proxy-authorization','cookie','set-cookie'];
    $clean = [];
    foreach ($headers as $k => $v) {
        $lower = strtolower($k);
        $clean[$k] = in_array($lower, $sensitive, true) ? ['[redacted]'] : $v;
    }
    return $clean;
}

protected function sanitizeOptions(array $options): array
{
    // 简化处理:移除常见敏感键
    foreach (['auth','headers.authorization','headers.cookie'] as $key) {
        data_forget($options, $key);
    }
    return $options;
}

120-127: 响应头同样需要按开关并脱敏

Set-Cookie 等敏感头应避免默认上报。

应用如下补丁:

-                    'http.response.headers' => $response->getHeaders(),
+                    // 'http.response.headers' 按需在下方单独上报

并在紧随其后添加:

+                if ($this->switcher->isTracingExtraTagEnable('http.response.headers')) {
+                    $span->setData(['http.response.headers' => $this->sanitizeHeaders($response->getHeaders())]);
+                }
♻️ Duplicate comments (1)
src/sentry/src/Listener/EventHandleListener.php (1)

365-367: RequestTerminated 事件未必存在 exception 属性:增加兼容处理

与过往评论一致,需兼容不同事件实现,避免未定义属性错误。

应用如下补丁:

-        $this->captureException($event->exception);
+        $throwable = property_exists($event, 'exception')
+            ? $event->exception
+            : (method_exists($event, 'getThrowable') ? $event->getThrowable() : null);
+        $this->captureException($throwable);
🧹 Nitpick comments (9)
src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php (2)

92-101: 多次 setData 是否“覆盖”而非“合并”?建议一次性合并后再设置

当前对 RedisDriver 分支再次调用 setData(...),若底层为覆盖语义会丢失前面的数据。更稳妥做法:先合并数组,再调用一次 setData

应用如下补丁:

-            $span->setData([
-                'messaging.system' => 'async_queue',
-                'messaging.operation' => 'publish',
-                'messaging.message.id' => $messageId,
-                'messaging.message.body.size' => $bodySize,
-                'messaging.destination.name' => $destinationName,
-            ]);
-            if ($driver instanceof RedisDriver) {
-                $span->setData($this->buildSpanDataOfRedisDriver($driver));
-            }
+            $data = [
+                'messaging.system' => 'async_queue',
+                'messaging.operation' => 'publish',
+                'messaging.message.id' => $messageId,
+                'messaging.message.body.size' => $bodySize,
+                'messaging.destination.name' => $destinationName,
+            ];
+            if ($driver instanceof RedisDriver) {
+                $data += $this->buildSpanDataOfRedisDriver($driver);
+            }
+            $span->setData($data);

为避免误判,请确认底层 Span::setData 的语义是否为“合并”。如需我帮忙验证,请运行 web 检索或在仓库内查找实现。


162-165: 类型守卫缺失:$carrier 需校验为非空字符串

Carrier::fromJson() 期望字符串;数组/对象会触发类型错误。

应用如下补丁:

-        /** @var null|string $carrier */
-        if ($carrier) {
-            Context::set(Constants::TRACE_CARRIER, Carrier::fromJson($carrier));
-        }
+        /** @var null|string $carrier */
+        if (is_string($carrier) && $carrier !== '') {
+            Context::set(Constants::TRACE_CARRIER, Carrier::fromJson($carrier));
+        }
src/sentry/src/Listener/EventHandleListener.php (5)

243-261: foreach 引用变量未释放:避免后续误用引用

foreach (&$server) 结束后建议显式 unset($server)

应用如下补丁:

-        $this->config->set('server.servers', $servers);
+        unset($server);
+        $this->config->set('server.servers', $servers);

269-271: 消除静态分析告警:避免未使用占位变量 $_

改用 array_keys 可同时提升可读性。

应用如下补丁:

-        foreach ($this->config->get('redis', []) as $pool => $_) {
+        foreach (array_keys($this->config->get('redis', [])) as $pool) {
             $this->config->set("redis.{$pool}.event.enable", true);
         }

285-287: 时间单位不一致风险:DB 与 Redis 面包屑字段单位需统一

DB: executionTimeMs 未换算;Redis: duration 乘以 1000。建议统一为毫秒并命名一致,避免下游分析混淆。

Also applies to: 339-340


154-169: 可选:避免重复 flush

captureException() finally 中 flush,一些处理函数随后又 flushEvents(),可能重复。可只保留后者集中处理(含日志 flush)。

应用如下补丁(可选):

-        } finally {
-            $hub->getClient()?->flush();
-        }
+        } finally {
+            // 统一由 handle* 方法中的 Integration::flushEvents() 负责 flush
+        }

186-188: 形参未使用:按需在方法体内添加 /* @unused */ 注释或前缀下划线以消除告警

不影响行为,仅为静态分析友好。

Also applies to: 372-372, 397-397, 409-409, 421-421, 446-446, 458-458, 483-483, 495-495, 520-520, 532-532

src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (2)

56-94: 建议用 try/finally 确保异常时也能 finish span

当前用 tap(..., fn() => $span->finish()) 无法覆盖抛异常的路径,异常时 span 可能悬空未结束。用 finally 更稳健。

可在该方法内替换收尾逻辑:

-        return tap($proceedingJoinPoint->process(), fn () => $span->finish());
+        try {
+            return $proceedingJoinPoint->process();
+        } finally {
+            $span->finish();
+        }

96-124: 统一风格:sendBatchAsync 仍使用位置参数且描述不含 ();同时覆盖异常路径

为与 sendAsync 一致,建议改为命名参数并在描述中补 ();同时用 finally 保证异常也会 finish()`。

应用如下修改:

-        $span = $this->startSpan(
-            'kafka.send_batch',
-            sprintf('%s::%s', $proceedingJoinPoint->className, $proceedingJoinPoint->methodName),
-            origin: 'auto.kafka'
-        );
+        $span = $this->startSpan(
+            op: 'kafka.send_batch',
+            description: sprintf('%s::%s()', $proceedingJoinPoint->className, $proceedingJoinPoint->methodName),
+            origin: 'auto.kafka'
+        );
@@
-        return tap($proceedingJoinPoint->process(), fn () => $span->finish());
+        try {
+            return $proceedingJoinPoint->process();
+        } finally {
+            $span->finish();
+        }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93fca7f and f69d5d5.

📒 Files selected for processing (14)
  • .vscode/cspell.json (2 hunks)
  • src/sentry/src/Listener/EventHandleListener.php (1 hunks)
  • src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php (2 hunks)
  • src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1 hunks)
  • src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1 hunks)
  • src/sentry/src/Tracing/Aspect/DbAspect.php (1 hunks)
  • src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (3 hunks)
  • src/sentry/src/Tracing/Aspect/GrpcAspect.php (1 hunks)
  • src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (3 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 (1 hunks)
  • src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (2 hunks)
  • src/sentry/src/Tracing/Listener/EventHandleListener.php (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .vscode/cspell.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/sentry/src/Tracing/Listener/EventHandleListener.php
🧰 Additional context used
🧬 Code graph analysis (7)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
src/sentry/src/Switcher.php (1)
  • isTracingExtraTagEnable (53-56)
src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
src/sentry/src/Switcher.php (1)
  • isTracingExtraTagEnable (53-56)
src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (7)
src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
  • process (37-103)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
  • process (46-183)
src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
  • process (48-118)
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)
  • process (37-69)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
  • process (40-108)
src/sentry/src/Tracing/Aspect/DbAspect.php (1)
  • process (44-134)
src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
  • process (44-104)
src/sentry/src/Listener/EventHandleListener.php (4)
src/sentry/src/Integration.php (1)
  • Integration (28-158)
src/sentry/src/Switcher.php (3)
  • Switcher (19-85)
  • isTracingEnable (35-42)
  • isBreadcrumbEnable (30-33)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
src/sentry/src/Tracing/Listener/EventHandleListener.php (17)
  • EventHandleListener (68-716)
  • __construct (76-82)
  • listen (84-124)
  • process (126-161)
  • handleDbQueryExecuted (163-211)
  • handleRedisCommandExecuted (382-435)
  • handleRequestReceived (213-282)
  • handleCommandStarting (320-338)
  • handleCommandFinished (340-380)
  • handleAsyncQueueJobProcessing (638-657)
  • handleAsyncQueueJobProcessed (659-695)
  • handleCrontabTaskStarting (437-452)
  • handleCrontabTaskFinished (454-486)
  • handleAmqpMessageProcessing (488-517)
  • handleAmqpMessageProcessed (519-564)
  • handleKafkaMessageProcessing (566-595)
  • handleKafkaMessageProcessed (597-636)
src/sentry/src/Tracing/Aspect/RpcAspect.php (3)
src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
  • process (37-103)
src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
  • process (46-183)
src/sentry/src/Switcher.php (1)
  • isTracingExtraTagEnable (53-56)
src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (8)
src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
  • __construct (33-35)
src/sentry/src/Tracing/Aspect/TraceAnnotationAspect.php (1)
  • __construct (32-34)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
  • __construct (55-57)
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)
  • __construct (33-35)
src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
  • __construct (36-38)
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php (1)
  • __construct (38-40)
src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
  • __construct (40-42)
src/sentry/src/Switcher.php (2)
  • __construct (21-23)
  • Switcher (19-85)
src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
src/sentry/src/Switcher.php (1)
  • isTracingExtraTagEnable (53-56)
🪛 PHPMD (2.15.0)
src/sentry/src/Listener/EventHandleListener.php

188-188: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


269-269: Avoid unused local variables such as '$_'. (undefined)

(UnusedLocalVariable)


347-347: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


372-372: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


397-397: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


409-409: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


421-421: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


446-446: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


458-458: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


483-483: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


495-495: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


520-520: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


532-532: Avoid unused parameters such as '$event'. (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). (9)
  • 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 5.1.7
  • 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.2 with Swoole 5.1.7
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
🔇 Additional comments (11)
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)

55-59: 标准化异常标签写法合理

error 统一为字符串 'true' 且对 exception.code 显式转为字符串,符合 Sentry 对 tag 值为字符串的约定。其余流程不变,LGTM。

src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)

88-92: 异常标签一致性改动可取

errorexception.code 的类型标准化,保持与其他 Aspect 一致,LGTM。另:defer 中先 setSpan($transaction)finish() 的时序请确认是刻意设计(避免影响父子 span 关联)。

src/sentry/src/Tracing/Aspect/DbAspect.php (1)

116-121: 异常信息标签标准化 OK

error 设为 'true'exception.code 转字符串,保持一致性,LGTM。

src/sentry/src/Tracing/Aspect/RedisAspect.php (2)

85-89: 固定 op 为 db.redis 的命名请确认

从可观测语义看也可用 cache.redis 或更细化的 db.redis.<cmd>。若需与现有告警/查询保持一致,请确认仪表盘/告警规则是否已覆盖新 op。


100-113: 异常标签统一化 LGTM

异常标签与栈追踪的开关逻辑一致,改动正确。

src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)

97-101: 异常标签一致性改动 OK

errorexception.code 的类型标准化无误,LGTM。

src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)

86-96: 异常标签一致性改动 OK

统一字符串化,补充显式栈追踪开关判断,LGTM。

src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php (1)

64-67: OK:目的地上下文设置合并成多行写法,无副作用

与调用点一致,读取缺省值为 default,合理。

src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)

149-154: error 标签类型统一为字符串 OK,但请确认与下游查询兼容

若现网告警基于布尔类型,需同步规则。

src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (2)

39-41: 构造函数改动:LGTM

与项目内其他 Aspect 的写法一致,无功能性变化。


59-61: 确认 SpanStarter::startSpan 的命名参数签名(op / description / origin)

改为命名参数可读性更好,但若方法签名不含这三个参数名会在运行时触发 “Unknown named parameter” 致命错误。请在仓库根目录运行以下命令并贴回输出以便验证:

rg -nP 'trait\s+SpanStarter\b' -S
rg -nP 'function\s+startSpan\s*\(' -S
rg -nP '\bstartSpan\s*\(' -S
# 若无 rg: git grep -n "startSpan(" || true

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (1)
src/sentry/src/Listener/EventHandleListener.php (1)

360-362: RequestTerminated 事件可能不存在 exception 属性,需兼容处理

直接访问 $event->exception 在某些实现中会触发未定义属性错误;请做属性/方法探测并保持后续 flush 正常执行。

应用如下补丁:

-        $this->captureException($event->exception);
+        $throwable = property_exists($event, 'exception')
+            ? $event->exception
+            : (method_exists($event, 'getThrowable') ? $event->getThrowable() : null);
+        $this->captureException($throwable);
🧹 Nitpick comments (5)
src/sentry/src/Listener/EventHandleListener.php (4)

149-164: 重复 flush:captureException 内部 flush 与各 handler 的 flushEvents 重叠

当前 captureException() 的 finally 分支已调用 $hub->getClient()?->flush(),而各 handler 随后又调用 flushEvents(),存在重复 I/O。建议统一只在 handler 里 flushEvents()

         try {
             $hub->captureException($throwable);
         } catch (Throwable $e) {
             $this->container->get(StdoutLoggerInterface::class)->error((string) $e);
-        } finally {
-            $hub->getClient()?->flush();
-        }
+        }

238-256: 按引用遍历数组后建议显式释放引用

foreach ($servers as &$server) 结束后未 unset($server),易引入后续意外引用副作用(虽当前紧随 set 覆盖问题不大)。建议释放。

         foreach ($servers as &$server) {
             ...
         }
 
+        unset($server); // 释放引用
         $this->config->set('server.servers', $servers);

264-266: 消除静态分析“未使用变量”告警

此处 $_ 未使用;可直接遍历键名。

-        foreach ($this->config->get('redis', []) as $pool => $_) {
+        foreach (array_keys($this->config->get('redis', [])) as $pool) {
             $this->config->set("redis.{$pool}.event.enable", true);
         }

471-473: Crontab 失败事件的异常获取方式需做兼容

部分事件提供 getThrowable(),部分是公开属性 throwable。建议统一探测后再捕获。

-        $this->captureException($event->throwable);
+        $throwable = property_exists($event, 'throwable')
+            ? $event->throwable
+            : (method_exists($event, 'getThrowable') ? $event->getThrowable() : null);
+        $this->captureException($throwable);
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)

693-711: 冗余的 instanceof 判断

参数已类型限定为 Dispatched$dispatched instanceof Dispatched 判断多余。

-        if ($dispatched instanceof Dispatched && $dispatched->isFound()) {
+        if ($dispatched->isFound()) {
             $route = $dispatched->handler->route;
             ...
         }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f69d5d5 and a9f85df.

📒 Files selected for processing (2)
  • src/sentry/src/Listener/EventHandleListener.php (1 hunks)
  • src/sentry/src/Tracing/Listener/EventHandleListener.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sentry/src/Listener/EventHandleListener.php (3)
src/sentry/src/Integration.php (1)
  • Integration (28-158)
src/sentry/src/Switcher.php (3)
  • Switcher (19-85)
  • isTracingEnable (35-42)
  • isBreadcrumbEnable (30-33)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
src/sentry/src/Tracing/Listener/EventHandleListener.php (5)
src/sentry/src/Switcher.php (4)
  • Switcher (19-85)
  • isTracingSpanEnable (44-51)
  • isTracingEnable (35-42)
  • isTracingExtraTagEnable (53-56)
src/sentry/src/Util/Carrier.php (3)
  • Carrier (21-111)
  • getSentryTrace (68-71)
  • getBaggage (73-76)
src/sentry/src/Util/SqlParser.php (1)
  • SqlParser (20-65)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
src/sentry/src/Tracing/SpanStarter.php (3)
  • startSpan (33-52)
  • startRequestTransaction (54-77)
  • continueTrace (84-115)
🪛 PHPMD (2.15.0)
src/sentry/src/Listener/EventHandleListener.php

183-183: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


264-264: Avoid unused local variables such as '$_'. (undefined)

(UnusedLocalVariable)


342-342: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


367-367: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


392-392: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


404-404: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


416-416: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


441-441: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


453-453: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


478-478: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


490-490: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


515-515: Avoid unused parameters such as '$event'. (undefined)

(UnusedFormalParameter)


527-527: Avoid unused parameters such as '$event'. (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). (12)
  • GitHub Check: Test on PHP 8.2 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
  • 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.2 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.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
🔇 Additional comments (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)

458-465: Crontab 标签必须为 string — 统一将 setTags() 的值字符串化

原因:SDK 的 setTags() 要求 array<string,string>,当前有 bool/非-string 值会违反约定。
位置: src/sentry/src/Tracing/Listener/EventHandleListener.php 行 458–465

         $transaction->setTags([
             'crontab.rule' => $crontab->getRule(),
-            'crontab.type' => $crontab->getType(),
-            'crontab.options.is_single' => $crontab->isSingleton(),
-            'crontab.options.is_on_one_server' => $crontab->isOnOneServer(),
+            'crontab.type' => (string) $crontab->getType(),
+            'crontab.options.is_single' => $crontab->isSingleton() ? 'true' : 'false',
+            'crontab.options.is_on_one_server' => $crontab->isOnOneServer() ? 'true' : 'false',
         ]);

自动搜索未返回结果;人工确认仓内其他 setTags() 调用并一并字符串化。

@huangdijia huangdijia merged commit cd0d765 into main Sep 20, 2025
16 checks passed
@huangdijia huangdijia deleted the refactor/sentry-consolidate-listeners branch September 20, 2025 10:59
huangdijia added a commit that referenced this pull request Sep 20, 2025
…ner (#913)

* refactor(sentry): consolidate event listeners into unified EventListener

- Replace 11 separate listener classes with single EventListener class
- Consolidate all event handling logic into centralized match statement
- Merge functionality from deleted listeners:
  - AmqpExceptionListener, AsyncQueueExceptionListener
  - CaptureExceptionListener, CommandExceptionListener
  - CrontabExceptionListener, DbQueryListener
  - KafkaExceptionListener, RedisCommandExecutedListener
  - RequestExceptionListener, SetRedisEventEnableListener
  - SetRequestLifecycleListener
- Improve code maintainability and reduce duplication
- Preserve all existing functionality and event handling behavior

* feat(sentry): replace EventListener with EventHandleListener and update listener configuration

* feat(sentry): replace CronEventListener with EventHandleListener and update listener configuration

* feat(sentry): add EventHandleListener for unified event handling and remove deprecated listeners

* refactor(sentry): 删除多个过时的事件监听器以简化代码结构

* fix(sentry): 添加异常处理以记录 flushEvents 方法中的错误

* feat(sentry): 添加缓存追踪选项以增强性能监控

* refactor(sentry): 合并异步队列重试和失败处理方法,并添加事件参数注释

* refactor(sentry): 重命名处理 Redis 命令的方法以提高可读性

* refactor(sentry): 更新常量名称以反映 Sentry Hub 的上下文

* fix(sentry): 在处理异常时添加 flushEvents 方法调用以确保事件被正确刷新

* fix(sentry): 在异常处理时根据配置条件添加堆栈跟踪数据

* refactor(sentry): 优化事务数据和标签的设置逻辑,提升代码可读性

* refactor(sentry): 合并设置事务数据和标签的逻辑以提升代码简洁性

* refactor(sentry): 简化 Redis 和 AMQP 事件处理中的数据设置逻辑,提升代码可读性

* refactor(sentry): 重命名事件处理方法以提高可读性和一致性

* refactor(sentry): 重构事件处理逻辑,提取任务处理方法以提升代码可读性和维护性

* fix(EventHandleListener): 将命令退出代码转换为字符串以确保一致性

* refactor(sentry): 将错误标记的布尔值从 true 转换为字符串 'true' 以确保一致性

* fix: 将异常代码转换为字符串以确保一致性

* refactor(sentry): 优化上下文设置和数据处理逻辑以提高可读性

* refactor: 优化多个方面的代码结构和异常处理逻辑以提升可读性和一致性

* refactor: 重命名数据库查询处理函数以提高一致性和可读性

* refactor: 统一事件处理类中的命名空间以提高一致性和可读性

* refactor: 统一Redis事件处理类中的命名空间以提高一致性和可读性

* refactor: 更新捕获异常方法的参数类型提示以提高代码可读性

* refactor: 优化构造函数参数并简化异常处理中的日志记录

* refactor: 移除未使用的Redis命名空间导入以清理代码

* refactor: 将缺失的键集中到一个数组中以简化请求生命周期设置

---------

Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants