Skip to content

Fix propagate_scope=False in ThreadingIntegration #4310

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

Open
wants to merge 6 commits into
base: potel-base
Choose a base branch
from

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Apr 16, 2025

ThreadingIntegration can optionally NOT propagate scope data to threads (propagate_scope=False). In that case, in POTel we were wrapping the thread's task in an isolation_scope():

with sentry_sdk.isolation_scope() as scope:
    return _run_old_run_func()

But as this forks the currently active isolation scope, the thread effectively gets all scope data from the parent isolation scope -- so the scope is actually propagated to the thread, even though it shouldn't be since propagate_scope=False.

We effectively need some way to give the thread a clear isolation scope instead. In this PR, I'm just clearing the forked iso scope, but I'm not sure if this is good enough and if something doesn't need to be done on the OTel side too.

Another option would be to set the iso/current scopes to the initial, empty iso/current scopes instead, before running the thread's target function.

Another change is that in OTel, the spans in the threads, now without a parent, automatically get promoted to transactions. (On master they'd just be orphaned spans, so they wouldn't be taken into account at all.) We probably need to instruct folks to add only_if_parent if they don't want this to happen.

Copy link

codecov bot commented Apr 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.36%. Comparing base (ac75685) to head (f835a19).

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@              Coverage Diff               @@
##           potel-base    #4310      +/-   ##
==============================================
- Coverage       84.38%   84.36%   -0.02%     
==============================================
  Files             144      144              
  Lines           14629    14630       +1     
  Branches         2325     2325              
==============================================
- Hits            12345    12343       -2     
- Misses           1559     1561       +2     
- Partials          725      726       +1     
Files with missing lines Coverage Δ
sentry_sdk/integrations/threading.py 92.30% <100.00%> (-1.45%) ⬇️

... and 1 file with indirect coverage changes

@sentrivana sentrivana marked this pull request as ready for review April 17, 2025 10:44
@sentrivana sentrivana requested a review from a team as a code owner April 17, 2025 10:45
@sentrivana sentrivana requested a review from sl0thentr0py April 17, 2025 10:45
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Looks good. But neel should give his blessings :-)

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