getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
566 stars 203 forks source link

ArgumentNullException When No DSN Is Configured (4.X) #3134

Closed Silvenga closed 2 months ago

Silvenga commented 3 months ago

Package

Sentry

.NET Flavor

.NET

.NET Version

8.0.0

OS

Any (not platform specific)

SDK Version

8.0.0

Self-Hosted Sentry Version

No response

Steps to Reproduce

When starting Sentry with no DSN configured (say, locally), Sentry now throws an exception.

Expected Result

No exception should be thrown when no DSN is specified - be it null or empty string. Having no DSN should disable the Sentry SDK (where possible).

Actual Result

storage-1     | [2024-02-08 15:37:48.1108 FATAL Diagnostics] Application startup exception
storage-1     | Autofac.Core.DependencyResolutionException: An exception was thrown while activating λ:System.Func`1[[Sentry.IHub, Sentry, Version=4.0.3.0, Culture=neutral, PublicKeyToken=fba2ec45388e2af0]].
storage-1     |  ---> 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')
storage-1     |    at Sentry.Internal.SettingLocator.GetDsn()
storage-1     |    at Sentry.SentrySdk.InitHub(SentryOptions options)
storage-1     |    at Sentry.Extensions.Logging.Extensions.DependencyInjection.ServiceCollectionExtensions.<>c__0`1.<AddSentry>b__0_3(IServiceProvider c)
storage-1     |    at Autofac.Extensions.DependencyInjection.AutofacRegistration.<>c__DisplayClass4_0.<Register>b__1(IComponentContext context, IEnumerable`1 parameters)
storage-1     |    at Autofac.Core.Activators.Delegate.DelegateActivator.ActivateInstance(IComponentContext context, IEnumerable`1 parameters)
storage-1     |    at Autofac.Core.Activators.Delegate.DelegateActivator.<ConfigurePipeline>b__2_0(ResolveRequestContext context, Action`1 next)
storage-1     |    at Autofac.Core.Resolving.Middleware.DelegateMiddleware.Execute(ResolveRequestContext context, Action`1 next)
storage-1     |    at Autofac.Core.Resolving.Pipeline.ResolvePipelineBuilder.<>c__DisplayClass14_0.<BuildPipeline>b__1(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Resolving.Middleware.DisposalTrackingMiddleware.Execute(ResolveRequestContext context, Action`1 next)
storage-1     |    at Autofac.Core.Resolving.Pipeline.ResolvePipelineBuilder.<>c__DisplayClass14_0.<BuildPipeline>b__1(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Resolving.Middleware.ActivatorErrorHandlingMiddleware.Execute(ResolveRequestContext context, Action`1 next)
storage-1     |    --- End of inner exception stack trace ---
storage-1     |    at Autofac.Core.Resolving.Middleware.ActivatorErrorHandlingMiddleware.Execute(ResolveRequestContext context, Action`1 next)
storage-1     |    at Autofac.Core.Resolving.Pipeline.ResolvePipelineBuilder.<>c__DisplayClass14_0.<BuildPipeline>b__1(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Pipeline.ResolvePipeline.Invoke(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Resolving.Middleware.RegistrationPipelineInvokeMiddleware.Execute(ResolveRequestContext context, Action`1 next)
storage-1     |    at Autofac.Core.Resolving.Pipeline.ResolvePipelineBuilder.<>c__DisplayClass14_0.<BuildPipeline>b__1(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Resolving.Middleware.SharingMiddleware.<>c__DisplayClass5_0.<Execute>b__0()
storage-1     |    at Autofac.Core.Lifetime.LifetimeScope.CreateSharedInstance(Guid id, Func`1 creator)
storage-1     |    at Autofac.Core.Lifetime.LifetimeScope.CreateSharedInstance(Guid primaryId, Nullable`1 qualifyingId, Func`1 creator)
storage-1     |    at Autofac.Core.Resolving.Middleware.SharingMiddleware.Execute(ResolveRequestContext context, Action`1 next)
storage-1     |    at Autofac.Core.Resolving.Pipeline.ResolvePipelineBuilder.<>c__DisplayClass14_0.<BuildPipeline>b__1(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Resolving.Pipeline.ResolvePipelineBuilder.<>c__DisplayClass14_0.<BuildPipeline>b__1(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Resolving.Middleware.CircularDependencyDetectorMiddleware.Execute(ResolveRequestContext context, Action`1 next)
storage-1     |    at Autofac.Core.Resolving.Pipeline.ResolvePipelineBuilder.<>c__DisplayClass14_0.<BuildPipeline>b__1(ResolveRequestContext context)
storage-1     |    at Autofac.Extensions.DependencyInjection.KeyedServiceMiddleware.Execute(ResolveRequestContext context, Action`1 next)
storage-1     |    at Autofac.Core.Resolving.Pipeline.ResolvePipelineBuilder.<>c__DisplayClass14_0.<BuildPipeline>b__1(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Pipeline.ResolvePipeline.Invoke(ResolveRequestContext context)
storage-1     |    at Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(ISharingLifetimeScope currentOperationScope, ResolveRequest& request)
storage-1     |    at Autofac.Core.Resolving.ResolveOperation.ExecuteOperation(ResolveRequest& request)
storage-1     |    at Autofac.Core.Resolving.ResolveOperation.Execute(ResolveRequest& request)
storage-1     |    at Autofac.Core.Lifetime.LifetimeScope.ResolveComponent(ResolveRequest& request)
storage-1     |    at Autofac.Core.Lifetime.LifetimeScope.Autofac.IComponentContext.ResolveComponent(ResolveRequest& request)
storage-1     |    at Autofac.ResolutionExtensions.TryResolveService(IComponentContext context, Service service, IEnumerable`1 parameters, Object& instance)
storage-1     |    at Autofac.ResolutionExtensions.ResolveOptionalService(IComponentContext context, Service service, IEnumerable`1 parameters)
storage-1     |    at Autofac.ResolutionExtensions.ResolveOptional(IComponentContext context, Type serviceType, IEnumerable`1 parameters)
storage-1     |    at Autofac.ResolutionExtensions.ResolveOptional(IComponentContext context, Type serviceType)
storage-1     |    at Autofac.Extensions.DependencyInjection.AutofacServiceProvider.GetService(Type serviceType)
storage-1     |    at Microsoft.Extensions.Internal.ActivatorUtilities.ConstructorMatcher.CreateInstance(IServiceProvider provider)
storage-1     |    at Microsoft.Extensions.Internal.ActivatorUtilities.CreateInstance(IServiceProvider provider, Type instanceType, Object[] parameters)
storage-1     |    at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.ReflectionMiddlewareBinder.CreateMiddleware(RequestDelegate next)
storage-1     |    at Microsoft.AspNetCore.Builder.ApplicationBuilder.Build()
storage-1     |    at Microsoft.AspNetCore.Hosting.GenericWebHostService.StartAsync(CancellationToken cancellationToken)storage-1     |    at Microsoft.Extensions.Hosting.Internal.Host.<StartAsync>b__15_1(IHostedService service, CancellationToken token)
storage-1     |    at Microsoft.Extensions.Hosting.Internal.Host.ForeachService[T](IEnumerable`1 services, CancellationToken token, Boolean concurrent, Boolean abortOnFirstException, List`1 exceptions, Func`3 operation)
storage-1     |    at Microsoft.Extensions.Hosting.Internal.Host.StartAsync(CancellationToken cancellationToken)
storage-1     |    at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.RunAsync(IHost host, CancellationToken token)
storage-1     | [2024-02-08 15:37:48.1108 ERROR Host] Hosting failed to start
bitsandfoxes commented 3 months ago

Thanks for opening this issue. We'll be fixing that.

bitsandfoxes commented 3 months ago

Also got mentioned here: https://github.com/getsentry/sentry-dotnet/pull/2655#issuecomment-1933675566

jamescrosswell commented 3 months ago

@Silvenga this is by design. It's one of the changes we made in the 4.0 release.

If you want to disable Sentry, you should provide the disabled DSN (which is string.Empty rather than null).

If you want to conditionally disable Sentry, that could be done by altering your call to UseSentry slightly - see here for an example.

Silvenga commented 3 months ago

@jamescrosswell regarding this approach - https://github.com/getsentry/sentry-dotnet/pull/2655#issuecomment-1933675566

How would this be configured?

Silvenga commented 3 months ago

To be clear, we are talking about not using the IConfiguration system.

Unless you are saying that when the Action<SentryAspNetCoreOptions> is called that Sentry has already pulled the DSN from the environment?

Silvenga commented 3 months ago

Ugh, no this isn't great either way.

If you are using multiple independent Sentry integrations e.g. NLog and ASP.NET Core, then one of them MUST specify a default empty string. But what if you conditionally enable one of the integrations e.g. ASP.NET, but keep the other one e.g. NLog (as it's much harder to disable conditionally) - if you don't setup the ASP.NET, then the NLog side will throw instead.

Sentry should default the DSN to be an empty string and throw on null - not default at null and throw on null.

jamescrosswell commented 3 months ago

If you are using multiple independent Sentry integrations e.g. NLog and ASP.NET Core, then one of them MUST specify a default empty string.

Yes, we did have to headscratch about that as well... which we resolved by adding non-initialising overrides.

Sentry should default the DSN to be an empty string and throw on null - not default at null and throw on null.

That would be one design. The issue we have with that is that new users of Sentry (who don't know anything about DSNs) will often forget to set the DSN and Sentry inexplicably doesn't work for them... no error messages, nothing. That's not a great first experience with the product. We figured if someone has just "forgotten" to configure the SDK correctly, we should be throwing an exception. On the other hand, users who know what they're doing can always explicitly/deliberately disable Sentry using the Disabled DSN.

jamescrosswell commented 3 months ago

@jamescrosswell regarding this approach - #2655 (comment)

How would this be configured?

See further comment in that thread.

Silvenga commented 3 months ago

That comment is where I'm confused -

Unless you are saying that when the Action is called that Sentry has already pulled the DSN from the environment?


which we resolved by https://github.com/getsentry/sentry-dotnet/pull/2928.

How should be do this when configuring NLog via the XML file? We want NLog to configure Sentry if it's the only integration, if the Sentry environment is set.

We need Sentry to not throw, when generically added to different applications. Some are web applications, while others might be service workers.

jamescrosswell commented 3 months ago

How should be do this when configuring NLog via the XML file? We want NLog to configure Sentry if it's the only integration, if the Sentry environment is set.

InitializeSdk is just another property on the options: https://github.com/getsentry/sentry-dotnet/blob/2285a7f3b805136ab858d4f61e456f30b8a1e2c4/src/Sentry.Serilog/SentrySerilogOptions.cs#L12

So this can be configured via Configuration Bindings as well. You can set it to true or to false via XML, a JSON file, and Environment Variable or via code.

I think everything you're talking about should be possible then. If you think you've found an edge case, I'd love to see it, in which case could you put together a small demo illustrating the problem?

I am wondering whether we need to make the same changes to NLog that we made to SeriLog... I don't recall having made any changes to the NLog library around the time we fixed all this... but it does look like it's there:

https://github.com/getsentry/sentry-dotnet/blob/57feab36cabc09723fd33f50f7dccf27c44faec1/src/Sentry.NLog/SentryNLogOptions.cs#L102

Silvenga commented 3 months ago

I think everything you're talking about should be possible then

RE: https://docs.sentry.io/platforms/dotnet/guides/nlog/

The SDK needs to be initialized only once. If a DSN is made available to this integration, by default it will initialize the SDK. If you do not wish to initialize the SDK via this integration, set the InitializeSdk flag to false. Not providing a DSN or leaving it as null instructs the integration not to initialize the SDK and unless another integration initializes it or you call SentrySdk.Init, the SDK will stay disabled.

NLog is configured with a XML file, there isn't a way to conditionally set the DSN to empty string if no other integration configured Sentry or Sentry didn't pick up the environment variable.

Yes, NLog can be configured with code, but that's a non-standard approach as NLog treats logging as a cross-cutting concern that may not actually be controlled by the developer.

At the end of the day, it is expected to be able to use multiple Sentry integrations at the same time. To avoid temporal coupling, each integration should self configure, if initialized first. With this change, developers now have to know about every integration that can possibly initialize the SDK, and ensure only one of them does, AND that integration can set a empty string on the DSN. I currently expect to be able to swap different integrations in and out, and everything should work. But not on 4.X - where we now need custom code in the first place the SDK can be configured.

jamescrosswell commented 3 months ago

NLog is configured with a XML file

Yeah but when you initialize Sentry with NLog, Sentry has to get it's settings from somewhere right? Do those come from the XML file? If so, we can configure InitializeSdk there in the same way that all the other Sentry settings get configured.

I think we might be talking past one another. If you can give me a small sample project (just new something up and add Sentry + NLog), I can take a look at it. Alternatively I can do the same (I created one yesterday to look into another issue, so I could share that).

Silvenga commented 3 months ago

I think we might be talking past one another.

Yeah, I agree. This isn't a purely functional problem in my mind, but also a design problem - increasing the complexity of using Sentry on larger projects and across many teams, to help the beginner out. I get it, Microsoft has been pushing for the same - with an compiler generated Main method and such - but I think the community agrees that this approach only works on trivial projects.

Here's an example of a basic NLog setup. The application crashes on startup with 4.X, but not with 3.X.

https://github.com/Silvenga/sentry-nlog-repro

Imagine deploying this code, ideally, I would configure an environment variable that only exists in the environments that care about Sentry.

jamescrosswell commented 3 months ago

ideally, I would configure an environment variable that only exists in the environments that care about Sentry.

OK that makes sense. Thanks for the demo project @Silvenga!

So you don't want to set it in the XML... and if you disable it in the config then any DSN from the environment variable would be ignored, because the config files take precedence.

In this case there's a fallback mechanism that we can use, which is to set an attribute on the assembly: https://github.com/getsentry/sentry-dotnet/blob/0aed96926720b59f20c6c3e781dce68df99c9e27/src/Sentry/Internal/SettingLocator.cs#L33-L47

So if you add a file to the project (typically called AssembyInfo.cs) with the following contents, you'll end up with the same behaviour as you had in 3.x (which is that if no DSN is provided anywhere, you get a disabled DSN by default):

using Sentry;

[assembly: Dsn(SentryConstants.DisableSdkDsnValue)]

If you do want to enable Sentry on a particular machine, just set the DSN in an environment variable. If you don't set it, this means Sentry will be disabled.

Silvenga commented 3 months ago

So you don't want to set it in the XML... and if you disable it in the config then any DSN from the environment variable would be ignored, because the config files take precedence.

The problem is that we CAN'T set it in the XML.

So if you add a file to the project

GetEntryAssembly can be incredibly iffy, noting under .NET Framework, e.g. https://stackoverflow.com/questions/4277692/getentryassembly-for-web-applications.

How would you propose handing the ASP.NET case?

slonopotamus commented 3 months ago

@Silvenga what do you think about this approach: https://github.com/getsentry/sentry-dotnet/pull/2655#issuecomment-1935550978

slonopotamus commented 3 months ago

Though I personally think that code in ServiceLocator should be changed so that env variable can override empty string.

Silvenga commented 3 months ago

@Silvenga what do you think about this approach: #2655 (comment)

I don't think that would work, given this may not be a ASP.NET Core app.

jamescrosswell commented 3 months ago

Though I personally think that code in ServiceLocator should be changed so that env variable can override empty string.

I think I agree with you there. It would be more consistent with how things are done in ASP.NET Core:

Using the default configuration, the EnvironmentVariablesConfigurationProvider loads configuration from environment variable key-value pairs after reading appsettings.json, appsettings.{Environment}.json, and user secrets. Therefore, key values read from the environment override values read from appsettings.json, appsettings.{Environment}.json, and user secrets.

This probably needs some discussion though as there are probably loads of customers out there who have configured their environments based on the current behaviour... so I think it'd be a breaking change.

@bitsandfoxes @bruno-garcia

bitsandfoxes commented 2 months ago

Considering this closed as we've restored the behaviour in 4.1.1 with https://github.com/getsentry/sentry-dotnet/pull/3147 Please feel free to reopen this if you're still running into issues.

slonopotamus commented 2 months ago

Thanks, 4.1.1 allowed me to upgrade with minor tweaks. I now pass empty string in code and then can dynamically enable sending by setting SENTRY_DSN env var, like in 3.x.