Skip to content
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

Disable reporting if DSN is not set #27

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

mbudde
Copy link
Contributor

@mbudde mbudde commented Oct 14, 2024

No description provided.

@pmb0
Copy link
Member

pmb0 commented Nov 18, 2024

Thanks!

@pmb0 pmb0 merged commit 4175742 into heiseonline:master Nov 18, 2024
@rabbiveesh
Copy link
Contributor

hey, the change to sample breaks existing code really badly - anything that created a transaction now suddenly gets undef during testing. One of the commits in #30 fixes this.
Unless i'm not understanding something, it never tried reporting anything when there's no dsn; i've been using this for years and in tests i have no DSN and everything is hunky-dory.

@rabbiveesh
Copy link
Contributor

In fact, i had already written this behavior in #6 ; what wasn't working about that?

@mbudde
Copy link
Contributor Author

mbudde commented Nov 28, 2024

In fact, i had already written this behavior in #6 ; what wasn't working about that?

The motivation is having a way to completely disable Sentry::SDK (integrations should not be initialized) such that Sentry::SDK->capture_exeception et al are just no-ops and there are no potential side-effects (performance or bugs) from the integrations. But looking at the Python SDK it looks like it is handled by having a default no-op client before initialization and if an empty DSN is set the SDK is initialized but doesn't send events. That might be a better way to do it.

@rabbiveesh
Copy link
Contributor

Oh, so then this is the wrong thing to do b/c this breaks any integration that was starting a transaction.
If you wanna disable the integrations, then just don't always load them. in #31 I implement this (it matches better to other SDKs where there's a defaultIntegrations function exported)

@mbudde mbudde deleted the disable-reporting branch January 23, 2025 13:49
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.

3 participants