Serilog + WorkerService + IHostBuilder

[Vandersteen]

I've been searching for quite a while how to make Serilog and Sentry work together in a WorkerService environment (.net core > 3)
I've come to the conclusion that this is not easily done / possible right now due to some limitation:

Known issue:

• There is no IHostBuilder extension method for sentry

Unable to 'hack' together what I need by using internals / copy pasting

• The current IApplicationBuilder is marked internal (

  sentry-dotnet/src/Sentry.AspNetCore/ApplicationBuilderExtensions.cs

  Line 25 in 351f264

  internal static class ApplicationBuilderExtensions

  )
• The current IServiceCollectionExtension method is marked internal (

  sentry-dotnet/src/Sentry.AspNetCore/Extensions/DependencyInjection/ServiceCollectionExtensions.cs

  Line 14 in 351f264

  internal static class ServiceCollectionExtensions

  )
• Some classes used in these methods are also marked internal

Work around

The current 'work-around' I have is using the Serilog sink and let it initialise the SDK, and add the following in ConfigureServices:

WARNING This is not production tested, just to make it work locally

services.TryAddTransient<IHub>((_) => HubAdapter.Instance);
services.TryAddTransient<ISentryClient>((_) => HubAdapter.Instance);
services.TryAddTransient<ISentryScopeManager>((_) => HubAdapter.Instance);

However this way you lose the extensibility of Sentry together with DI

There might be other ways, in which case feel free to add them :)

Proposal

First an observation, IWebHostBuilder on a high level does 2 things (i know it's a bit reductive):

• (1) Integrates Sentry with the AspNetCore host (DI, Logging, HostLifetime, ...)
• (2) Decorates the request pipeline using a middleware

Proposed changes:

• Add IHostBuilder extension method
  • UseSentryCore that handles (1)
• Add IWebHostBuilder extension method
  • UseSentryWeb that handles (2)
• Keep IWebHostBuilder extension method
  • UseSentry for backwards compatibility

Analysis

This is after a short, preliminary & quick swoop through the code base:

Split following classes into core and web (or add methods to support them)

• https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry.AspNetCore/Extensions/DependencyInjection/ServiceCollectionExtensions.cs
• https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry.AspNetCore/SentryWebHostBuilderExtensions.cs
• https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry.AspNetCore/SentryAspNetCoreOptions.cs
  • Optional, you could just add to the summary that some of the options have no effect in core

I might be able to free some time for a PR once this proposal is finalised

[Tyrrrz]

@bruno-garcia thoughts? Would we add another package or shoehorn this new UseSentry() into Sentry.Extensions.Logging?
Also, I think UseSentryCore() is redundant, we can call all of them UseSentry() and just resolve based on type (IHostBuilder vs IWebHostBuilder etc).

[bruno-garcia]

Thanks for taking the time to look at this. This came up a while ago (#190) and it's a constant ask.
I agree with @Tyrrrz that we should stick to UseSentry() as opposed to the alternative names, they'd be extending different classes.

It's ideal that we keep a single entrypoint (that can do N things, for example as you mentioned DI, Logging and also decorating the request pipeline). That's because the goal is for us to have as little friction to add the SDK as possible.

For that reason I believe it's best we have UseSentry on IWebHostBuilder also do whatever IHostBuilder.UseSentry does. The challenge is for it not to do it if the user for some reason already called it. Things for example like TryAdd in DI.

So docs for ASP.NET Core should continue to be:

Host.CreateDefaultBuilder(args)
    .ConfigureWebHostDefaults(webBuilder =>
    {
        webBuilder.UseSentry()
        webBuilder.UseStartup<Startup>();
    });

As opposed to:

Host.CreateDefaultBuilder(args)
    .UseSentry()
    .ConfigureWebHostDefaults(webBuilder =>
    {
        webBuilder.UseSentry()
        webBuilder.UseStartup<Startup>();
    });

We could then have docs for worker projects that rely only on IHostBuilder.

Would we add another package or shoehorn this new UseSentry() into Sentry.Extensions.Logging?

I believe that makes sense. We don't need to have a 1:1 parity to the ASP.NET packages. Today Sentry.Extensions.Logging is the 'lowest' package in terms of dependency one can take. It wasn't ever bundled with Sentry.AspNetCore as it's obvious one could take only the M.E.L bits without building an ASP.NET Core app but would someone build anything on .NET Core today without the M.E.L integration? I don't believe so since the built-in logging types (ILogger) exists in this package. So for as long as we're not taking new dependencies into Sentry.E.L we can put this "common" bits in there. There can be exceptions (as we did with M.E.Http) though so if needed, lets discuss.

Once again thanks a lot for doing this analysis and for offering to contribute with a PR. I always suggest folks to open a quick and dirty draft PR without any tests or docs (CI doesn't need to pass) to discuss things before putting too much effort. It can help us talk about the design before the final touches happen.

[bruno-garcia]

To keep in mind: https://github.com/dotnet/maui/blob/main/src/Core/src/Hosting/IAppHostBuilder.cs#L10

[Tyrrrz]

Also maybe related to (or even duplicate of?) #190

[Vandersteen]

Ok, thank you for the feedback.
I'll work on a first draft tomorrow

[mattjohnsonpint]

Blocked on #190. Keeping this open for followup after that is done.

[jamescrosswell]

Since we decided not to implement #190, maybe this also needs to be closed?