-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat(sentry): add singleton support for Logs class #1050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add \Sentry\Logs\Logs::getInstance to SingletonAspect whitelist - Move Logs::getInstance()->flush() outside client null check for proper cleanup - Ensure Logs singleton instances are properly isolated per coroutine context 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Walkthrough该PR在单例注册表中添加了Sentry\Logs\Logs::class以启用其懒加载实例化,并修改了flushEvents()方法使日志和指标刷新调用总是执行,而不依赖客户端存在。 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4)**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/**/src/**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/*/src/**/*.php📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.php📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-12-12T05:33:43.947ZApplied to files:
🧬 Code graph analysis (2)src/sentry/src/Aspect/SingletonAspect.php (1)
src/sentry/src/Integration.php (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
🔇 Additional comments (3)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.33)At least one path must be specified to analyse. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances Sentry integration coroutine safety by adding proper singleton isolation for the Logs class and ensuring cleanup methods are always called regardless of client state.
Key Changes:
- Added
Logssingleton to the coroutine isolation whitelist inSingletonAspect - Moved
LogsandTraceMetricsflush calls outside the client null check to ensure proper cleanup in all scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/sentry/src/Aspect/SingletonAspect.php | Added Logs class to singleton whitelist in both $classes array and match expression to enable per-coroutine instance isolation |
| src/sentry/src/Integration.php | Moved Logs::getInstance()->flush() and TraceMetrics::getInstance()->flush() outside the client null check to ensure these singletons are always flushed, preventing data loss when client is unavailable |
The changes are well-structured and follow existing patterns in the codebase. The Logs class is correctly configured using Closure::bind() for instantiation (consistent with other protected-constructor singletons like HubAdapter and IntegrationRegistry), and the flush logic improvement ensures that accumulated logs and metrics are properly cleaned up even when the Sentry client is not available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sentry/src/Aspect/SingletonAspect.php (1)
43-45: 修复预存在的逻辑错误代码中存在逻辑错误:当
$arguments[0]不存在时(条件为真),却尝试访问$arguments[0],这会导致未定义索引错误。正确的逻辑应该是:当参数存在时才追加到 key 中。🔎 建议的修复
-if (! array_key_exists(0, $arguments)) { +if (array_key_exists(0, $arguments)) { $key .= '#' . $arguments[0]; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sentry/src/Aspect/SingletonAspect.phpsrc/sentry/src/Integration.php
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: All PHP files must includedeclare(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.phpconfiguration for PHP code formattingAll 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; ensurecomposer test:typesstays green before pushing
Files:
src/sentry/src/Aspect/SingletonAspect.phpsrc/sentry/src/Integration.php
src/**/src/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use namespace pattern
FriendsOfHyperf\{ComponentName}for all component classes
Files:
src/sentry/src/Aspect/SingletonAspect.phpsrc/sentry/src/Integration.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/Aspect/SingletonAspect.phpsrc/sentry/src/Integration.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/Aspect/SingletonAspect.phpsrc/sentry/src/Integration.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 : Leverage Hyperf's AOP (Aspect-Oriented Programming) for cross-cutting concerns via aspects defined in ConfigProvider
Applied to files:
src/sentry/src/Aspect/SingletonAspect.php
🧬 Code graph analysis (2)
src/sentry/src/Aspect/SingletonAspect.php (1)
src/sentry/src/Integration.php (1)
Integration(29-155)
src/sentry/src/Integration.php (1)
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). (18)
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 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.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 6.1.3
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 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 6.1.3
🔇 Additional comments (3)
src/sentry/src/Aspect/SingletonAspect.php (2)
25-25: 正确添加了 Logs 单例到白名单将
\Sentry\Logs\Logs::class . '::getInstance'添加到$classes数组中,使得 AOP 切面能够拦截该方法调用,这与 PR 目标一致。
50-51: 正确实现了 Logs 单例的协程隔离将
\Sentry\Logs\Logs::class添加到 match 表达式中,并使用Context::getOrSet确保每个协程上下文都有独立的 Logs 实例。使用Closure::bind模式表明 Logs 类的构造函数是私有的,这与 IntegrationRegistry 的实现模式一致。src/sentry/src/Integration.php (1)
109-111: 改进了刷新逻辑,确保日志和指标始终被正确刷新使用 nullsafe 操作符
?->简化了客户端刷新逻辑。由于 Hyperf 中所有状态数据应该存储在 Hyperf\Context\Context 类中,get/set 操作仅限于执行它们的对应协程,并在协程结束时自动销毁相关上下文数据,SingletonAspect 通过Context::getOrSet()确保Logs::getInstance()和TraceMetrics::getInstance()总是返回有效实例,因此将这些调用设为无条件执行是安全的。这种方式充分利用了 Hyperf 的协程上下文管理机制,避免了传统 null 检查的开销,同时确保即使客户端为 null 时日志和指标也能被正确刷新。
* feat(sentry): add singleton support for Logs class - Add \Sentry\Logs\Logs::getInstance to SingletonAspect whitelist - Move Logs::getInstance()->flush() outside client null check for proper cleanup - Ensure Logs singleton instances are properly isolated per coroutine context 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(sentry): simplify flushEvents method by using nullsafe operator --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Summary
\Sentry\Logs\Logs::getInstancetoSingletonAspectwhitelist to ensure proper coroutine isolationLogs::getInstance()->flush()andTraceMetrics::getInstance()->flush()outside the$clientnull check for proper cleanupChanges
SingletonAspect.php
\Sentry\Logs\Logs::classto the singleton whitelist in both the method patterns and match expressionIntegration.php
Logs::getInstance()->flush()andTraceMetrics::getInstance()->flush()outside the$client !== nullconditional blockTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
发布说明
Bug 修复
优化
✏️ Tip: You can customize this high-level summary in your review settings.