Simplify how users configure Sentry with Serilog #2884
Description
Problem statement
Currently it's extremely confusing to configure Sentry to work with Serilog. See this StackOverflow question to get some flavour, and also these issues/PRs:
- Not obvious DSN settings Serilog and ASP .NET Core #1223
- fix: Sentry.Serilog throws with a disabled DSN #2883
See also this comment Originally posted by @kanadaj in #2883 (comment)
@jamescrosswell I just want to highlight the fact that dsn
is an optional argument of WriteTo.Sentry()
:
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.
Analysis
You can provide a Sentry DSN and various other settings via:
- The
SentrySdk.Init
method in an application that's not using a Generic Host - The
SentryWebHostBuilderExtensions.UseSentry
extension method, when configuring the generic host - And/Or the
SentrySinkExtensions.Sentry
extension method when configuring Serilog
At the moment, the default value of initializeSdk
is true
:
sentry-dotnet/src/Sentry.Serilog/SentrySerilogOptions.cs
Lines 7 to 12 in 2285a7f
So if you initialize Sentry using SentrySdk.Init
or UsingSentry
and then you wire up Serilog to use Sentry via the SentrySinkExtensions.Sentry
extension method we provide, without supplying any parameters to that method, you get an exception at runtime because it tries to initialize the SDK without a DSN 😢
A simple example is:
Log.Logger = new LoggerConfiguration()
.WriteTo.Sentry() // <--- Fails to initialize Sentry without a DSN here
.CreateLogger();
var builder = WebApplication.CreateBuilder(args);
builder.WebHost.UseSentry(o =>
{
o.Dsn = "https://b887218a80114d26a9b1a51c5f88e0b4@o447951.ingest.sentry.io/6601807";
o.Debug = true;
});
builder.Host.UseSerilog();
var app = builder.Build();
As a user, you might think, "OK, maybe I have to initialize Sentry first... I'll move the logging initialization after the Sentry initialization" and maybe you do this:
var builder = WebApplication.CreateBuilder(args);
// This will eventually initialize Sentry... but it gets delayed until after the MEL backend has been replaced
builder.WebHost.UseSentry(o =>
{
o.Dsn = "https://b887218a80114d26a9b1a51c5f88e0b4@o447951.ingest.sentry.io/6601807";
o.Debug = true;
});
builder.Host.UseSerilog(new LoggerConfiguration()
.WriteTo.Sentry() // <-- Still fails trying to initialize Sentry with no DSN...
.CreateLogger());
var app = builder.Build();
But that won't work either. In both cases, the code to initialize Serilog (and the call to WriteTo.Sentry()
) will be executed before the code that initializes sentry as a result of the call to UseSentry
(where a DSN is actually provided).
Brainstorming
A couple of different solutions:
- Don't initialize Sentry in
WriteTo.Sentry()
- Different overrides for
WriteTo.Sentry()
- Only automatically initialize Sentry via the first Initializer
- Documentat existing behaviour
Don't initialize Sentry in WriteTo.Sentry()
One simple solution would be not to initialize Sentry via the WriteTo.Sentry()
extension method (ever).
We would require users initiailize Sentry separately via SentrySkd.Init
or UseSentry
.
Different overrides for WriteTo.Sentry()
Rather than changing WriteTo.Sentry()
from always initialize by default to never initialize we could potentially create different overrides for this extension method as @kanadaj suggested. e.g.:
public static LoggerConfiguration Sentry(
this LoggerSinkConfiguration loggerConfiguration,
string dsn, // <-- Make the DSN required... use this override if you want to initialize Sentry
...
);
// Implicitly this one does not initialize the SDK so only takes the logging specific parameters
public static LoggerConfiguration Sentry(
this LoggerSinkConfiguration loggerConfiguration,
LogEventLevel minimumEventLevel = LogEventLevel.Error,
LogEventLevel minimumBreadcrumbLevel = LogEventLevel.Information,
IFormatProvider? formatProvider = null,
ITextFormatter? textFormatter = null
);
Only automatically initialize Sentry via the first Initializer
Another solution could be to detect, when initializing Sentry or one of it's integrations, whether Sentry has already been initialized. By default, the we would only automatically initialize the SDK on the first initialiser that was called. When users wanted to reinitialise the SDK they would need to explicitly pass in a value for initializeSdk: true
to override the default behaviour.
The implementation for this might be complex:
InitializeSdk
is currently abool
. We would need this to change this to be either abool?
or an enum, so that it could store one of three values:true
,false
andimplicit
- Currently the order in which people write their code is not always the order in which it gets executed. When using
SentryWebHostBuilderExtensions.UseSentry
initialization of the SDK is delayed if the MEL has been replaced (e.g. by Serilog):
sentry-dotnet/src/Sentry.AspNetCore/SentryWebHostBuilderExtensions.cs
Lines 79 to 82 in 7584368
We would need a way to delay Serilog trying to initialize the SDK until we can tell whether it has been initialized elsewhere (irrespective of the order in which they initialize Serilog and Sentry). - We'd need to sort out what the expected behaviour of DSK initialization would be in each of the different possible scenarios. Putting this in a table makes me think the code might not be very straight forward:
Document existing behaviour
We could try to document the existing behaviour better to illustrate some common problems people might have (like those outlined above) and how they can use explicit values for InitializeSdk to overcome these.
Recommendation
I'm leaning towards options 1 or 2... and option 2 (Different overrides for WriteTo.Sentry()
) is probably the more flexible so I reckon we go with that.
Metadata
Assignees
Labels
Type
Projects
Status
No status
Status
Done
Activity