Added non-initialising overload to SerilogSinkExtensions

[jamescrosswell]

Resolves #2884

Problem statement

Previously configuring Sentry to work with Serilog was extremely error prone. If you initialized 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:

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();

Whilst it looks like that should work, the initialization code in the call to WriteTo.Sentry() would be executed before the initialization code in the call to UseSentry (where a DSN is actually provided) and the user would get an error that was not obvious to resolve.

Solution

We have changed one of the WriteTo.Sentry overrides and added another.

    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 = null,
        LogEventLevel? minimumBreadcrumbLevel = null,
        IFormatProvider? formatProvider = null,
        ITextFormatter? textFormatter = null
        );

The first override has been changed such that the dsn parameter is no longer optional. This is the override that should be used when users do want to initialize the Sentry SDK at the same time as setting up a Sentry Serilog Sink.

The second override doesn't take any dsn parameter... indeed it only accepts the limited set of arguments concerned with configuring the Sentry Serilog Sink itself. This is the variant that will be used if no arguments are supplied. Thus the sample code above will work as expected.

[bruno-garcia]

There's a ton of refactoring mixed with the change in behavior which makes code review really difficult. Ideally try to keep refactoring to a minimum in PRs that are fixing or adding a feature. PRs done just for refactoring are much simpler to review

https://www.codewithjason.com/dont-mix-refactorings-behavior-changes/

[jamescrosswell]

There's a ton of refactoring mixed with the change in behavior which makes code review really difficult.

I removed almost all the refactoring... the only one I left in there is to remove redundant default values for minimumBreadcrumbLevel and minimumEventLevel...

I put the refactoring in a separate PR, in case we want to consider it:

• https://github.com/getsentry/sentry-dotnet/pull/2941/files

[bitsandfoxes]

Thanks for this! LGTM!