Skip to content

Conversation

@huangdijia
Copy link
Contributor

@huangdijia huangdijia commented Dec 24, 2025

Summary

  • Add enable_pool_metrics and enable_queue_metrics configuration options to allow independent control of pool and queue metrics collection
  • Add isPoolMetricsEnabled() and isQueueMetricsEnabled() methods to the Feature class for cleaner feature flag checks
  • Update PoolWatcher and QueueWatcher to use their respective enable flags instead of the general metrics flag

Changes

Configuration (sentry.php)

  • Added enable_pool_metrics option (env: SENTRY_ENABLE_POOL_METRICS, default: true)
  • Added enable_queue_metrics option (env: SENTRY_ENABLE_QUEUE_METRICS, default: true)

Feature Class

  • Added isPoolMetricsEnabled() method - checks if pool metrics are enabled
  • Added isQueueMetricsEnabled() method - checks if queue metrics are enabled

Metrics Listeners

  • Updated PoolWatcher to use isPoolMetricsEnabled() instead of isMetricsEnabled()
  • Updated QueueWatcher to use isQueueMetricsEnabled() instead of isMetricsEnabled()
  • Moved MetricFactoryReady event dispatch earlier in OnCoroutineServerStart for proper initialization
  • Removed unnecessary Timer::STOP callback logic from timer tick handlers
  • Refactored timer tick callback formatting for consistency across listeners

Motivation

This change allows users to enable/disable pool and queue metrics independently from the general metrics settings. Previously, all metric types were controlled by the same flags, which lacked granular control. Users may want to collect default and command metrics but not pool/queue metrics, or vice versa.

Test plan

  • Verify pool metrics can be enabled/disabled independently via config
  • Verify queue metrics can be enabled/disabled independently via config
  • Verify default metrics still work when pool/queue metrics are disabled
  • Test with coroutine server (Swoole/Swow)
  • Test with worker server
  • Test command metrics collection

Summary by CodeRabbit

发布说明

  • 新功能

    • 新增池指标与队列指标开关,默认启用,可通过环境变量控制。
  • 重构

    • 指标采集与发布流程重构:在定时采集回调中统一收集并一次性刷新指标,简化计时器控制流并提升稳定性。
    • 池与队列监控使用独立开关,队列指标按队列汇总后统一发送。

✏️ Tip: You can customize this high-level summary in your review settings.

…etrics

- Add enable_pool_metrics and enable_queue_metrics config options
- Add isPoolMetricsEnabled() and isQueueMetricsEnabled() methods to Feature class
- Update PoolWatcher and QueueWatcher to use their respective enable flags
- Move MetricFactoryReady event dispatch earlier in OnCoroutineServerStart for proper initialization
- Remove Timer::STOP callback logic from timer tick handlers (no longer needed)
- Refactor timer tick callback formatting for consistency
Copilot AI review requested due to automatic review settings December 24, 2025 04:40
@coderabbitai
Copy link

coderabbitai bot commented Dec 24, 2025

Warning

Rate limit exceeded

@huangdijia has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 37 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 7504512 and 1ce1191.

📒 Files selected for processing (3)
  • src/sentry/src/Factory/ClientBuilderFactory.php
  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

为指标收集引入两个新配置(enable_pool_metrics、enable_queue_metrics),在 Feature 中添加对应检查方法,并重构多处指标监听器的计时器回调与 MetricFactoryReady 派发,移除基于 isClosing 的早停逻辑,将指标采集与 flush 集中于回调内。

Changes

Cohort / File(s) 变更说明
配置扩展
src/sentry/publish/sentry.php
新增配置项 enable_pool_metricsenable_queue_metrics(从环境变量读取,默认 true)
功能检查方法
src/sentry/src/Feature.php
新增 isPoolMetricsEnabled(bool $default = true)isQueueMetricsEnabled(bool $default = true),在总体 metrics 开关通过后读取对应配置
OnBeforeHandle — 容器注入与事件分发
src/sentry/src/Metrics/Listener/OnBeforeHandle.php
构造函数新增 ContainerInterface 注入;在启动处通过容器获取 EventDispatcher 并派发 MetricFactoryReady;将指标采集与 flush 合并至计时器回调内
OnCoroutineServerStart — 条件化分发与回调内联
src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
MetricFactoryReady 派发移入 metrics 启用判断;移除早期退出守卫;将统计采集与 flush 内联到计时器回调中
OnMetricFactoryReady — 回调集中化
src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
移除 isClosing 早停;在单一计时器回调内集中采集协程/Timer/可选 server/Swoole 统计、负载与内存指标,并在末端 flush
OnWorkerStart — 有条件派发与回调简化
src/sentry/src/Metrics/Listener/OnWorkerStart.php
仅当 workerId === 0 且 metrics 启用时派发 MetricFactoryReady;计时器回调不再基于关闭返回 STOP,保留采集与 flush
PoolWatcher — 功能门控与回调精简
src/sentry/src/Metrics/Listener/PoolWatcher.php
将门控改为 isPoolMetricsEnabled();移除基于关闭的 STOP;在回调内发出连接相关 gauges 并一次性 flush
QueueWatcher — 队列门控与批量 flush
src/sentry/src/Metrics/Listener/QueueWatcher.php
将门控改为 isQueueMetricsEnabled();移除 STOP-on-close;回调内读取队列配置,按队列发出四类 gauge(waiting/delayed/failed/timeout),并在处理完所有队列后单次 metrics()->flush()

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Worker as Worker/Process
    participant OnBefore as OnBeforeHandle
    participant Container as Container
    participant Dispatcher as EventDispatcher
    participant FactoryReady as MetricFactoryReady Event
    participant OnMetric as OnMetricFactoryReady
    participant Pool as PoolWatcher
    participant Queue as QueueWatcher
    participant Metrics as Metrics System

    Worker->>OnBefore: 启动/进入处理
    OnBefore->>Container: 获取 EventDispatcher
    OnBefore->>Dispatcher: Dispatch MetricFactoryReady
    Dispatcher->>OnMetric: 通知 MetricFactoryReady 监听器
    Dispatcher->>Pool: 通知 PoolWatcher(若 isPoolMetricsEnabled)
    Dispatcher->>Queue: 通知 QueueWatcher(若 isQueueMetricsEnabled)

    loop 周期性 tick (OnMetric)
        OnMetric->>Metrics: 收集 Co::stats / Timer::stats
        alt serverStatsFactory 存在
            OnMetric->>Metrics: 收集 server stats
        end
        alt Swoole\Timer 存在
            OnMetric->>Metrics: 收集 Swoole Timer stats
        end
        OnMetric->>Metrics: 发出负载/内存 gauges
        OnMetric->>Metrics: Flush
    end

    loop 周期性 tick (PoolWatcher)
        Pool->>Metrics: 发出连接相关 gauges
        Pool->>Metrics: Flush
    end

    loop 周期性 tick (QueueWatcher)
        Queue->>Config: 读取 async_queue 列表
        loop 每个队列
            Queue->>Metrics: 发出 waiting/delayed/failed/timeout gauges
        end
        Queue->>Metrics: Flush(一次)
    end
Loading

Estimated code review effort

🎯 3 (中等) | ⏱️ ~25 分钟

Possibly related PRs

🐰✨ 新标志分两路,池与队各自护,
事件起时一起走,回调内里刷数据,
兔儿轻跳庆改动,仪表盘上乐无数! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.45% 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 标题准确概括了本次变更的主要目的——为池和队列指标添加独立的配置选项,与更改内容的核心目标完全相符。

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

🧹 Nitpick comments (6)
src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php (1)

97-118: 考虑使用匿名占位符或注解来抑制 PHPMD 警告。

$isClosing 参数是 Timer::tick 回调签名的一部分,由 Hyperf 框架提供。既然此 PR 移除了基于 $isClosingTimer::STOP 逻辑,可以考虑使用下划线前缀或 @SuppressWarnings 注解来明确表示该参数被有意忽略。

🔎 建议的修复
         $this->timer->tick(
             $this->feature->getMetricsInterval(),
-            function ($isClosing = false) use ($metrics) {
+            function ($_isClosing = false) use ($metrics) {
                 $this->trySet('gc_', $metrics, gc_status());
src/sentry/src/Metrics/Listener/OnWorkerStart.php (1)

91-124: 实现一致,与其他监听器保持统一风格。

Timer 回调的重构与其他监听器(如 OnCoroutineServerStartOnBeforeHandle)保持一致。同样建议使用 $_isClosing 来抑制 PHPMD 警告。

src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php (1)

98-134: 重构良好:统一的指标收集和刷新逻辑。

将所有指标收集和 flush() 调用整合到单个回调中,使代码更易维护。移除 Timer::STOP 逻辑与其他监听器保持一致。

同样建议使用 $_isClosing 来抑制静态分析警告。

src/sentry/src/Metrics/Listener/OnBeforeHandle.php (1)

98-119: Timer 回调重构一致。

与其他监听器保持一致的实现风格。同样建议使用 $_isClosing 来抑制 PHPMD 警告。

src/sentry/src/Metrics/Listener/PoolWatcher.php (1)

64-94: 池指标收集逻辑清晰。

收集连接池的三个关键指标(使用中、等待中、最大连接数)并在最后统一刷新,实现简洁明了。同样建议使用 $_isClosing 抑制警告。

src/sentry/src/Metrics/Listener/QueueWatcher.php (1)

57-62: 考虑在回调外部缓存队列配置。

每次 tick 都从 ConfigInterface 读取队列配置可能不是必要的,因为 async_queue 配置在运行时通常不会改变。可以考虑将配置读取移到 process() 方法中,在 timer 启动前获取队列列表。

另外,如果 DriverFactory::get() 找不到对应的驱动,可能会抛出异常。

🔎 建议的优化
     public function process(object $event): void
     {
         if (! $this->feature->isQueueMetricsEnabled()) {
             return;
         }

+        $config = $this->container->get(ConfigInterface::class);
+        $queues = array_keys($config->get('async_queue', []));
+
+        if (empty($queues)) {
+            return;
+        }
+
+        $driverFactory = $this->container->get(DriverFactory::class);
+
         $this->timer->tick(
             $this->feature->getMetricsInterval(),
-            function ($isClosing = false) {
-                $config = $this->container->get(ConfigInterface::class);
-                $queues = array_keys($config->get('async_queue', []));
-
+            function ($_isClosing = false) use ($queues, $driverFactory) {
                 foreach ($queues as $name) {
-                    $queue = $this->container->get(DriverFactory::class)->get($name);
+                    $queue = $driverFactory->get($name);
                     $info = $queue->info();
📜 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 ba4f3df and a67ffa8.

📒 Files selected for processing (8)
  • src/sentry/publish/sentry.php
  • src/sentry/src/Feature.php
  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
  • src/sentry/src/Metrics/Listener/PoolWatcher.php
  • src/sentry/src/Metrics/Listener/QueueWatcher.php
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.php: All PHP files must include declare(strict_types=1) at the top
Use PSR-12 coding standards for PHP code formatting
Use 4-space indentation with short array syntax in PHP code
Use .php-cs-fixer.php configuration for PHP code formatting

All PHP files must include a strict_types declaration at the top

**/*.php: Follow project PHP coding standard enforced by php-cs-fixer with PSR-12 style, 4-space indentation, and short array syntax
Maintain type coverage by updating or adding tests when public APIs change; ensure composer test:types stays green before pushing

Files:

  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/publish/sentry.php
  • src/sentry/src/Metrics/Listener/PoolWatcher.php
  • src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
  • src/sentry/src/Metrics/Listener/QueueWatcher.php
  • src/sentry/src/Feature.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
src/**/src/**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use namespace pattern FriendsOfHyperf\{ComponentName} for all component classes

Files:

  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/PoolWatcher.php
  • src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
  • src/sentry/src/Metrics/Listener/QueueWatcher.php
  • src/sentry/src/Feature.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
src/*/src/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/*/src/**/*.php: Namespace convention for components must follow FriendsOfHyperf{ComponentName}
All code must be coroutine-safe and avoid global state without proper context management, blocking I/O operations, and non-coroutine-safe third-party libraries without wrappers
Use Hyperf's Context API for request-scoped data instead of global state
Follows PSR-12 coding standards and use PHP-CS-Fixer for automatic formatting
Use PHPStan at maximum level for static analysis
Ensure component namespace doesn't conflict with existing Hyperf components or other packages in the ecosystem
Integrate deeply with Hyperf's Dependency Injection container for service registration
Leverage Hyperf's AOP (Aspect-Oriented Programming) for cross-cutting concerns via aspects defined in ConfigProvider
Use Hyperf's Event System to register listeners for framework events in components
All code must support coroutine-based concurrency using Swoole/Swow compatibility

Files:

  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/PoolWatcher.php
  • src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
  • src/sentry/src/Metrics/Listener/QueueWatcher.php
  • src/sentry/src/Feature.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
src/**/*.php

📄 CodeRabbit inference engine (AGENTS.md)

Each component lives in its own subdirectory with PSR-4 namespaces matching FriendsOfHyperf\*

Files:

  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/publish/sentry.php
  • src/sentry/src/Metrics/Listener/PoolWatcher.php
  • src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
  • src/sentry/src/Metrics/Listener/QueueWatcher.php
  • src/sentry/src/Feature.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
🧠 Learnings (1)
📚 Learning: 2025-12-12T05:33:43.947Z
Learnt from: CR
Repo: friendsofhyperf/components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-12T05:33:43.947Z
Learning: Applies to src/*/src/**/*.php : Use Hyperf's Event System to register listeners for framework events in components

Applied to files:

  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
🧬 Code graph analysis (4)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php (4)
src/sentry/src/Metrics/Event/MetricFactoryReady.php (2)
  • MetricFactoryReady (14-19)
  • __construct (16-18)
src/sentry/src/Metrics/Timer.php (1)
  • Timer (18-62)
src/sentry/src/Function.php (1)
  • metrics (30-33)
src/sentry/src/Metrics/Traits/MetricSetter.php (1)
  • trySet (20-33)
src/sentry/src/Metrics/Listener/PoolWatcher.php (1)
src/sentry/src/Feature.php (2)
  • isPoolMetricsEnabled (57-64)
  • getMetricsInterval (75-84)
src/sentry/src/Metrics/Listener/QueueWatcher.php (2)
src/sentry/src/Function.php (2)
  • feature (25-28)
  • metrics (30-33)
src/sentry/src/Feature.php (2)
  • isQueueMetricsEnabled (66-73)
  • getMetricsInterval (75-84)
src/sentry/src/Metrics/Listener/OnWorkerStart.php (5)
src/sentry/src/Function.php (2)
  • feature (25-28)
  • metrics (30-33)
src/sentry/src/Feature.php (2)
  • isMetricsEnabled (34-37)
  • getMetricsInterval (75-84)
src/sentry/src/Metrics/Event/MetricFactoryReady.php (1)
  • MetricFactoryReady (14-19)
src/telescope/src/Server/Server.php (1)
  • Server (19-21)
src/sentry/src/Metrics/Traits/MetricSetter.php (1)
  • trySet (20-33)
🪛 PHPMD (2.15.0)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php

100-100: Avoid unused parameters such as '$isClosing'. (undefined)

(UnusedFormalParameter)

src/sentry/src/Metrics/Listener/PoolWatcher.php

66-66: Avoid unused parameters such as '$isClosing'. (undefined)

(UnusedFormalParameter)

src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php

99-99: Avoid unused parameters such as '$isClosing'. (undefined)

(UnusedFormalParameter)

src/sentry/src/Metrics/Listener/QueueWatcher.php

56-56: Avoid unused parameters such as '$isClosing'. (undefined)

(UnusedFormalParameter)

src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php

100-100: Avoid unused parameters such as '$isClosing'. (undefined)

(UnusedFormalParameter)

src/sentry/src/Metrics/Listener/OnWorkerStart.php

93-93: Avoid unused parameters such as '$isClosing'. (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). (20)
  • GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
  • GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
  • 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.8
  • GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
  • 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.1.3
  • GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
  • GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
  • 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.3 with Swoole 6.0.2
  • GitHub Check: Agent
  • GitHub Check: Seer Code Review
🔇 Additional comments (9)
src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php (1)

59-62: 逻辑正确:MetricFactoryReady 分发现在有条件地进行。

将事件分发包装在 isMetricsEnabled() 检查中是正确的做法,这确保了只有在启用指标时才会触发后续的监听器(如 OnMetricFactoryReady)。

src/sentry/src/Metrics/Listener/OnWorkerStart.php (1)

52-56: 逻辑合理:仅在 workerId 为 0 时分发 MetricFactoryReady。

限制在 workerId == 0 时分发事件可以避免在多 worker 环境中重复触发 OnMetricFactoryReady 监听器,这是一个合理的优化。

src/sentry/src/Metrics/Listener/OnBeforeHandle.php (2)

34-39: 构造函数正确注入 ContainerInterface。

添加 ContainerInterface 依赖以获取 EventDispatcherInterface 是合理的做法,符合 Hyperf 的依赖注入模式。根据 learnings,使用 Hyperf 的事件系统来注册和触发框架事件是推荐的做法。


64-66: 命令处理期间正确分发 MetricFactoryReady 事件。

这确保了命令执行时 OnMetricFactoryReady 监听器能够被触发,从而启用系统级指标收集。

src/sentry/src/Metrics/Listener/PoolWatcher.php (1)

60-62: 正确使用新的 isPoolMetricsEnabled() 方法。

isMetricsEnabled() 切换到 isPoolMetricsEnabled() 实现了 PR 的目标——允许用户独立控制池指标的启用/禁用。

src/sentry/publish/sentry.php (1)

68-69: 配置项添加正确,保持向后兼容。

新增的 enable_pool_metricsenable_queue_metrics 配置项:

  • 命名与现有的 enable_default_metricsenable_command_metrics 保持一致
  • 默认值为 true 确保向后兼容
  • 环境变量命名遵循 SENTRY_ENABLE_* 约定
src/sentry/src/Feature.php (1)

57-73: 新方法实现正确,与现有模式一致。

isPoolMetricsEnabled()isQueueMetricsEnabled() 方法:

  • 遵循与 isDefaultMetricsEnabled()isCommandMetricsEnabled() 相同的模式
  • 正确地先检查 isMetricsEnabled() 再读取特定配置
  • 支持 $default 参数保持 API 一致性

这种分层检查确保了当全局指标被禁用时,子级指标也会被禁用。

src/sentry/src/Metrics/Listener/QueueWatcher.php (2)

50-52: 正确使用新的 isQueueMetricsEnabled() 方法。

切换到 isQueueMetricsEnabled() 实现了队列指标的独立控制。


64-86: 队列指标收集逻辑完整。

收集 waitingdelayedfailedtimeout 四个关键指标,并在处理完所有队列后统一刷新,符合批量操作的最佳实践。

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds granular configuration controls for pool and queue metrics collection in the Sentry integration. The changes allow users to independently enable or disable pool metrics and queue metrics, which were previously controlled only by the general metrics flag. This provides more flexibility for users who want to collect some metrics types but not others.

Key changes:

  • Added two new configuration options (enable_pool_metrics and enable_queue_metrics) with corresponding environment variables
  • Introduced isPoolMetricsEnabled() and isQueueMetricsEnabled() methods in the Feature class that check both the general metrics flag and their specific flags
  • Refactored all metrics listeners to use specific enable flags and improved code formatting consistency

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/sentry/publish/sentry.php Added enable_pool_metrics and enable_queue_metrics configuration options with environment variable support
src/sentry/src/Feature.php Added isPoolMetricsEnabled() and isQueueMetricsEnabled() methods with proper hierarchical checks
src/sentry/src/Metrics/Listener/QueueWatcher.php Updated to use isQueueMetricsEnabled() instead of isMetricsEnabled() and reformatted timer callback
src/sentry/src/Metrics/Listener/PoolWatcher.php Updated to use isPoolMetricsEnabled() instead of isMetricsEnabled() and reformatted timer callback
src/sentry/src/Metrics/Listener/OnWorkerStart.php Moved MetricFactoryReady event dispatch earlier and reformatted timer callback
src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php Removed unused Timer::STOP logic and reformatted timer callback for consistency
src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php Moved MetricFactoryReady dispatch inside metrics check and reformatted timer callback
src/sentry/src/Metrics/Listener/OnBeforeHandle.php Added MetricFactoryReady event dispatch and reformatted timer callback for consistency

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 (4)
src/sentry/src/Metrics/Listener/OnWorkerStart.php (1)

91-124: LGTM - 重构正确完成。

计时器回调已正确重构,将所有指标收集逻辑集中在回调内部,并在最后统一调用 flush()。之前审查中提到的移除未使用的 $isClosing 参数的建议已经实现。

src/sentry/src/Metrics/Listener/QueueWatcher.php (1)

54-88: LGTM - 回调重构合理。

计时器回调已正确重构:

  • 移除了未使用的 $isClosing 参数(已解决之前的审查意见)
  • 将配置读取移至回调内部
  • 为每个队列发送指标后统一调用 flush()

逻辑清晰且符合其他监听器的重构模式。

src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php (1)

98-134: LGTM - 指标收集逻辑全面整合。

计时器回调的重构非常全面:

  • 移除了未使用的 $isClosing 参数(已解决之前的审查意见)
  • 将所有指标收集集中在回调内部,包括协程统计、计时器统计、服务器统计、系统负载和内存使用情况
  • 正确处理了可选组件(serverStatsFactory、Swoole\Timer)的条件检查
  • 在回调结束时统一调用 flush()

这种集中化的方式提高了代码的可维护性和一致性。

src/sentry/src/Metrics/Listener/OnBeforeHandle.php (1)

98-119: LGTM - 回调重构与其他监听器保持一致。

计时器回调的重构正确:

  • 移除了未使用的 $isClosing 参数(已解决之前的审查意见)
  • 将指标收集(gc_statusgetrusage、内存使用情况)集中在回调内部
  • 在命令上下文中使用硬编码的 worker => '0' 是合理的
  • 在回调结束时统一调用 flush()
🧹 Nitpick comments (1)
src/sentry/src/Metrics/Listener/OnWorkerStart.php (1)

52-56: 建议使用严格比较运算符。

Line 52 对 workerId 使用了 == 进行比较。由于 workerId 是整数类型,建议使用 === 进行严格比较以避免类型转换。

🔎 建议的修复
-        if ($this->feature->isMetricsEnabled() && $event->workerId == 0) {
+        if ($this->feature->isMetricsEnabled() && $event->workerId === 0) {
📜 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 1ee1f99 and 7504512.

📒 Files selected for processing (6)
  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
  • src/sentry/src/Metrics/Listener/PoolWatcher.php
  • src/sentry/src/Metrics/Listener/QueueWatcher.php
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/sentry/src/Metrics/Listener/PoolWatcher.php
  • src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.php: All PHP files must include declare(strict_types=1) at the top
Use PSR-12 coding standards for PHP code formatting
Use 4-space indentation with short array syntax in PHP code
Use .php-cs-fixer.php configuration for PHP code formatting

All PHP files must include a strict_types declaration at the top

**/*.php: Follow project PHP coding standard enforced by php-cs-fixer with PSR-12 style, 4-space indentation, and short array syntax
Maintain type coverage by updating or adding tests when public APIs change; ensure composer test:types stays green before pushing

Files:

  • src/sentry/src/Metrics/Listener/QueueWatcher.php
  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
src/**/src/**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use namespace pattern FriendsOfHyperf\{ComponentName} for all component classes

Files:

  • src/sentry/src/Metrics/Listener/QueueWatcher.php
  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
src/*/src/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/*/src/**/*.php: Namespace convention for components must follow FriendsOfHyperf{ComponentName}
All code must be coroutine-safe and avoid global state without proper context management, blocking I/O operations, and non-coroutine-safe third-party libraries without wrappers
Use Hyperf's Context API for request-scoped data instead of global state
Follows PSR-12 coding standards and use PHP-CS-Fixer for automatic formatting
Use PHPStan at maximum level for static analysis
Ensure component namespace doesn't conflict with existing Hyperf components or other packages in the ecosystem
Integrate deeply with Hyperf's Dependency Injection container for service registration
Leverage Hyperf's AOP (Aspect-Oriented Programming) for cross-cutting concerns via aspects defined in ConfigProvider
Use Hyperf's Event System to register listeners for framework events in components
All code must support coroutine-based concurrency using Swoole/Swow compatibility

Files:

  • src/sentry/src/Metrics/Listener/QueueWatcher.php
  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
src/**/*.php

📄 CodeRabbit inference engine (AGENTS.md)

Each component lives in its own subdirectory with PSR-4 namespaces matching FriendsOfHyperf\*

Files:

  • src/sentry/src/Metrics/Listener/QueueWatcher.php
  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/OnWorkerStart.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
🧠 Learnings (1)
📚 Learning: 2025-12-12T05:33:43.947Z
Learnt from: CR
Repo: friendsofhyperf/components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-12T05:33:43.947Z
Learning: Applies to src/*/src/**/*.php : Use Hyperf's Event System to register listeners for framework events in components

Applied to files:

  • src/sentry/src/Metrics/Listener/OnBeforeHandle.php
  • src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
🧬 Code graph analysis (4)
src/sentry/src/Metrics/Listener/QueueWatcher.php (2)
src/sentry/src/Function.php (2)
  • feature (25-28)
  • metrics (30-33)
src/sentry/src/Feature.php (2)
  • isQueueMetricsEnabled (66-73)
  • getMetricsInterval (75-84)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php (5)
src/sentry/src/Metrics/Event/MetricFactoryReady.php (2)
  • MetricFactoryReady (14-19)
  • __construct (16-18)
src/sentry/src/Metrics/Timer.php (1)
  • Timer (18-62)
src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php (1)
  • __construct (34-39)
src/sentry/src/Metrics/Listener/OnWorkerStart.php (1)
  • __construct (33-38)
src/sentry/src/Metrics/Traits/MetricSetter.php (1)
  • trySet (20-33)
src/sentry/src/Metrics/Listener/OnWorkerStart.php (5)
src/sentry/src/Function.php (2)
  • feature (25-28)
  • metrics (30-33)
src/sentry/src/Feature.php (2)
  • isMetricsEnabled (34-37)
  • getMetricsInterval (75-84)
src/sentry/src/Metrics/Event/MetricFactoryReady.php (1)
  • MetricFactoryReady (14-19)
src/telescope/src/Server/Server.php (1)
  • Server (19-21)
src/sentry/src/Metrics/Traits/MetricSetter.php (1)
  • trySet (20-33)
src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php (4)
src/sentry/src/Function.php (2)
  • feature (25-28)
  • metrics (30-33)
src/sentry/src/Feature.php (1)
  • getMetricsInterval (75-84)
src/sentry/src/Metrics/Traits/MetricSetter.php (1)
  • trySet (20-33)
src/sentry/src/Metrics/Timer.php (1)
  • Timer (18-62)
⏰ 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). (3)
  • 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: Seer Code Review
🔇 Additional comments (4)
src/sentry/src/Metrics/Listener/QueueWatcher.php (1)

50-52: LGTM - 特性门控更新正确。

将特性检查从 isMetricsEnabled() 更改为 isQueueMetricsEnabled(),这与 PR 目标一致,实现了队列指标的独立开关控制。

src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php (1)

20-20: LGTM - 别名使用符合 Hyperf 惯例。

Hyperf\Engine\Coroutine 添加 Co 别名是 Hyperf 生态系统中的惯用做法,使代码更简洁。

src/sentry/src/Metrics/Listener/OnBeforeHandle.php (2)

16-16: LGTM - 依赖注入正确实现。

添加 ContainerInterface 到构造函数以及相关的导入语句是正确的。这遵循了 Hyperf 的依赖注入模式,使得监听器能够访问事件分发器。

根据学习内容:Hyperf 组件应使用事件系统注册监听器,此更改符合该指导原则。

Also applies to: 22-23, 34-37


64-65: LGTM - 事件分发位置合理。

在设置 Constants::$runningInCommand 后立即分发 MetricFactoryReady 事件,使得下游监听器(如 PoolWatcherQueueWatcher)能够开始收集指标。这与 OnWorkerStart 中的模式保持一致。

@huangdijia huangdijia merged commit 494d48b into main Dec 24, 2025
22 checks passed
@huangdijia huangdijia deleted the feat/sentry-separate-metrics-config branch December 24, 2025 05:04
huangdijia added a commit that referenced this pull request Dec 24, 2025
…etrics (#1053)

* feat(sentry): add separate configuration options for pool and queue metrics

- Add enable_pool_metrics and enable_queue_metrics config options
- Add isPoolMetricsEnabled() and isQueueMetricsEnabled() methods to Feature class
- Update PoolWatcher and QueueWatcher to use their respective enable flags
- Move MetricFactoryReady event dispatch earlier in OnCoroutineServerStart for proper initialization
- Remove Timer::STOP callback logic from timer tick handlers (no longer needed)
- Refactor timer tick callback formatting for consistency

* fix(sentry): alias Coroutine class for consistency in metric processing

* fix(metrics): remove unused parameter from timer callback functions

* fix(metrics): check for EventDispatcherInterface existence before dispatching MetricFactoryReady event

* fix(metrics): ensure command metrics are enabled before dispatching MetricFactoryReady event

* feat(sentry): add enable_queue_metrics and enable_pool_metrics options to ClientBuilderFactory

---------

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