feat: enhance Sentry metrics with configurable default metrics collection #1034
Conversation
…tion - Add SENTRY_ENABLE_DEFAULT_METRICS configuration option - Implement isDefaultMetricsEnabled() method in Feature class - Extend OnBeforeHandle listener to collect command-line metrics - Add worker identification tags to memory metrics - Separate default metrics from custom metrics collection - Add Timer-based periodic metrics collection for commands - Include garbage collection and resource usage metrics This enhancement provides more granular control over metrics collection and extends monitoring capabilities to command-line operations.
|
Warning Rate limit exceeded@huangdijia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (2)
Walkthrough添加两个公开配置键 Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant OnBeforeHandle
participant Timer
participant MetricSetter
participant MetricsBackend
Worker->>OnBeforeHandle: BeforeHandle 事件
OnBeforeHandle->>OnBeforeHandle: 设置运行中命令标记
alt enable_command_metrics == true
OnBeforeHandle->>Timer: 启动/注册周期 tick 回调
Timer->>MetricSetter: 周期触发采集
MetricSetter->>MetricsBackend: 收集并上报指标 (memory/gc/ru...)
MetricsBackend-->>MetricSetter: 确认或缓存
OnBeforeHandle->>Worker: 注册退出时刷新/清理动作
end
Worker->>OnBeforeHandle: AfterExecute 事件
OnBeforeHandle->>OnBeforeHandle: 清理命令标记与停止 Timer（如存在）
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
庆贺诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/sentry/src/Metrics/Traits/MetricSetter.php (1)
34-37: 移除无用的注释占位代码。注释掉的
spawnDefaultMetrics方法没有实际用途,会增加代码维护负担。如果有未来计划,建议用 TODO 注释说明,否则请移除。- - // protected function spawnDefaultMetrics() - // { - // }src/sentry/publish/sentry.php (1)
45-45: 建议添加文档注释。其他配置项如
enable_metrics都有@see注释链接到文档,建议为enable_default_metrics也添加相应说明,便于用户理解其用途。// @see: https://docs.sentry.io/platforms/php/guides/laravel/configuration/options/#enable_metrics 'enable_metrics' => env('SENTRY_ENABLE_METRICS', true), + // Controls default system metrics collection (memory, GC, resource usage) 'enable_default_metrics' => env('SENTRY_ENABLE_DEFAULT_METRICS', true),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/sentry/publish/sentry.php(1 hunks)src/sentry/src/Factory/ClientBuilderFactory.php(1 hunks)src/sentry/src/Feature.php(1 hunks)src/sentry/src/Metrics/Listener/OnBeforeHandle.php(2 hunks)src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php(2 hunks)src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php(1 hunks)src/sentry/src/Metrics/Listener/OnWorkerStart.php(1 hunks)src/sentry/src/Metrics/Traits/MetricSetter.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/*/src/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All components use the namespace pattern:
FriendsOfHyperf\{ComponentName}
Files:
src/sentry/src/Metrics/Traits/MetricSetter.phpsrc/sentry/src/Factory/ClientBuilderFactory.phpsrc/sentry/src/Metrics/Listener/OnWorkerStart.phpsrc/sentry/src/Metrics/Listener/OnCoroutineServerStart.phpsrc/sentry/src/Metrics/Listener/OnMetricFactoryReady.phpsrc/sentry/src/Feature.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.php
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Follow PSR-12 coding standards for all PHP code
Use PHP-CS-Fixer for code formatting as configured in.php-cs-fixer.php
Use PHPStan for static analysis as configured inphpstan.neon.dist
Files:
src/sentry/src/Metrics/Traits/MetricSetter.phpsrc/sentry/src/Factory/ClientBuilderFactory.phpsrc/sentry/src/Metrics/Listener/OnWorkerStart.phpsrc/sentry/src/Metrics/Listener/OnCoroutineServerStart.phpsrc/sentry/src/Metrics/Listener/OnMetricFactoryReady.phpsrc/sentry/src/Feature.phpsrc/sentry/publish/sentry.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.php
🧬 Code graph analysis (3)
src/sentry/src/Metrics/Listener/OnWorkerStart.php (2)
src/sentry/src/Function.php (1)
feature(25-28)src/sentry/src/Feature.php (1)
isDefaultMetricsEnabled(39-46)
src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php (2)
src/sentry/src/Function.php (2)
feature(25-28)metrics(30-33)src/sentry/src/Feature.php (1)
isDefaultMetricsEnabled(39-46)
src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php (2)
src/sentry/src/Function.php (1)
feature(25-28)src/sentry/src/Feature.php (1)
isDefaultMetricsEnabled(39-46)
⏰ 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). (17)
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- 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 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
🔇 Additional comments (6)
src/sentry/src/Factory/ClientBuilderFactory.php (1)
34-34: LGTM!新增的
enable_default_metrics配置项正确地添加到SPECIFIC_OPTIONS数组中,确保该选项在合并到ClientBuilder之前被移除,与其他框架特定选项的处理方式保持一致。src/sentry/src/Feature.php (1)
39-46: LGTM!新方法
isDefaultMetricsEnabled()实现正确:
- 首先检查
isMetricsEnabled(),确保基础指标功能已启用- 遵循现有方法签名模式,支持默认值参数
- 逻辑清晰,符合分层控制的设计意图
src/sentry/src/Metrics/Listener/OnWorkerStart.php (1)
56-58: LGTM!将检查条件从
isMetricsEnabled()更改为isDefaultMetricsEnabled()与 PR 目标一致,实现了默认指标收集的独立控制。src/sentry/src/Metrics/Listener/OnCoroutineServerStart.php (2)
70-72: LGTM!正确添加了默认指标启用检查,与其他监听器逻辑保持一致。
112-118: LGTM!为内存指标添加
worker标签,与其他指标收集点保持一致。协程服务器使用硬编码'0'是合理的,因为它运行在单一 worker 上下文中。src/sentry/src/Metrics/Listener/OnBeforeHandle.php (1)
63-108: 指标收集逻辑实现良好。周期性指标收集涵盖了 GC 状态、资源使用情况和内存指标,使用
defer确保指标刷新,符合 PR 目标中描述的命令行指标收集需求。
| if ($event instanceof AfterExecute) { | ||
| Context::destroy(Constants::RUN_IN_COMMAND); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Timer 资源泄漏:命令结束时未清理定时器。
当 AfterExecute 事件触发时,只是销毁了上下文但没有清理 $timerId。如果命令在 WORKER_EXIT 之前完成,定时器会继续运行直到 worker 退出,造成不必要的资源消耗。
建议保存 timerId 并在 AfterExecute 时清理:
class OnBeforeHandle implements ListenerInterface
{
use MetricSetter;
protected Timer $timer;
+ protected ?int $timerId = null;
+
public function __construct(protected Feature $feature)
{
$this->timer = new Timer();
} public function process(object $event): void
{
if ($event instanceof AfterExecute) {
+ if ($this->timerId !== null) {
+ $this->timer->clear($this->timerId);
+ $this->timerId = null;
+ }
Context::destroy(Constants::RUN_IN_COMMAND);
return;
}- $timerId = $this->timer->tick($this->feature->getMetricsInterval(), function () use ($metrics) {
+ $this->timerId = $this->timer->tick($this->feature->getMetricsInterval(), function () use ($metrics) {- Coroutine::create(function () use ($timerId) {
+ Coroutine::create(function () {
CoordinatorManager::until(\Hyperf\Coordinator\Constants::WORKER_EXIT)->yield();
- $this->timer->clear($timerId);
+ if ($this->timerId !== null) {
+ $this->timer->clear($this->timerId);
+ $this->timerId = null;
+ }
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/sentry/src/Metrics/Listener/OnBeforeHandle.php around lines 52-55, the
AfterExecute branch destroys the context but does not clear the active timer,
leaking the timer until worker exit; capture and store the timer ID when
creating the timer (e.g. as a private property on the listener or in Context),
and in the AfterExecute handler call the appropriate clear function
(clearTimer/Timer::clear/pcntl_alarm/ev_timer_clear depending on implementation)
using that stored timerId before destroying the context so the timer is
cancelled immediately when the command finishes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php (1)
52-55: Timer 资源泄漏问题已在之前的审查中指出。此问题已在之前的审查评论中详细说明：当
AfterExecute事件触发时，仅销毁了上下文但未清理定时器。请参考之前的审查建议，将timerId存储为类属性，并在AfterExecute时清理。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sentry/publish/sentry.php(1 hunks)src/sentry/src/Factory/ClientBuilderFactory.php(1 hunks)src/sentry/src/Feature.php(1 hunks)src/sentry/src/Metrics/Listener/OnBeforeHandle.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/sentry/publish/sentry.php
- src/sentry/src/Feature.php
🧰 Additional context used
📓 Path-based instructions (2)
src/*/src/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All components use the namespace pattern:
FriendsOfHyperf\{ComponentName}
Files:
src/sentry/src/Factory/ClientBuilderFactory.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.php
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Follow PSR-12 coding standards for all PHP code
Use PHP-CS-Fixer for code formatting as configured in.php-cs-fixer.php
Use PHPStan for static analysis as configured inphpstan.neon.dist
Files:
src/sentry/src/Factory/ClientBuilderFactory.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.php
🧬 Code graph analysis (1)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php (1)
src/sentry/src/Metrics/Traits/MetricSetter.php (1)
trySet(20-33)
⏰ 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). (15)
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.1 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 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.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: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.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 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
🔇 Additional comments (3)
src/sentry/src/Factory/ClientBuilderFactory.php (1)
34-35: LGTM!新增的
enable_default_metrics和enable_command_metrics配置项正确添加到SPECIFIC_OPTIONS常量中，确保这些选项在合并用户配置前被正确处理。与metrics_interval放在一起，逻辑分组清晰。src/sentry/src/Metrics/Listener/OnBeforeHandle.php (2)
15-37: 导入和类结构设计合理。新增的依赖注入（
Feature）和Timer属性初始化遵循良好的设计模式。使用MetricSettertrait 复用指标设置逻辑是正确的做法。
110-113: WORKER_EXIT 清理逻辑正确。使用协程等待
WORKER_EXIT信号来清理定时器是正确的模式，确保在 worker 退出时资源被释放。结合之前评论中建议的AfterExecute清理，可以形成完整的资源管理策略。
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php (2)
51-54: 严重：命令结束时未清理定时器导致资源泄漏。当
AfterExecute事件触发时，只是将$runningInCommand标志设为false，但没有清理在BeforeHandle中启动的定时器（第 89 行）。如果命令在 worker 退出前完成，定时器会持续运行直到 worker 退出，造成：
- 不必要的资源消耗（定期执行回调）
- 可能采集到错误的指标数据（命令已结束但仍在采集）
建议保存定时器 ID 并在
AfterExecute时清理：class OnBeforeHandle implements ListenerInterface { use MetricSetter; protected Timer $timer; + protected ?int $timerId = null; public function __construct(protected Feature $feature) { $this->timer = new Timer(); }public function process(object $event): void { if ($event instanceof AfterExecute) { + if ($this->timerId !== null) { + $this->timer->clear($this->timerId); + $this->timerId = null; + } Constants::$runningInCommand = false; return; }- $timerId = $this->timer->tick($this->feature->getMetricsInterval(), function () use ($metrics) { + $this->timerId = $this->timer->tick($this->feature->getMetricsInterval(), function () use ($metrics) {- Coroutine::create(function () use ($timerId) { + Coroutine::create(function () { CoordinatorManager::until(\Hyperf\Coordinator\Constants::WORKER_EXIT)->yield(); - $this->timer->clear($timerId); + if ($this->timerId !== null) { + $this->timer->clear($this->timerId); + $this->timerId = null; + } });
89-107: Worker 标签不一致。在定时器回调中，
trySet()调用使用默认参数workerId=0（参见MetricSettertrait 定义），导致gc_*和ru_*指标的 worker 标签为'0'，而显式的gauge()调用使用'N/A'。在命令行上下文中，所有指标应使用一致的 worker 标识符。建议统一使用相同的标识符，例如：
方案 1：统一使用
'N/A'（如果trySet()支持字符串类型）- $this->trySet('gc_', $metrics, gc_status()); - $this->trySet('', $metrics, getrusage()); + $this->trySet('gc_', $metrics, gc_status(), 'N/A'); + $this->trySet('', $metrics, getrusage(), 'N/A');方案 2：统一使用
-1- $this->trySet('gc_', $metrics, gc_status()); - $this->trySet('', $metrics, getrusage()); + $this->trySet('gc_', $metrics, gc_status(), -1); + $this->trySet('', $metrics, getrusage(), -1); metrics()->gauge( 'memory_usage', (float) memory_get_usage(), - ['worker' => 'N/A'], + ['worker' => '-1'], Unit::byte() ); metrics()->gauge( 'memory_peak_usage', (float) memory_get_peak_usage(), - ['worker' => 'N/A'], + ['worker' => '-1'], Unit::byte() );推荐使用方案 2，保持类型一致性（整数转字符串标签）。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sentry/src/Constants.php(1 hunks)src/sentry/src/Metrics/Listener/OnBeforeHandle.php(2 hunks)src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sentry/src/Metrics/Listener/OnMetricFactoryReady.php
🧰 Additional context used
📓 Path-based instructions (2)
src/*/src/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All components use the namespace pattern:
FriendsOfHyperf\{ComponentName}
Files:
src/sentry/src/Constants.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.php
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Follow PSR-12 coding standards for all PHP code
Use PHP-CS-Fixer for code formatting as configured in.php-cs-fixer.php
Use PHPStan for static analysis as configured inphpstan.neon.dist
Files:
src/sentry/src/Constants.phpsrc/sentry/src/Metrics/Listener/OnBeforeHandle.php
🧬 Code graph analysis (1)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php (5)
src/sentry/src/Feature.php (4)
Feature(18-109)__construct(20-22)isCommandMetricsEnabled(48-55)getMetricsInterval(57-66)src/sentry/src/Metrics/Timer.php (1)
Timer(18-62)src/sentry/src/Function.php (1)
metrics(30-33)src/sentry/src/Constants.php (1)
Constants(14-43)src/sentry/src/Metrics/Traits/MetricSetter.php (1)
trySet(20-33)
⏰ 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). (14)
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 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 6.1.3
- 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.3 with Swoole 6.0.2
- 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 5.1.8
- 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 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
🔇 Additional comments (4)
src/sentry/src/Metrics/Listener/OnBeforeHandle.php (4)
15-25: 导入语句正确。新增的导入项与新增功能需求一致，包括特性标志检查、指标设置、定时器管理和协程调度。
29-31: 特性和属性声明正确。
MetricSetter特性提供了后续使用的trySet()方法，Timer属性正确声明为 Hyperf 协调器定时器类型。
33-36: 构造函数依赖注入恰当。通过构造函数注入
Feature依赖并初始化Timer，符合依赖注入最佳实践。由于这是事件监听器，通常由容器管理，构造函数签名变更不会影响用户代码。
56-60: 命令指标启用检查正确。在设置
$runningInCommand标志后，正确使用特性标志isCommandMetricsEnabled()进行门控，避免在未启用时执行不必要的指标收集逻辑。
…tion (#1034) * feat: enhance Sentry metrics with configurable default metrics collection - Add SENTRY_ENABLE_DEFAULT_METRICS configuration option - Implement isDefaultMetricsEnabled() method in Feature class - Extend OnBeforeHandle listener to collect command-line metrics - Add worker identification tags to memory metrics - Separate default metrics from custom metrics collection - Add Timer-based periodic metrics collection for commands - Include garbage collection and resource usage metrics This enhancement provides more granular control over metrics collection and extends monitoring capabilities to command-line operations. * feat: 添加命令指标启用选项并更新相关逻辑 * feat: 使用静态变量替代上下文管理命令运行状态 * feat: 移除AfterExecute事件监听并优化命令指标处理逻辑 * fix: 更新注释以更清晰地说明命令自动退出的条件 * fix: 修复命令指标处理逻辑，确保在适当条件下启用指标收集 * fix: 优化命令指标启用逻辑，确保在适当条件下收集指标 * fix: 修复命令指标启用逻辑，确保在适当条件下收集默认指标 * fix: 更新指标收集逻辑，将工人标识从'N/A'更改为'0' * fix: 修复默认指标启用逻辑，确保在禁用时正确返回 --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
Changes Made
SENTRY_ENABLE_DEFAULT_METRICSenvironment variable supportisDefaultMetricsEnabled()method for granular controlOnBeforeHandlelistener to collect comprehensive command-line metricsTest plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.