Throw ArgumentNullException if DSN is null #2655
Conversation
|
| if (dsn != null) | ||
| _options.Dsn ??= GetEnvironmentVariable(Constants.DsnEnvironmentVariable) | ||
| ?? AssemblyForAttributes?.GetCustomAttribute<DsnAttribute>()?.Dsn; | ||
| if (_options.Dsn is null) |
There was a problem hiding this comment.
Do we always call the locator after any framework bootstrapping phase? Like ASP.NET Core Configuration system? I imagine integration tests would break otherwise but worth testing a sample that loads the DSN from appsettings.json
There was a problem hiding this comment.
Yeah I think it's all covered by the integration tests.
Some manual smoke tests on my part:
ASP.NET Core
Configure Disabled DSN as Assembly Attribute and removing from Options Callback
Setting this in the Directory.Build.props in the Samples directory and removing any other DSN configuration in C# resulted in a DisabledHub (as expected):
<ItemGroup>
<SentryAttributes Include="Sentry.DsnAttribute">
<_Parameter1></_Parameter1>
</SentryAttributes>
</ItemGroup>
Removing DSN as Assembly Attribute and from Options Callback
Removing the Assembly Attribute from the Directory.Build.props in the Samples directory and removing any other DSN configuration in C# code resulted in an ArgumentNullException, as expected:
/usr/local/share/dotnet/dotnet /Users/jamescrosswell/code/sentry-dotnet/samples/Sentry.Samples.AspNetCore.Basic/bin/Debug/net6.0/Sentry.Samples.AspNetCore.Basic.dll
Debug: Logging enabled with ConsoleDiagnosticLogger and min level: Debug
Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'You must supply a DSN to use Sentry.To disable Sentry, pass an empty string: "".See https://docs.sentry.io/platforms/dotnet/configuration/options/#dsn')
at Sentry.Internal.SettingLocator.GetDsn() in /Users/jamescrosswell/code/sentry-dotnet/src/Sentry/Internal/SettingLocator.cs:line 42
| { | ||
| return dsn; | ||
| throw new ArgumentNullException( |
There was a problem hiding this comment.
Also, kind of unrelated, if we can use the new ArgumentNullException.ThrowIfNull(); and friends. We'll help JIT emit faster code.
There was a problem hiding this comment.
I don't think we can use ArgumentNullException.ThrowIfNull() as it doesn't allow you to pass an exception message (only the name of the parameter that was null).
bruno-garcia
changed the title
jamescrosswell
changed the title
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
…getsentry/sentry-dotnet into feat/4.0.0-throw-if-no-dsn-2494
|
So how are we now supposed to write ASP.NET apps that want to conditionally enable Sentry? Before this PR, I unconditionally called But now I can't do that. If I call |
|
Hey @slonopotamus. Looks like we made a mistake there with our assumptions. Based on your input I think we should not have changed it in the first place. We'll fix this asap. |
Hm, I'm not so sure. If @slonopotamus is providing a DSN via an environment variable then it shouldn't throw. I think this is an issue with the config bindings that needs to be worked through. @slonopotamus are you able to provide some minimal sample code illustrarting how you're initialising the SDK? I just want to double check there isn't a way to achieve what you want with the current set of initialization APIs before we throw the baby out with the bathwater. It is really useful to throw an exception when people try to initialize the SDK without supplying a DSN (it saves us a lot of support requests). |
@slonopotamus I think I follow what you're trying to do now. I believe you should be able to achieve what you want with this: builder.WebHost.UseSentry((SentryAspNetCoreOptions options) =>
{
options.Dsn ??= SentryConstants.DisableSdkDsnValue;
});That code will explicitly disable Sentry if the Dsn has not been provided via configuration binding... |
|
Buy that won't fix |
|
Minimal repro: using Microsoft.AspNetCore;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
var webHost = WebHost
.CreateDefaultBuilder()
.UseSentry()
.UseKestrel()
.Configure(
applicationBuilder => applicationBuilder
.UseRouting())
.Build();
await webHost.RunAsync();As I said, with Sentry 3.x, this works the following way:
But with Sentry 4.0.x, this fails: If I use |
OK understood. This is the same behaviour another customer wanted as well - see comment here for a solution. @bitsandfoxes maybe we add that to the migration guide for people who want to force Sentry 4.x to behave like Sentry 3.x in this respect? |
|
Whoops, I missed a link in your comment |
All good. Now I think I understand your problem better, we should be able to improve our docs: |
|
After thinking about it more, I think I will go the different route. I am going to |
|
That's probably more explicit and easier for other dev's on the team to understand yeah 👍 |
Completes #2494
Migration notes
In Sentry.NET SDK 3.x.x and below, it was possible to initialise Sentry with a
DisabledHubby setting the DSN to eitherString.Emptyornull. However this sometimes tripped up people who were new to Sentry and/or had forgotten to configure the DSN in their Sentry options.As such, starting with version 4.x.x of the SDK, if you want to configure a
DisabledHubthen you have to set the DSN toString.Empty. If you ommit the DSN altogether or forget to supply this, Sentry will throw an exception during initialization to let you know you've made a mistake.