Skip to content

Conversation

@huangdijia
Copy link
Contributor

@huangdijia huangdijia commented Sep 26, 2025

Summary

  • Use defer functions to ensure proper transaction cleanup in coroutines
  • Separate transaction handling for coroutine and non-coroutine contexts
  • Simplify transaction finishing logic by removing try-finally blocks
  • Apply consistent pattern across command, crontab, AMQP, Kafka, and async queue handlers

Changes Made

This PR improves Sentry transaction handling in the EventHandleListener by:

  1. Coroutine-aware transaction management: Using defer() functions to ensure transactions are properly finished in coroutine contexts
  2. Conditional transaction finishing: Only finishing transactions in non-coroutine contexts in the finally blocks, while coroutine contexts use deferred cleanup
  3. Code simplification: Removing unnecessary try-finally blocks and moving transaction data setting outside of these blocks
  4. Consistent pattern: Applying the same improvement pattern across all event handlers (command, crontab, AMQP, Kafka, async queue)

Benefits

  • Prevents transaction leaks in coroutine contexts
  • Ensures proper transaction lifecycle management regardless of execution context
  • Cleaner, more maintainable code structure
  • Consistent behavior across different event types

Test Plan

  • Verify command execution tracking works correctly in both coroutine and non-coroutine contexts
  • Test crontab task tracking with proper transaction cleanup
  • Validate AMQP message processing transaction handling
  • Check Kafka consumer transaction management
  • Ensure async queue job processing transactions are handled properly

Summary by CodeRabbit

  • Bug Fixes

    • 修复在协程/非协程场景下追踪可能未正确结束的问题,确保命令、定时任务及消息队列等事件均能可靠完成并上报。
    • 异常信息(类型、代码、消息、堆栈)更完整地附加到追踪中(在启用相关选项时)。
  • Refactor

    • 统一事务/跨度的生命周期管理与完成路径,采用延后结束策略提升稳定性与一致性。
    • 对非采样情况优化早返回,降低不必要的开销。

- Use defer functions to ensure proper transaction cleanup in coroutines
- Separate transaction handling for coroutine and non-coroutine contexts
- Simplify transaction finishing logic by removing try-finally blocks
- Apply consistent pattern across command, crontab, AMQP, Kafka, and async queue handlers
@coderabbitai
Copy link

coderabbitai bot commented Sep 26, 2025

Walkthrough

src/sentry/src/Tracing/Listener/EventHandleListener.php 中统一并强化了各类事件处理(命令、定时任务、AMQP、Kafka、异步队列等)的事务生命周期管理:以局部变量保存事务、依据是否处于协程选择使用 defer 或 finally 收尾、在异常时附加标签/数据、并在非采样时提前返回。

Changes

Cohort / File(s) Summary
Tracing listener transaction lifecycle
src/sentry/src/Tracing/Listener/EventHandleListener.php
将直接返回的 startTransaction 改为赋值保存;在协程中使用 defer 完成事务并设置活动 Span,非协程通过 finally 收尾;统一异常标签/数据附加;在多种事件源(command/crontab/amqp/kafka/async_queue)中标准化收尾逻辑与非采样短路。

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Source as Event Source
    participant L as EventHandleListener
    participant T as Tracing SDK

    Source->>L: onEvent(...)
    L->>T: startTransaction(ctx)
    T-->>L: transaction
    Note over L,T: 保存为局部变量;设置 active span

    alt Coroutine
        Note over L: 使用 defer 注册收尾
        L->>L: process handler
        alt Error occurs
            L->>T: attach exception tags/data
        end
        Note over L,T: defer 内调用 finish()
    else Non-Coroutine
        L->>L: try { process handler } finally { finish() }
        alt Error occurs
            L->>T: attach exception tags/data
        end
    end

    Note right of L: 非采样时尽早返回(短路)
Loading

Estimated code review effort

🎯 4 (复杂) | ⏱️ ~45–60 minutes

Possibly related PRs

Suggested reviewers

  • xuanyanwow

Poem

小兔执笔画时钟,defer 轻拢慢收功;
协程起落如潮涌,finally 护航不放松。
事务有始终,异常不逃踪;
耳尖一动——span 已对准,踪迹清清楚楚嗡。 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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 标题“feat(sentry): improve transaction handling in coroutine contexts”清晰地概括了此次 PR 的核心改动,即在协程上下文中改进 Sentry 事务处理,语言简洁且符合常见的 commit 规范,没有包含无关噪音或模糊描述。
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/sentry-coroutine-transaction-handling

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.28)

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.

❤️ 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: 2

📜 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 f0429aa and c181c7e.

📒 Files selected for processing (1)
  • src/sentry/src/Tracing/Listener/EventHandleListener.php (10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (6)
src/sentry/src/Tracing/Tracer.php (1)
  • startTransaction (38-63)
src/sentry/src/Function.php (1)
  • startTransaction (24-29)
src/sentry/src/Tracing/SpanStarter.php (2)
  • startTransaction (39-44)
  • continueTrace (114-143)
src/sentry/class_map/SentrySdk.php (2)
  • SentrySdk (24-65)
  • getCurrentHub (51-54)
src/sentry/src/Switcher.php (1)
  • isTracingExtraTagEnabled (85-88)
src/sentry/src/Util/Carrier.php (4)
  • getSentryTrace (100-103)
  • getBaggage (105-108)
  • get (120-123)
  • has (115-118)
⏰ 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

@huangdijia huangdijia merged commit 6a7a602 into main Sep 26, 2025
17 checks passed
@huangdijia huangdijia deleted the feat/sentry-coroutine-transaction-handling branch September 26, 2025 00:24
huangdijia added a commit that referenced this pull request Sep 26, 2025
- Use defer functions to ensure proper transaction cleanup in coroutines
- Separate transaction handling for coroutine and non-coroutine contexts
- Simplify transaction finishing logic by removing try-finally blocks
- Apply consistent pattern across command, crontab, AMQP, Kafka, and async queue handlers

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