Skip to content

ref(track_outcome): use FutureTrackingProducer for everything#119126

Open
bmckerry wants to merge 1 commit into
masterfrom
ben/sentry-use-futuretrackingproducer
Open

ref(track_outcome): use FutureTrackingProducer for everything#119126
bmckerry wants to merge 1 commit into
masterfrom
ben/sentry-use-futuretrackingproducer

Conversation

@bmckerry

@bmckerry bmckerry commented Jul 7, 2026

Copy link
Copy Markdown
Member

getsentry/arroyo#547 introduced a producer abstraction in Arroyo which is essentially the same as TaskProducer but can be used anywhere, not just in tasks.

This PR replaces TaskProducer with FutureTrackingProducer in track_outcome, and cleans up all of the rollout logic now that this is fully rolled out everywhere.

@bmckerry bmckerry requested a review from a team as a code owner July 7, 2026 16:55
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 7, 2026
Comment on lines +176 to 183
outcomes_producer = FutureTrackingProducer(
"sentry.utils.outcomes",
_get_outcomes_producer,
)
billing_producer = SingletonProducer(_get_billing_producer)
billing_tp_name = "sentry.utils.outcomes.billing.taskproducer"
billing_task_producer = get_task_producer(
billing_tp_name, partial(_get_billing_producer, billing_tp_name)
billing_producer = FutureTrackingProducer(
"sentry.utils.outcomes.billing",
_get_billing_producer,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: Replacing SingletonProducer with FutureTrackingProducer may cause outcome loss during server shutdown, as the new producer might close before OutcomeAggregator flushes its buffer.
Severity: HIGH

Suggested Fix

Verify that FutureTrackingProducer registers its atexit handler at construction time, ensuring it closes after OutcomeAggregator flushes during shutdown. Consider adding a test to confirm this shutdown ordering and prevent data loss.

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/utils/outcomes.py#L176-L183

Potential issue: The replacement of `SingletonProducer` with `FutureTrackingProducer`
may reintroduce a shutdown-ordering bug. The previous implementation used an eager
`atexit` handler to ensure the producer remained active until after `OutcomeAggregator`
flushed its buffered outcomes during server shutdown. It is unclear if
`FutureTrackingProducer` provides the same guarantee. If the new producer's shutdown
handler runs before `OutcomeAggregator._atexit_flush()`, any outcomes flushed by the
aggregator during shutdown will be silently lost because the producer will already be
closed.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FutureTrackingProducer has effectively the same behaviour as SingletonProducer on shutdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants