Skip to content

Conversation

@huangdijia
Copy link
Contributor

@huangdijia huangdijia commented Sep 29, 2025

Summary

This PR optimizes the Sentry event data setting in EventHandleListener by moving data configuration from after-event handlers to before-event handlers across multiple event types.

Changes Made

  • Command Events: Move command arguments and options data setting to BeforeCommandHandle
  • Crontab Events: Move crontab rule, type, and options data/tags setting to BeforeCrontabDispatch
  • AMQP Events: Move messaging system data setting to BeforeAmqpConsume
  • Kafka Events: Move messaging system data setting to BeforeKafkaConsume
  • Async Queue Events: Move messaging system data setting to BeforeAsyncQueueHandle

Benefits

  • Performance: Data is set earlier in the event lifecycle, reducing overhead
  • Consistency: All event types now follow the same pattern of setting data upfront
  • Maintainability: Reduces code duplication and centralizes data configuration
  • Reliability: Ensures tracing data is available immediately when transactions start

Affected Components

  • Console command tracing
  • Crontab execution tracing
  • AMQP message processing tracing
  • Kafka consumer tracing
  • Async queue job processing tracing

Test Plan

  • Verify console commands still trace correctly with proper arguments/options
  • Test crontab execution tracing includes rule and type information
  • Confirm AMQP message processing maintains all messaging metadata
  • Validate Kafka consumer tracing preserves topic and group information
  • Ensure async queue jobs trace with proper retry counts and latency data

Summary by CodeRabbit

  • 新功能

    • 在命令、Crontab、AMQP、Kafka、异步队列的开始阶段记录更丰富的追踪数据与标签(系统、操作、消息ID、体积、接收延迟、重试次数、目的地等)。
    • 新增标签与数据:命令退出码、Crontab 规则/类型、任务选项等。
  • 重构

    • 精简结束阶段的数据写入,移除冗余数据块,仅保留关键结果标签。
    • 调整协程上下文传递,停止自动传播部分监控上下文键,降低噪声与开销。

Move data setting from after-event handlers to before-event handlers to:
- Improve tracing performance by setting data earlier
- Ensure consistent data availability across all event types
- Reduce code duplication and improve maintainability

Affects command, crontab, AMQP, Kafka, and async queue tracing
@coderabbitai
Copy link

coderabbitai bot commented Sep 29, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

在两个 CoroutineAspect 中移除了在协程上下文传播键列表中的 SentrySdk::class;在 Tracing 的 EventHandleListener 中调整了各类事件(命令、Crontab、AMQP、Kafka、异步队列)的开始与结束时记录的数据与标签策略,增加开始阶段的结构化数据并在结束阶段移除详细数据或仅保留结果性标签。

Changes

Cohort / File(s) Summary of changes
协程上下文键精简(Aspect)
src/sentry/src/Aspect/CoroutineAspect.php, src/sentry/src/Tracing/Aspect/CoroutineAspect.php
从受保护的 $keys 数组中移除(注释掉)SentrySdk::class,保留其他键不变;不改变运行时对 SentrySdk::getCurrentHub() 的调用。Tracing 版本同样移除该键,影响协程创建/执行时的上下文合并范围。
事件处理监听重构(Tracing Listener)
src/sentry/src/Tracing/Listener/EventHandleListener.php
调整数据记录时机与内容:
- 命令:开始时记录 command.argumentscommand.options 数据;结束时移除这些数据,仅以标签记录 command.exit_code
- Crontab:开始时新增 crontab.rulecrontab.type 标签及 crontab.options.is_singlecrontab.options.is_on_one_server 数据;结束时移除该数据块。
- AMQP:开始时记录消息系统、操作、消息标识、体积、接收延迟、重试次数、目的地、以及 AMQP 相关字段;处理完成时移除数据,仅保留 messaging.amqp.message.result
- Kafka:开始时记录消息系统、操作、消息标识、体积、接收延迟、重试次数、目的地、消费者组与连接池;完成时移除数据块。
- 异步队列:开始时记录系统、操作、消息标识、体积、接收延迟、重试次数、目的地等;完成时移除该数据块。
- 同步清理部分基于 carrier 的后置数据填充与相应读取逻辑;错误处理与 span 结束保留。

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Producer as 触发源
  participant Listener as EventHandleListener
  participant Tracer as Tracing/Transaction
  participant Sink as Exporter

  rect rgb(242,248,255)
  note over Producer,Listener: 事件开始(命令/Crontab/AMQP/Kafka/异步队列)
  Producer->>Listener: onStart(event)
  Listener->>Tracer: startTransaction/span
  Listener->>Tracer: set tags/data(丰富的起始元数据)
  end

  alt 处理成功
    Listener-->>Listener: 处理业务逻辑
    Listener->>Tracer: finish准备(清理详细data)
    Listener->>Tracer: set result/exit_code 等结果性标签
    Tracer-->>Sink: export(span/transaction)
  else 处理失败
    Listener-->>Tracer: recordException
    Listener->>Tracer: finish with error
    Tracer-->>Sink: export(error span)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • xuanyanwow
  • zds-s

Poem

小兔挥笔跃云端,
协程轻装少一环。
事件开场多讯息,
收尾只留果与签。
鼓耳听风踪迹稳,
码海留痕更清澜。 🐇✨

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/sentry-optimize-event-data-setting

📜 Recent 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 0660eb2 and 1ba9136.

📒 Files selected for processing (3)
  • src/sentry/src/Aspect/CoroutineAspect.php (1 hunks)
  • src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1 hunks)
  • src/sentry/src/Tracing/Listener/EventHandleListener.php (7 hunks)

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.

@huangdijia huangdijia merged commit 8ccab0e into main Sep 29, 2025
15 of 16 checks passed
@huangdijia huangdijia deleted the refactor/sentry-optimize-event-data-setting branch September 29, 2025 01:18
huangdijia added a commit that referenced this pull request Sep 29, 2025
…937)

* refactor(sentry): optimize event data setting in EventHandleListener

Move data setting from after-event handlers to before-event handlers to:
- Improve tracing performance by setting data earlier
- Ensure consistent data availability across all event types
- Reduce code duplication and improve maintainability

Affects command, crontab, AMQP, Kafka, and async queue tracing

* 修复:注释掉 SentrySdk 类的导入

---------

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