Skip to content

refactor(sentry): remove redundant Integration::flushEvents() calls from defer callbacks #1054

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.

Sign up for GitHub

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

Merged
huangdijia merged 2 commits into from
Dec 25, 2025
Merged

refactor(sentry): remove redundant Integration::flushEvents() calls from defer callbacks #1054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ebb71f
refactor(sentry): remove redundant Integration::flushEvents() calls f…
huangdijia Dec 25, 2025
f97b01d
chore: disable Sentry SingletonAspect registration
huangdijia Dec 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/sentry/src/ConfigProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public function __invoke(): array
Aspect\GuzzleHttpClientAspect::class,
Aspect\LoggerAspect::class,
Aspect\RedisAspect::class,
Aspect\SingletonAspect::class,
// Aspect\SingletonAspect::class,
Metrics\Aspect\CounterAspect::class,
Metrics\Aspect\HistogramAspect::class,
Tracing\Aspect\AmqpProducerAspect::class,
Expand Down
15 changes: 0 additions & 15 deletions src/sentry/src/Tracing/Listener/EventHandleListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,6 @@ protected function handleRequestReceived(HttpEvent\RequestReceived|RpcEvent\Requ

// Finish transaction
$transaction->finish();
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of Integration::flushEvents() from this defer callback is incorrect. This defer callback handles HTTP and RPC request lifecycles, which are completely separate from command execution lifecycles. The Integration::flushEvents() in handleCommandFinished's finally block (line 457) only executes for CommandEvent\AfterExecute events (CLI commands), not for HTTP/RPC requests.

Without this flush call, Sentry events generated during HTTP/RPC request handling will not be sent to Sentry, resulting in data loss. The flush call must be restored here.

Suggested change
$transaction->finish();
$transaction->finish();
// Flush Sentry events for this HTTP/RPC request lifecycle
Integration::flushEvents();

Copilot uses AI. Check for mistakes.

// Flush events
Integration::flushEvents();
});
}

Comment on lines 337 to 342
Copy link

@sentry sentry bot Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Removing Integration::flushEvents() from defer callbacks causes Sentry events for HTTP, AMQP, Kafka, and other non-command contexts to be queued but never sent.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The refactoring centralizes event flushing into handleCommandFinished, which only triggers on CommandEvent::AfterExecute. This incorrectly assumes all operations are console commands. For other operations like HTTP requests, AMQP/Kafka consumers, and crontab/async jobs, the defer callbacks finish the transaction, but CommandEvent::AfterExecute never fires. Consequently, the new centralized flushing mechanism is never invoked for these contexts, leading to Sentry tracing data being queued indefinitely and ultimately lost.

💡 Suggested Fix

Restore the Integration::flushEvents() calls within the defer() callbacks for each non-command event handler (HTTP, AMQP, Kafka, Crontab, AsyncQueue). This ensures that events are flushed correctly at the end of each respective operation's lifecycle, independent of the command event system.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/src/Tracing/Listener/EventHandleListener.php#L337-L342

Potential issue: The refactoring centralizes event flushing into
`handleCommandFinished`, which only triggers on `CommandEvent::AfterExecute`. This
incorrectly assumes all operations are console commands. For other operations like HTTP
requests, AMQP/Kafka consumers, and crontab/async jobs, the `defer` callbacks finish the
transaction, but `CommandEvent::AfterExecute` never fires. Consequently, the new
centralized flushing mechanism is never invoked for these contexts, leading to Sentry
tracing data being queued indefinitely and ultimately lost.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7923473

Expand Down Expand Up @@ -538,9 +535,6 @@ protected function handleCrontabTaskStarting(CrontabEvent\BeforeExecute $event):

// Finish transaction
$transaction->finish();
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of Integration::flushEvents() from this defer callback is incorrect. This defer callback handles crontab task lifecycles, which are completely separate from command execution lifecycles. The Integration::flushEvents() in handleCommandFinished's finally block (line 457) only executes for CommandEvent\AfterExecute events (CLI commands), not for crontab tasks.

Without this flush call, Sentry events generated during crontab task execution will not be sent to Sentry, resulting in data loss. The flush call must be restored here.

Suggested change
$transaction->finish();
$transaction->finish();
// Flush Sentry events generated during crontab task execution
Integration::flushEvents();

Copilot uses AI. Check for mistakes.

// Flush events
Integration::flushEvents();
});
}

Expand Down Expand Up @@ -607,9 +601,6 @@ protected function handleAmqpMessageProcessing(AmqpEvent\BeforeConsume $event):

// Finish transaction
$transaction->finish();
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of Integration::flushEvents() from this defer callback is incorrect. This defer callback handles AMQP message processing lifecycles, which are completely separate from command execution lifecycles. The Integration::flushEvents() in handleCommandFinished's finally block (line 457) only executes for CommandEvent\AfterExecute events (CLI commands), not for AMQP message consumption.

Without this flush call, Sentry events generated during AMQP message processing will not be sent to Sentry, resulting in data loss. The flush call must be restored here.

Suggested change
$transaction->finish();
$transaction->finish();
// Flush Sentry events generated during AMQP message processing
Integration::flushEvents();

Copilot uses AI. Check for mistakes.

// Flush events
Integration::flushEvents();
});
}

Expand Down Expand Up @@ -675,9 +666,6 @@ protected function handleKafkaMessageProcessing(KafkaEvent\BeforeConsume $event)

// Finish transaction
$transaction->finish();
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of Integration::flushEvents() from this defer callback is incorrect. This defer callback handles Kafka message processing lifecycles, which are completely separate from command execution lifecycles. The Integration::flushEvents() in handleCommandFinished's finally block (line 457) only executes for CommandEvent\AfterExecute events (CLI commands), not for Kafka message consumption.

Without this flush call, Sentry events generated during Kafka message processing will not be sent to Sentry, resulting in data loss. The flush call must be restored here.

Suggested change
$transaction->finish();
$transaction->finish();
// Flush Sentry events generated during Kafka message processing
Integration::flushEvents();

Copilot uses AI. Check for mistakes.

// Flush events
Integration::flushEvents();
});
}

Expand Down Expand Up @@ -725,9 +713,6 @@ protected function handleAsyncQueueJobProcessing(AsyncQueueEvent\BeforeHandle $e

// Finish transaction
$transaction->finish();
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of Integration::flushEvents() from this defer callback is incorrect. This defer callback handles async queue job processing lifecycles, which are completely separate from command execution lifecycles. The Integration::flushEvents() in handleCommandFinished's finally block (line 457) only executes for CommandEvent\AfterExecute events (CLI commands), not for async queue job processing.

Without this flush call, Sentry events generated during async queue job processing will not be sent to Sentry, resulting in data loss. The flush call must be restored here.

Suggested change
$transaction->finish();
$transaction->finish();
// Flush Sentry events generated during async queue job processing
Integration::flushEvents();

Copilot uses AI. Check for mistakes.

// Flush events
Integration::flushEvents();
});
}

Expand Down