Skip to content

Simplify how users configure Sentry with Serilog #2884

Closed
@jamescrosswell

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:

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:

  1. The SentrySdk.Init method in an application that's not using a Generic Host
  2. The SentryWebHostBuilderExtensions.UseSentry extension method, when configuring the generic host
  3. And/Or the SentrySinkExtensions.Sentry extension method when configuring Serilog

At the moment, the default value of initializeSdk is true:

public class SentrySerilogOptions : SentryOptions
{
/// <summary>
/// Whether to initialize this SDK through this integration
/// </summary>
public bool InitializeSdk { get; set; } = true;

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:

  1. Don't initialize Sentry in WriteTo.Sentry()
  2. Different overrides for WriteTo.Sentry()
  3. Only automatically initialize Sentry via the first Initializer
  4. 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 a bool. We would need this to change this to be either a bool? or an enum, so that it could store one of three values: true, false and implicit
  • 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):
    // The earliest we can hook the SDK initialization code with the framework
    // Initialization happens at a later time depending if the default MEL backend is enabled or not.
    // In case the logging backend was replaced, init happens later, at the StartupFilter
    _ = builder.ConfigureLogging((context, logging) =>

    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:
    image

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Labels

No labels
No labels

Type

No type

Projects

  • Status

    Done

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions