-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(deno): Clear pre-existing OTel global before registering TracerProvider #19723
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
Changes from 2 commits
235c683
1c1a149
e083e2a
cb4f1f8
0f123f8
a815bff
59205fd
00531da
90ad535
d7e540a
2ff86c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment, no direct action required: I'm fine with these unit tests for now but to be clear these don't prove that the fix works as intended in an actual app. Long-term I'd like us to at least add one e2e app for Deno (or an integration tests setup like for Node) to more reliably verify this. This goes back to my main review comment that I don't think we fully grasped the scope of the current behavior yet. If we had such a test, we could more reliably say that at least some spans are sent. |
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.
ml: I'm wondering if this backfires for people using Sentry with a custom OTel setup or deliberately with Deno's native tracing (OTLP exporter). The good news is that we don't document this setup for Deno, so I think we can just ignore it for the moment and walk back on this change if anyone complains.Update: I just saw that we gate this function call with
skipOpenTelemetrySetup, so users can opt out of it. That's good. So I guess the worst consequence here is that anyone using native tracing with Sentry might need to set this flag now. Which we can classify as afixbecause that's how we intended the SDK to work anyway. Downgraded from logaf M to LThere 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.
I think it's actually even more of a fix than that. If someone is using native Deno OTel tracing, then this will register Sentry as the global trace provider. If you're using Sentry in Deno, that's almost certainly what you want to see happen (and what would happen, if nothing else was getting in before our initialization).
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.
sounds reasonable. Let's roll with it!