-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
fix: Sentry.Serilog throws with a disabled DSN #2883
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.
downside of this fix is that adding Sentry on Serilog without specifying a DSN it will silently do nothing. While before (before we started changing this) you needed to specify ""
explicitly.
Maybe we should revert the original PR that broke this, i.e. throwing for an empty DSN. I now see that the issue was an internal suggestion with a strong counterargument #2494 that doesn't seem to be resolved, at least not in the issue. It is still time to revert this without user headaches because a final release isn't out. |
I think that goes against the principle of fail early. We got frequent support requests from people who couldn't get Sentry working because they hadn't supplied a DSN... so not only did we not fail early, but people got all the way passed "should I give up on this" to contact our support channel for something that should/would have been really obvious if we had have just thrown an exception for what is obviously a misconfiguration. I get that this will be a change for people who've been using the API for years, but for anyone who is new to Sentry, I think it's a small but big improvement. The alternative is to go back to failing silently and leaving people to scratch their heads and either give up or reach out to us on Discord/GitHub (taking hours or days to resolve something that could have been resolved in minutes). |
Yeah agreed, I reversed that particular part of the change. I think the solution is not to provide a fake/disabled DSN but to supply |
@kanadaj thoughts? |
Any reason not to just print a warning or error if someone runs the code without a DSN configured? |
We can turn back on people using this for 10 years or anything between now and then. The
Logging can help here. Even if Debug=false we could print here, without a way to turn it off. |
We could... so make it a There are two scenarios:
Other than "existing users don't need to change anything", I'm not seeing any advantages of the silent fail approach over fail early. It seems like it's designed specifically to trip new users up and make it less likely their first experience with Sentry will be a positive one. We could do LogError instead of throwing an exception. But that error would never make it through to Sentry.io and it would kind of water down the whole fail early approach. Once again, we'd only be failing there in a way that people would notice if they checked their Debug output. Some people don't do that. Everyone notices when the application fails to start with an exception though. We can change this back, but I think it would be a step backwards for the SDK usability overall. |
I'm fine with throwing if How to communicate this to users is the challenge now. Should we bake this whole context in the exception message? Perhaps if we check loaded modules and see some Sentry.Extensions.Logging also loaded? And if only Sentry.Serilog then it's fine to crash as it's not this edge case? |
Yeah that all makes sense.
I've added a link to your comment in the related issue where we hopefully deal with that. |
@jamescrosswell I just want to highlight the fact that
So supplying the dsn is required, but the dsn argument is not? It's kind of super confusing as a user of the library, especially since the Serilog provider doesn't seem to read the value from I guess a solution would be to have 2 alternative calls with |
Resolves #2866
Passing a disabled DSN when initialising Sentry via the Serilog integration results in an
ArgumentNullException
:Note
I think the fix/workaround to the initial bug/issue that triggered this change is to pass
initializeSdk: false
into theWriteTo.Sentry
extension method. Ideally this wouldn't be necessary and I've created a new issue #2884 to try to solve that.