Add IHostBuilder support

[Vandersteen]

As discussed here: https://github.com/getsentry/sentry-dotnet/issues/1001

First quick and dirty commit

My first thought after beginning to add an implementation: would it be bad if it acted 100% like the IWebHostBuilder extension.

The user would have the choice between:

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

And

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

Only 'caveat' right now is that it would add a few classes that would never be used in a WorkerService environment:

• IStartupFilter, SentryStartupFilter
• IRequestPayloadExtractor, FormRequestPayloadExtractor
• IRequestPayloadExtractor, DefaultRequestPayloadExtractor

However i'm not sure this is such an issue, thoughts ?

Also, I haven't taken https://github.com/getsentry/sentry-dotnet/issues/1001#issuecomment-846674053 into consideration

[codecov-commenter]

Codecov Report

Merging #1015 (7c20f88) into main (f84c23b) will increase coverage by 0.01%. The diff coverage is 91.11%.

@@            Coverage Diff             @@
##             main    #1015      +/-   ##
==========================================
+ Coverage   81.43%   81.45%   +0.01%     
==========================================
  Files         193      195       +2     
  Lines        6325     6348      +23     
  Branches     1526     1537      +11     
==========================================
+ Hits         5151     5171      +20     
- Misses        739      740       +1     
- Partials      435      437       +2     

Impacted Files	Coverage Δ
.../Sentry.AspNetCore/SentryAspNetCoreOptionsSetup.cs	100.00% <ø> (ø)
...ry/Extensibility/DefaultRequestPayloadExtractor.cs	81.81% <0.00%> (ø)
...c/Sentry.AspNetCore/SentryHostBuilderExtensions.cs	81.25% <81.25%> (ø)
...DependencyInjection/ServiceCollectionExtensions.cs	100.00% <100.00%> (ø)
...entry.AspNetCore/SentryLoggingBuilderExtensions.cs	100.00% <100.00%> (ø)
...entry.AspNetCore/SentryWebHostBuilderExtensions.cs	81.25% <100.00%> (-7.22%)	:arrow_down:
src/Sentry/SentryClient.cs	76.56% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f84c23b...7c20f88. Read the comment docs.

[Tyrrrz]

Can we converge on just one way of using UseSentry() somehow?

[Vandersteen]

Can we converge on just one way of using UseSentry() somehow?

What exactly do you mean ?

[Tyrrrz]

Can we converge on just one way of using UseSentry() somehow?

What exactly do you mean ?

Sorry. I was referring to how having both the option to put UseSentry() inside ConfigureWebHostDefaults(...) and outside of it seems a bit confusing from user perspective. I wonder if we can do anything about that.

[Vandersteen]

Can we converge on just one way of using UseSentry() somehow?

What exactly do you mean ?

Sorry. I was referring to how having both the option to put UseSentry() inside ConfigureWebHostDefaults(...) and outside of it seems a bit confusing from user perspective. I wonder if we can do anything about that.

I'm not sure we can do anything about that. IWebHostBuilder was the predecessor for IHostBuilder

I understand it could be confusing.

One way I suggested to 'tackle' that problem was explained in the original issue

• UseSentryWeb() on IWebHostBuilder
• UseSentryCore() on IHostBuilder

But that was also not desirable behaviour

[Tyrrrz]

Let's see what @bruno-garcia thinks about this

[bruno-garcia]

Not a fan of the two methods either. Can we phase out the one on IWebHostBuilder then? Can we do everything through IHostBuilder instead?

[Vandersteen]

My 2 cents, so you can make an informed decision

Can we phase out the one on IWebHostBuilder then?

That could indeed be an option, deprecate IWebHostBuilder extensions. However, do note that WebHost.CreateDefaultBuilder will no longer be supported then, forcing users to use Host.CreateDefaultBuilder

While both still seem acceptable ways to create a Host from asp.net core's standpoint. There does not seem to be plans to deprecate/remove WebHost

• https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/web-host?view=aspnetcore-5.0

• This article covers the Web Host, which remains available only for backward compatibility. The ASP.NET Core templates create a .NET Generic Host, which is recommended for all app types.

• https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/generic-host?view=aspnetcore-5.0

// no longer supported
WebHost.CreateDefaultBuilder()
    .UseSentry()

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

As far as I know, both of these are equivalent. I don't see any signs that IWebHostBuilder will be deprecated as well (f.e. it is used in the callback of ConfigureWebHostDefaults)

Can we do everything through IHostBuilder instead?

I can't see anything that you cannot achieve through the IHostBuilder that you can with the IWebHostBuilder

But don't take my word for it :)

[bruno-garcia]

Sounds like we do need to keep both. So one way is: We add the infrastructure for IHostBuilder and update our docs to show only the use of this extension going forward.

We could add a note that UseSentry exists on IWebHostBuilder but is not needed if the one in IHostBuilder was already called.

All of that assumes that we can make them equivalent. If we can add "more" features through IWebHostBuilder this wouldn't really work.

[Vandersteen]

Exactly, at the time of writing, the current IHostBuilder extensions in this PR are a 'literal' copy of the IWebHostBuilder with IWebHostBuilder replaced with IHostBuilder and some namespace / include changes.

[bruno-garcia]

Exactly, at the time of writing, the current IHostBuilder extensions in this PR are a 'literal' copy of the IWebHostBuilder with IWebHostBuilder replaced with IHostBuilder and some namespace / include changes.

Sounds like we could pull the code to a single place and forward the call from both places then?

Is the code reentrant already? We can expect that ppl will call it twice (from each interface).

[Vandersteen]

Sounds like we could pull the code to a single place and forward the call from both places then?

I looked into that, but IHostBuilder and IWebHostBuilder are not the same interface, so code sharing is not easily done

Is the code reentrant already? We can expect that ppl will call it twice (from each interface).

No not yet, I will comment in code where I see issues

[Vandersteen]

I dropped some comments here and there (see above)

In some code bases, I worked around this with a private static bool that keeps track if it is already called or not. I could proceed and resolve those that way, so that calling it on both interfaces does not give strange issues

[bruno-garcia]

In some code bases, I worked around this with a private static bool that keeps track if it is already called or not. I could proceed and resolve those that way, so that calling it on both interfaces does not give strange issues

This strategy won't work in the ASP.NET Core case because even on a normal run of it, 2 containers are created and the first time we'd add (before the static fields are set) would end up on a dead host, and the new one wouldn't have it. See: https://github.com/bruno-garcia/repro-logger-provider-double-instantiation https://github.com/getsentry/sentry-dotnet/issues/103#issuecomment-433882680 https://github.com/aspnet/Hosting/issues/1499

[Vandersteen]

Took the comments into consideration, made AddSentry() idempotent on

• IHostBuilder
• IWebHostBuilder

[Vandersteen]

Fixed those if ('s I found

I'll add some coverage and some samples

[Vandersteen]

I had some trouble running it in .netcoreapp2.1 (IHostingEnvironment was unknown with HostBuilder)

I changed it to fix it, ran all (most) the aspnetcore samples, they still seem to work properly, however I can't check if the issues get to sentry

I changed the generic sample to use the new extension method.

Would you like me to make another sample for the Mvc's (using IHostBuilder instead), or shall I add it as a comment underneath ?

[bruno-garcia]

Great stuff!

Would you like me to make another sample for the Mvc's (using IHostBuilder instead), or shall I add it as a comment underneath ?

I wonder if we're just better off moving over to IHostBuilder on the Sample.AspNetCore.Mvc then. So we have one sample (.Basic) using IWebHostBuilder and the other using IHostBuilder. What do you think?

Btw if you want to join us on Discord, we're online there often: http://discord.gg/sentry

[Vandersteen]

hey @bruno-garcia i've tried to add some tests, but for some reason I can't seem to make them work. I wasn't able to run these tests in debug mode either.

I keep getting:

Diagnostic information:

Remaining (non-bound) argument specifications:
    any Action<IServiceCollection>

All argument specifications:
    any Action<IServiceCollection>

I don't understand how the SentryWebHostBuilderTests pass but not the SentryHostBuilderTests I must be missing something silly.

[Vandersteen]

Ok after a long debugging session, i finally found the issue.

IWebHostBuilder's interface contains

• ConfigureServices(Action<IServiceCollection> a);

While IHostBuilder does not, it has an extension method, that falls back to

• ConfigureServices(Action<HostBuilderContext, IServiceCollection> a)

[Vandersteen]

@bruno-garcia @Tyrrrz Is there anything else I can do ?

[Vandersteen]

Latest commit should've fixed failing tests

[Tyrrrz]

I would also prefer to avoid having a dependency on Sentry.AspNetCore when one needs a generic host integration. It seems counter-intuitive. Sentry.Extensions.Logging is in a weird place right now, in a sense that it should probably be called Sentry.Shared or something, but I think it's still a better place for the generic IHostBuilder stuff.

[Vandersteen]

I understand

How do we proceed with these dependencies:

• Adding the Startupfilter / middleware
• Calling the AddSentry() on servicecollection extensions (and it's depedencies on aspnetcore)
• SentryAspNetCoreOptions
• Microsoft.AspNetCore.Hosting

[Tyrrrz]

I'm also okay with merging as is since the PR has been up for a while, and then improving on it as we go along 😉

[Vandersteen]

I'll leave that up to you guys

[bruno-garcia]

I understand

How do we proceed with these dependencies:

• Adding the Startupfilter / middleware
• Calling the AddSentry() on servicecollection extensions (and it's depedencies on aspnetcore)
• SentryAspNetCoreOptions
• Microsoft.AspNetCore.Hosting

Could we move all of that to Sentry.Extensions.Logging? Oh I see, we'd need to add Microsoft.AspNetCore.Hosting as a dependency or:

  <ItemGroup Condition="'$(TargetFramework)' != 'netstandard2.0'">
    <FrameworkReference Include="Microsoft.AspNetCore.App" />
  </ItemGroup>

Instead of AspNetCore.Hosting, isn't this Microsoft.Extensions.Hosting?

So it's either that or a new Sentry.Extensions.Hosting and have Sentry.AspNetCore depend on it?

[Vandersteen]

I'll take a closer look tomorrow

[Vandersteen]

Well I couldn't keep myself, took a quick gander.

Things like:

• IApplicationBuilder
• IStartupFilter
• HttpContext

All require AspNetCore ref

I believe you won't be able to substitute IHostBuilder.UseSentry() for IWebHostBuilder.UseSentry() completely if you go that path

IHostBuilder.UseSentry() could contain the minimum requirements in a Sentry.Extensions.Hosting but will not be a replacement for IWebHostBuilder.UseSentry()

I don't see anything wrong with that, if it's clear from a usage point of view.

[Vandersteen]

@bruno-garcia Going forward, I believe a new Sentry.Extensions.Hosting might be the best bet. Considering future .NET MAUI support

Do you wish for me to proceed on that path ? Anything special I should know when setting up a new Project ?