-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
There was a problem hiding this 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?
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(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@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(), |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
tag pollution between taskworker tasks reported in slack, I think this should fix it?