feat(sentry): improve event flushing and expand listener coverage #908
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit enhances the Sentry integration by implementing comprehensive event flushing across all listeners and improving event handling coverage: - Add log flushing to Integration::flushEvents() to ensure all logs are sent - Implement automatic event flushing in CoroutineAspect finally block - Expand event listener coverage to handle completion events (AfterConsume, AfterHandle, AfterExecute, RetryHandle) - Add flushEvents() calls after processing completion and failure events - Ensure proper cleanup and event delivery in all async contexts These changes improve reliability of error reporting and ensure events are not lost when processes terminate or coroutines complete.
Walkthrough在多处监听器与协程切面中引入或扩展事件处理与刷新流程：新增/订阅 After* 与 Retry* 类事件，统一在异常捕获后或处理结束时调用 Integration::flushEvents()，Integration::flushEvents() 同时触发日志 flush。协程切面改为 try-catch-finally，确保任意路径均执行 flush。 Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant A as CoroutineAspect
participant F as Inner Callable
participant I as Integration
participant L as Logs
C->>A: invoke(callable)
A->>F: execute()
alt Throwable thrown
F-->>A: throw
A->>A: catch and rethrow
end
A->>I: flushEvents() [finally]
I->>L: Logs::flush()
A-->>C: return/propagate
note over A,I: 新增 finally 确保所有路径均 flush
sequenceDiagram
participant S as EventSource
participant Ls as *ExceptionListener
participant SS as Sentry SDK
participant I as Integration
participant LG as Logs
S-->>Ls: Before*/After*/Retry*/Failed*/Terminated
alt Failed*/Retry*
Ls->>SS: captureException(event.exception)
Ls->>I: flushEvents()
else After*/Terminated
Ls->>I: flushEvents()
else Before*
Ls->>SS: setupSentrySdk()
end
I->>LG: Logs::flush()
note over Ls,I: 新增/扩展事件触发统一 flush
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.17)At least one path must be specified to analyse. Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
huangdijia
added a commit
that referenced
this pull request
Sep 18, 2025
This commit enhances the Sentry integration by implementing comprehensive event flushing across all listeners and improving event handling coverage: - Add log flushing to Integration::flushEvents() to ensure all logs are sent - Implement automatic event flushing in CoroutineAspect finally block - Expand event listener coverage to handle completion events (AfterConsume, AfterHandle, AfterExecute, RetryHandle) - Add flushEvents() calls after processing completion and failure events - Ensure proper cleanup and event delivery in all async contexts These changes improve reliability of error reporting and ensure events are not lost when processes terminate or coroutines complete. Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
![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 (11)
src/sentry/src/Listener/CaptureExceptionListener.php (1)
65-68: 建议添加错误处理
flushEvents()方法直接调用Integration::flushEvents()，但没有错误处理。建议添加 try-catch 块以防止刷新失败影响正常流程。protected function flushEvents(): void { - Integration::flushEvents(); + try { + Integration::flushEvents(); + } catch (Throwable $e) { + if ($this->container->has(StdoutLoggerInterface::class)) { + $this->container->get(StdoutLoggerInterface::class)->error('Failed to flush Sentry events: ' . $e->getMessage()); + } + } }src/sentry/src/Listener/AmqpExceptionListener.php (1)
41-45: 重复的 match 语句可能影响性能使用了两个独立的
match语句处理同一个事件。建议合并为单个 match 语句以提高效率。match ($event::class) { - Event\FailToConsume::class => $this->captureException($event->getThrowable()), - default => $this->setupSentrySdk(), -}; - -match ($event::class) { - Event\AfterConsume::class, - Event\FailToConsume::class => $this->flushEvents(), - default => null, + Event\FailToConsume::class => (function() use ($event) { + $this->captureException($event->getThrowable()); + $this->flushEvents(); + })(), + Event\AfterConsume::class => $this->flushEvents(), + default => $this->setupSentrySdk(), };src/sentry/src/Listener/RequestExceptionListener.php (1)
45-49: 建议合并重复的 match 语句与 AmqpExceptionListener 类似，这里也使用了两个 match 语句。建议合并以提高代码效率和可读性。
match ($event::class) { - RequestTerminated::class, RpcRequestTerminated::class => $this->captureException($event->exception), - default => $this->setupSentrySdk(), -}; - -match ($event::class) { - RequestTerminated::class, - RpcRequestTerminated::class => $this->flushEvents(), - default => null, + RequestTerminated::class, RpcRequestTerminated::class => (function() use ($event) { + $this->captureException($event->exception); + $this->flushEvents(); + })(), + default => $this->setupSentrySdk(), };src/sentry/src/Listener/CrontabExceptionListener.php (1)
46-50: 建议优化重复的 match 语句结构与其他监听器类似，这里也使用了两个 match 语句。为保持代码一致性和效率，建议合并。
match ($event::class) { - Event\FailToExecute::class => $this->captureException($event->throwable), - default => $this->setupSentrySdk(), -}; - -match ($event::class) { - Event\AfterExecute::class, - Event\FailToExecute::class => $this->flushEvents(), - default => null, + Event\FailToExecute::class => (function() use ($event) { + $this->captureException($event->throwable); + $this->flushEvents(); + })(), + Event\AfterExecute::class => $this->flushEvents(), + default => $this->setupSentrySdk(), };src/sentry/src/Listener/KafkaExceptionListener.php (4)
20-24: 新增 AfterConsume 监听，请确认兼容性与吞吐影响
- 确认所依赖的 hyperf/kafka 版本已包含 Event\AfterConsume（以及与现有事件名保持一致）。
- 在高吞吐消费场景，每次成功消费后主动 flush 可能显著增加延迟与 I/O；建议提供可配置开关，仅在需要时开启成功路径的 flush。
28-29: Docblock 类型注释与监听集合一致性 OK；可微调以强化静态分析当前注释已覆盖 Before/Fail/After 三类事件。为提高静态分析严格度，可去掉末尾的
|object（或将方法签名升级为 union 类型，如果生态不依赖object签名）。
41-45: FailToConsume 路径存在重复 flush（一次在 captureException finally，另一次在此处）为避免重复对 Sentry 客户端 flush，同时保证日志也能被刷出，建议将统一的刷出逻辑放到 CaptureExceptionListener 中（调用 Integration::flushEvents），然后此处仅在 AfterConsume 时 flush。示例改动：
- CaptureExceptionListener 统一刷出（跨文件变更）：
--- a/src/sentry/src/Listener/CaptureExceptionListener.php +++ b/src/sentry/src/Listener/CaptureExceptionListener.php @@ - } finally { - $hub->getClient()?->flush(); - } + } finally { + $this->flushEvents(); + }
- 本文件避免 FailToConsume 再次 flush：
- match ($event::class) { - Event\AfterConsume::class, - Event\FailToConsume::class => $this->flushEvents(), - default => null, - }; + match ($event::class) { + Event\AfterConsume::class => $this->flushEvents(), + default => null, + };
36-39: 避免在 AfterConsume 等非初始化阶段重复 setupSentrySdk在当前分支下，AfterConsume 也会落入
default => $this->setupSentrySdk()，会产生“已初始化”的告警且无实质收益。可仅在 BeforeConsume 时进行初始化，提升信噪比：- match ($event::class) { - Event\FailToConsume::class => $this->captureException($event->getThrowable()), - default => $this->setupSentrySdk(), - }; + match (true) { + $event instanceof Event\BeforeConsume => $this->setupSentrySdk(), + $event instanceof Event\FailToConsume => $this->captureException($event->getThrowable()), + default => null, + };（同时使用
instanceof可增强对子类/代理事件的兼容性。）src/sentry/src/Listener/AsyncQueueExceptionListener.php (3)
22-25: 扩展监听集（AfterHandle/RetryHandle）总体 LGTM；注意成功路径 flush 的吞吐影响设计上覆盖了成功、失败与重试阶段，方向正确。请评估在高并发队列下对 AfterHandle 每次 flush 的额外耗时/带宽影响，必要时提供可配置开关或分环境控制。
29-30: Docblock 遗漏 RetryHandle
process()已处理Event\RetryHandle，Docblock 建议同步补齐，避免误导静态分析与阅读者：- * @param Event\FailedHandle|Event\AfterHandle|Event\BeforeHandle|object $event + * @param Event\FailedHandle|Event\AfterHandle|Event\RetryHandle|Event\BeforeHandle|object $event
44-48: 重复 flush 与成功路径 flush 的性能考量；建议统一刷出逻辑
- Failed/Retry 分支：
captureException()的 finally 已对客户端 flush；此处再次flushEvents()会重复。建议如前所述将 finally 改为$this->flushEvents()（以同时刷日志），并考虑在本处仅对成功路径（AfterHandle）进行 flush。- 成功路径（AfterHandle）：建议提供开关（如
sentry.flush_on_success）或按环境控制，以兼顾“尽量不丢事件”和吞吐/延迟。示例（本文件仅保留 AfterHandle 的 flush）：
- match ($event::class) { - Event\AfterHandle::class, - Event\RetryHandle::class, - Event\FailedHandle::class => $this->flushEvents(), - default => null, - }; + match (true) { + $event instanceof Event\AfterHandle => $this->flushEvents(), + default => null, + };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/sentry/src/Aspect/CoroutineAspect.php(2 hunks)src/sentry/src/Integration.php(2 hunks)src/sentry/src/Listener/AmqpExceptionListener.php(2 hunks)src/sentry/src/Listener/AsyncQueueExceptionListener.php(2 hunks)src/sentry/src/Listener/CaptureExceptionListener.php(2 hunks)src/sentry/src/Listener/CommandExceptionListener.php(2 hunks)src/sentry/src/Listener/CrontabExceptionListener.php(2 hunks)src/sentry/src/Listener/KafkaExceptionListener.php(2 hunks)src/sentry/src/Listener/RequestExceptionListener.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/sentry/src/Aspect/CoroutineAspect.php (2)
src/sentry/src/Integration.php (2)
Integration(28-158)flushEvents(106-115)src/sentry/src/Listener/CaptureExceptionListener.php (1)
flushEvents(65-68)
src/sentry/src/Listener/CommandExceptionListener.php (2)
src/sentry/src/Integration.php (1)
flushEvents(106-115)src/sentry/src/Listener/CaptureExceptionListener.php (1)
flushEvents(65-68)
src/sentry/src/Listener/KafkaExceptionListener.php (3)
src/facade/src/Event.php (1)
Event(21-28)src/sentry/src/Integration.php (1)
flushEvents(106-115)src/sentry/src/Listener/CaptureExceptionListener.php (1)
flushEvents(65-68)
src/sentry/src/Listener/AmqpExceptionListener.php (2)
src/sentry/src/Integration.php (1)
flushEvents(106-115)src/sentry/src/Listener/CaptureExceptionListener.php (1)
flushEvents(65-68)
src/sentry/src/Listener/AsyncQueueExceptionListener.php (3)
src/sentry/src/Listener/CaptureExceptionListener.php (3)
captureException(34-49)setupSentrySdk(51-63)flushEvents(65-68)src/exception-event/src/Event/ExceptionDispatched.php (1)
getThrowable(30-33)src/sentry/src/Integration.php (1)
flushEvents(106-115)
src/sentry/src/Listener/CrontabExceptionListener.php (3)
src/facade/src/Event.php (1)
Event(21-28)src/sentry/src/Integration.php (1)
flushEvents(106-115)src/sentry/src/Listener/CaptureExceptionListener.php (1)
flushEvents(65-68)
src/sentry/src/Listener/RequestExceptionListener.php (2)
src/sentry/src/Integration.php (1)
flushEvents(106-115)src/sentry/src/Listener/CaptureExceptionListener.php (1)
flushEvents(65-68)
src/sentry/src/Listener/CaptureExceptionListener.php (1)
src/sentry/src/Integration.php (2)
Integration(28-158)flushEvents(106-115)
⏰ 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 5.1.7
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.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
🔇 Additional comments (2)
src/sentry/src/Aspect/CoroutineAspect.php (1)
48-55: 良好的资源清理实践在
finally块中调用Integration::flushEvents()确保了无论协程执行成功还是失败，事件都会被正确刷新。这是一个很好的改进，保证了 Sentry 事件不会丢失。src/sentry/src/Listener/CommandExceptionListener.php (1)
23-23: 正确处理了命令执行完成事件添加
AfterExecute事件监听并在执行后刷新事件，确保命令执行期间产生的所有 Sentry 事件都能被发送。Also applies to: 38-38
| if ($client !== null) { | ||
| $client->flush(); | ||
|
|
||
| Logs::getInstance()->flush(); |
There was a problem hiding this comment.
🧩 Analysis chain
需要验证 Logs 类的导入和使用是否正确
添加了 Logs::getInstance()->flush() 调用，但需要确认：
Sentry\Logs\Logs类是否在当前使用的 Sentry SDK 版本中存在- 该类是否提供了
getInstance()和flush()方法
🏁 Script executed:
#!/bin/bash
# 检查 composer.json 中的 Sentry SDK 版本
cat composer.json | jq -r '.require | to_entries[] | select(.key | contains("sentry")) | "\(.key): \(.value)"'
# 搜索项目中是否有其他地方使用了 Logs 类
rg -n "Sentry\\\\Logs\\\\Logs" --type php
# 检查是否有 Logs 相关的配置或初始化
rg -n "Logs::getInstance\(\)" --type phpLength of output: 155
🌐 Web query:
Sentry PHP SDK Logs class documentation
💡 Result:
Here are the official docs for the Sentry PHP SDK logs API / Log class:
- Sentry PHP — Logs (how to enable and use \Sentry\Logs\Log in the SDK; includes examples and before_send_log signature). [1]
- Sentry SDK developer docs — Logs protocol and SDK logging requirements (spec for log envelope / SDK API). [2]
- getsentry/sentry-php GitHub repository (official PHP SDK source & code). [3]
References
[1] https://docs.sentry.io/platforms/php/logs/
[2] https://develop.sentry.dev/sdk/telemetry/logs/
[3] https://github.com/getsentry/sentry-php
必须修复：确认并修正 Sentry Logs 的导入与用法（会导致运行时错误）
- 现状：composer.json 中使用 sentry/sentry: ^4.15.0；文件 src/sentry/src/Integration.php 存在
use Sentry\Logs\Logs;（第17行），并在第113行调用Logs::getInstance()->flush();。 - 问题：官方文档/SDK 资料中提及的是
\Sentry\Logs\Log（单数），未见Logs::getInstance()或flush()这类单例 API，当前导入/调用很可能不存在且会在运行时报错。 - 要求：替换为 SDK 支持的日志接口或正确的类名/命名空间；若这是自定义实现，请在仓库中添加该类定义并更新依赖说明；否则移除该调用或改用 SDK 提供的正确“flush”机制。
Comment on lines
+38
to
41
| Event\RetryHandle::class, | ||
| Event\FailedHandle::class => $this->captureException($event->getThrowable()), | ||
| default => $this->setupSentrySdk(), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
确认 RetryHandle 是否恒定提供 getThrowable()；建议以 instanceof + 守卫式调用更稳妥
若 RetryHandle 在部分实现中不包含 getThrowable()，当前直接调用将导致致命错误。建议改为 instanceof 并在 RetryHandle 分支做方法存在性守卫：
- match ($event::class) {
- Event\RetryHandle::class,
- Event\FailedHandle::class => $this->captureException($event->getThrowable()),
- default => $this->setupSentrySdk(),
- };
+ match (true) {
+ $event instanceof Event\BeforeHandle => $this->setupSentrySdk(),
+ $event instanceof Event\RetryHandle => method_exists($event, 'getThrowable') ? $this->captureException($event->getThrowable()) : null,
+ $event instanceof Event\FailedHandle => $this->captureException($event->getThrowable()),
+ default => null,
+ };为稳妥，请核实框架事件定义：
🌐 Web query:
Hyperf AsyncQueue Event RetryHandle 是否提供 getThrowable() 方法？
💡 Result:
短答：没有。RetryHandle 事件类并不带 getThrowable() 方法（文档/示例中只有 FailedHandle 使用 event->getThrowable()，RetryHandle 只是标记“重试前”事件）。[1][2]
来源：
- Hyperf 异步队列事件文档（列出 RetryHandle/FailedHandle）. [1]
- 示例代码：FailedHandle 调用 getThrowable()，RetryHandle 未调用（示例监听器）。[2]
必须修复：RetryHandle 不提供 getThrowable()，当前实现会抛致命错误
已确认：Hyperf 的 RetryHandle 事件类不包含 getThrowable()（文档/示例）；在 Retry 事件上直接调用 $event->getThrowable() 会触发运行时错误。
文件：src/sentry/src/Listener/AsyncQueueExceptionListener.php 行 38-41
- match ($event::class) {
- Event\RetryHandle::class,
- Event\FailedHandle::class => $this->captureException($event->getThrowable()),
- default => $this->setupSentrySdk(),
- };
+ match (true) {
+ $event instanceof Event\BeforeHandle => $this->setupSentrySdk(),
+ $event instanceof Event\RetryHandle => method_exists($event, 'getThrowable') ? $this->captureException($event->getThrowable()) : null,
+ $event instanceof Event\FailedHandle => $this->captureException($event->getThrowable()),
+ default => null,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Event\RetryHandle::class, | |
| Event\FailedHandle::class => $this->captureException($event->getThrowable()), | |
| default => $this->setupSentrySdk(), | |
| }; | |
| match (true) { | |
| $event instanceof Event\BeforeHandle => $this->setupSentrySdk(), | |
| $event instanceof Event\RetryHandle => method_exists($event, 'getThrowable') ? $this->captureException($event->getThrowable()) : null, | |
| $event instanceof Event\FailedHandle => $this->captureException($event->getThrowable()), | |
| default => null, | |
| }; |
This was referenced Sep 20, 2025
This was referenced Sep 29, 2025
This was referenced Dec 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Changes Made
Logs::getInstance()->flush()to ensure logs are flushedIntegration::flushEvents()in finally block for proper cleanupTest plan
Summary by CodeRabbit