fix: Sentry.Serilog throws with a disabled DSN

[jamescrosswell]

Resolves #2866

Passing a disabled DSN when initialising Sentry via the Serilog integration results in an ArgumentNullException:

Log.Logger = new LoggerConfiguration().WriteTo.Sentry(dsn: "").CreateLogger();

Note

I think the fix/workaround to the initial bug/issue that triggered this change is to pass initializeSdk: false into the WriteTo.Sentry extension method. Ideally this wouldn't be necessary and I've created a new issue #2884 to try to solve that.

[bruno-garcia]

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.

[vaind]

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.

[jamescrosswell]

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).

[jamescrosswell]

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.

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 initializeSdk: false in the arguments if Sentry has already been initialised.

[bruno-garcia]

@kanadaj thoughts?

[kanadaj]

Any reason not to just print a warning or error if someone runs the code without a DSN configured?

[bruno-garcia]

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.

We can turn back on people using this for 10 years or anything between now and then. The "" behavior is in all SDKs and well documented understood. "" isnt' default anywhere so in shares the intention of disabling things, without having to recompile code or whatever sometimes via config file.

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).

Logging can help here. Even if Debug=false we could print here, without a way to turn it off.

[jamescrosswell]

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 LogError rather than a LogDebug. I'm wondering why we'd do that instead of throwing an exception though.

There are two scenarios:

1. Someone has supplied a disabled DSN: ""... in this case they have deliberately configured a disabled DSN
2. No DSN has been supllied... in this case, it's possible (if they're a long time user of Sentry) that they know the .NET SDK also treats that as a disabled DSN and this is a lazy way to provide it. However it's also possible (even likely, especially for new Sentry users) that they left out he DSN by mistake. In this scenario we can
   • Either silently fail... reverting to a disabled hub. Everything seems fine. The app runs but Sentry doesn't work. If the user is smart enough to turn on SentryOptions.Debug and run the app again and check their logs (which pretty much only the experienced Sentry users know to do) they could work through that issue. More often though, either people give up or they reach out to us... at which point we tell them how to turn SentryOptions.Debug on and/or ask what value they're supplying for the DSN.
   • Or fail early... throwing an exception, with an error message explaining a DSN hasn't been supplied

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.

[bruno-garcia]

I'm fine with throwing if null (I even reviewed and approved the original change). The issue was that the SDK was already initialized or so at least the user thought so. And this is making now obvious what we didn't realize: That the first call to Init (UseSentry) actually isn't the first one to run. And the DSN should be provided to Serilog instead of M.E.L.

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?

[jamescrosswell]

Yeah that all makes sense.

How to communicate this to users is the challenge now.

I've added a link to your comment in the related issue where we hopefully deal with that.

[kanadaj]

@jamescrosswell I just want to highlight the fact that dsn is an optional argument of WriteTo.Sentry():

sentry-dotnet/src/Sentry.Serilog/SentrySinkExtensions.cs

Line 86 in 0ea0f0a

string? dsn = null,

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 IConfiguration unlike UseSentry().

I guess a solution would be to have 2 alternative calls with WriteTo.Sentry(), one that takes a non-null string as the first argument, and one that takes a boolean for initializeSdk as the first non-optional argument.