Skip to content

Patch TracerProvider if it already exists #4455

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 11 commits into from
Jun 12, 2025

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Jun 10, 2025

If a TracerProvider already exists, patch it. TracerProvider is a singleton, so if we aren't the first ones setting it up, we need to use the existing one.

In tests, reset TracerProvider after each test so that we start with a clean slate and it gets set up anew on init.

Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.78%. Comparing base (f5fb6e7) to head (199485a).
Report is 2 commits behind head on potel-base.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sentry_sdk/opentelemetry/tracing.py 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           potel-base    #4455      +/-   ##
==============================================
- Coverage       84.80%   84.78%   -0.02%     
==============================================
  Files             144      144              
  Lines           14728    14742      +14     
  Branches         2343     2346       +3     
==============================================
+ Hits            12490    12499       +9     
- Misses           1523     1525       +2     
- Partials          715      718       +3     
Files with missing lines Coverage Δ
sentry_sdk/tracing.py 80.35% <ø> (ø)
sentry_sdk/opentelemetry/tracing.py 90.00% <82.35%> (-10.00%) ⬇️

... and 2 files with indirect coverage changes

@sentrivana sentrivana marked this pull request as ready for review June 11, 2025 11:47
@sentrivana sentrivana requested a review from a team as a code owner June 11, 2025 11:47
@sentrivana sentrivana requested a review from sl0thentr0py June 11, 2025 11:47
Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

just a question

there's also the case where someone adds a custom sampler but let's just ignore that for now, worst case we tell people to update their sampler to derive from sentry's sampler if they really want even more custom logic

for span_processor in existing_span_processors:
if isinstance(span_processor, SentrySpanProcessor):
break
else:
Copy link
Member

Choose a reason for hiding this comment

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

lol til there is for else in python

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is kinda obscure. I used to avoid it for that reason but I somewhat like it now


if _TRACER_PROVIDER is not None:
logger.debug("[Tracing] Detected an existing TracerProvider, patching")
tracer_provider = trace.get_tracer_provider()
Copy link
Member

Choose a reason for hiding this comment

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

is _TRACER_PROVIDER above different than what get_tracer_provider returns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's some extra logic tied around get_tracer_provider, see here

Copy link
Member

Choose a reason for hiding this comment

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

but that branch only runs when it's None, anyway doesn't matter that much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got you now. Thought this was about potentially not using _TRACER_PROVIDER at all and switching to get_tracer_provider.

Yeah we can just use _TRACER_PROVIDER on this line, the get_tracer_provider call here is redundant. Will change.

@sentrivana
Copy link
Contributor Author

there's also the case where someone adds a custom sampler but let's just ignore that for now, worst case we tell people to update their sampler to derive from sentry's sampler if they really want even more custom logic

We can also skip setting the sampler here and tell people to do it themselves. Arguably just overwriting it here silently is not great, but at the same time, we have so much important custom logic in the Sentry sampler that it kinda feels like it should be there from the get go to be able to use the whole thing meaningfully.

@sentrivana sentrivana merged commit 6ad4031 into potel-base Jun 12, 2025
123 of 124 checks passed
@sentrivana sentrivana deleted the ivana/potel/dual-setup-ii branch June 12, 2025 09:42
@sl0thentr0py
Copy link
Member

We can also skip setting the sampler here and tell people to do it themselves.

No, the default experience is more important than custom use cases.

JS exposes a skipOpentelemetrySetup
https://github.com/getsentry/sentry-javascript/blob/61940fc22052e9005043f77c7355db1742d1d78c/packages/node/src/sdk/index.ts#L154

sl0thentr0py pushed a commit that referenced this pull request Jun 12, 2025
If a `TracerProvider` already exists, patch it. `TracerProvider` is a
singleton, so if we aren't the first ones setting it up, we need to use
the existing one.

In tests, reset `TracerProvider` after each test so that we start with a
clean slate and it gets set up anew on init.
sl0thentr0py pushed a commit that referenced this pull request Jun 12, 2025
If a `TracerProvider` already exists, patch it. `TracerProvider` is a
singleton, so if we aren't the first ones setting it up, we need to use
the existing one.

In tests, reset `TracerProvider` after each test so that we start with a
clean slate and it gets set up anew on init.
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