Skip to content

fix(taskworker): add isolation scope to taskworker execution #93998

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.

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
merged 2 commits into from
Jun 20, 2025

Conversation

JoshFerge
Copy link
Member

@JoshFerge JoshFerge commented Jun 20, 2025

tag pollution between taskworker tasks reported in slack, I think this should fix it?

@JoshFerge JoshFerge requested a review from a team as a code owner June 20, 2025 21:14
@JoshFerge JoshFerge requested a review from a team June 20, 2025 21:14
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 20, 2025
Copy link
Member

@cmanallen cmanallen left a comment

Choose a reason for hiding this comment

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

Do you have to call isolation scope before starting the transaction?

@JoshFerge
Copy link
Member Author

Do you have to call isolation scope before starting the transaction?

I think they are independent? I think the scope contains all of the currently set tags and context, and when a transaction is sent, those attributes are applied to the event? I don't think it matters the order of starting, but i'll switch them just to be safe

https://docs.sentry.io/platforms/javascript/enriching-events/scopes/#isolation-scope

@@ -311,6 +311,7 @@ def _execute_activation(task_func: Task[Any, Any], activation: TaskActivation) -
with (
track_memory_usage("taskworker.worker.memory_change"),
sentry_sdk.start_transaction(transaction),
sentry_sdk.isolation_scope(),
Copy link
Member

Choose a reason for hiding this comment

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

Should the transaction be created from the isolation scope as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

reading our docs i'm not sure transactions are usually created on scopes, but i'm not sure. just tagged the python sdk team for review. https://docs.sentry.io/platforms/python/tracing/instrumentation/custom-instrumentation/#accessing-the-current-transaction

Copy link
Member

@sl0thentr0py sl0thentr0py Jun 20, 2025

Choose a reason for hiding this comment

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

as long as the transaction is within the isolation_scope context manager, you don't need to use the scope manually so this will work

@cmanallen
Copy link
Member

@JoshFerge That makes sense. Since we're not applying any tags until after the fork we should be fine in either case.

@@ -311,6 +311,7 @@ def _execute_activation(task_func: Task[Any, Any], activation: TaskActivation) -
with (
track_memory_usage("taskworker.worker.memory_change"),
sentry_sdk.start_transaction(transaction),
sentry_sdk.isolation_scope(),
Copy link
Member

@sl0thentr0py sl0thentr0py Jun 20, 2025

Choose a reason for hiding this comment

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

as long as the transaction is within the isolation_scope context manager, you don't need to use the scope manually so this will work

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@JoshFerge JoshFerge enabled auto-merge (squash) June 20, 2025 21:30
@JoshFerge JoshFerge merged commit 86e843a into master Jun 20, 2025
63 checks passed
@JoshFerge JoshFerge deleted the jferg/add-isolation-scope branch June 20, 2025 21:37
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants