Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extensions for IHostBuilder #190

Closed
LordMike opened this issue May 7, 2019 · 30 comments
Closed

Add extensions for IHostBuilder #190

LordMike opened this issue May 7, 2019 · 30 comments
Assignees
Labels

Comments

@LordMike
Copy link

LordMike commented May 7, 2019

The current implementation for MS's DI is for IWebHostBuilder, but given the new Generic Hosts, I think more and more will use the IHostBuilder builder.

Please add a .UseSentry() for IHostBuilder. :)

@bruno-garcia bruno-garcia added Feature New feature or request help wanted labels May 10, 2019
@bruno-garcia
Copy link
Member

True this is pending. Would be great to get some help on this.

@xt0rted
Copy link
Contributor

xt0rted commented Jul 29, 2019

I'm migrating a console app over to the new generic host and in order to use appsettings.json for my config with the ILoggingBuilder.AddSentry() method I needed to adjust my setup like so:

 var builder = Host.CreateDefaultBuilder()
     .ConfigureLogging((ctx, loggingBuilder) =>
     {
+        loggingBuilder.Services.Configure<SentryLoggingOptions>(ctx.Configuration.GetSection("Sentry"));
         loggingBuilder.AddSentry();
     })

To make this more reusable I've added a helper function:

public static class SentryHostBuilderExtensions
{
    public static IHostBuilder UseSentry(this IHostBuilder builder) =>
        builder.ConfigureLogging((ctx, logging) =>
        {
            logging.Services.Configure<SentryLoggingOptions>(ctx.Configuration.GetSection("Sentry"));

            logging.AddSentry();
        });
}

And then I use it just like the asp.net core integration:

var builder = Host.CreateDefaultBuilder()
    .UseSentry()
    .ConfigureServices((ctx, services) => { ... });

@bruno-garcia
Copy link
Member

This looks straightforward @xt0rted thanks.
Isn't there any other hooks we can add via IHostBuilder? With IWebHostBuilder we have the middleware pipeline so that's an obvious place we add code to. But I'm unaware of IHostBuilder.

Is there a way to capture exceptions of BackgroundServices or whatever else is usually setup?

@xt0rted
Copy link
Contributor

xt0rted commented Aug 1, 2019

@bruno-garcia I'm not seeing any graceful exception handling for these (here's the Host code). In the StopAsync() method it does throw an aggregate exception if anything was thrown inside of the IHostedServices, but that just throws, it doesn't even call logger.LogError(ex) 😞

Here's a question about this and the feature has been moved to the backlog https://github.com/aspnet/Extensions/issues/813.

@bruno-garcia
Copy link
Member

bruno-garcia commented Sep 15, 2019

I just tried the template for ASP.NET Core 3.0 on preview 9 which is supposed to be the last one (3.0 will be released in a week) and it looks like this:

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        });

So until we support Generic host, please just add:

weBuilder.UseSentry();

and it should all work fine.

@Kampfmoehre
Copy link

Kampfmoehre commented Jan 16, 2020

Is there a way to configure Sentry release for the .NET core worker template? I generated a new project with .NET core 3.1 with dotnet new worker and this is how CreateHostBuilder looks like:

    public static IHostBuilder CreateHostBuilder(string[] args) =>
      Host.CreateDefaultBuilder(args)
        .ConfigureServices((hostContext, services) =>
        {
          services.AddHostedService<Worker>();
        });

With the solution from @xt0rted I can add Sentry in general but I am unable to configure it. I would like to set the Release property for example.

Edit: Nevermind, I found the solution:

    public static IHostBuilder UseSentry(this IHostBuilder builder) =>
      builder.ConfigureLogging((context, logging) =>
      {
        IConfigurationSection section = context.Configuration.GetSection("Sentry");

        logging.Services.Configure<SentryLoggingOptions>(section);
        logging.AddSentry((c) =>
        {
          var version = Assembly
            .GetEntryAssembly()
            .GetCustomAttribute<AssemblyInformationalVersionAttribute>()
            .InformationalVersion;
          c.Release = $"my-application@{version}";
        });
      });

@bruno-garcia
Copy link
Member

bruno-garcia commented Jan 16, 2020

@Kampfmoehre do you tag your assemblies? Like AssemblyInformationalVersion or AssemblyVersion? The SDK picks those up automatically.

The docs are here: https://docs.sentry.io/platforms/dotnet/#automatically-discovering-release-version

Code is:

public static SdkVersion GetNameAndVersion(this Assembly asm)
{
var asmName = asm.GetName();
var name = asmName.Name;
var version = asm.GetCustomAttribute<AssemblyInformationalVersionAttribute>()
?.InformationalVersion
?? asmName.Version.ToString();
return new SdkVersion { Name = name, Version = version };
}

internal static class ApplicationVersionLocator
{
public static string GetCurrent() => GetCurrent(Assembly.GetEntryAssembly());
internal static string GetCurrent(Assembly asm)
{
var version = asm?.GetNameAndVersion().Version;
return !string.IsNullOrEmpty(version)
// If it really was on of the following, app would need to be set explicitly since these are defaults.
&& version != "0.0.0"
&& version != "1.0.0"
&& version != "0.0.0.0"
&& version != "1.0.0.0"
? version
: null;
}

public static string GetCurrent()
=> Environment.GetEnvironmentVariable(Constants.ReleaseEnvironmentVariable)
?? ApplicationVersionLocator.GetCurrent();

private readonly Lazy<string> _release = new Lazy<string>(ReleaseLocator.GetCurrent);

@Kampfmoehre
Copy link

Kampfmoehre commented Jan 16, 2020

I don't want to go too much off topic here, but here is a little background:
We use Semantic Release (NodeJS) with a self written plugin (that just uses the Semantic Release CLI) that creates releases. During Semantic Release the Version property in Directory.build.props is set to the version Semantic Release offers. During deploy we use the Sentry CLI to notify Sentry about the deployment.
When I set all those up I stumbled upon a few problems and ended up, using a project-key in combination with the version number to distinguish more between versions. Maybe this is not necessary but at least the examples in the docs use that too.
Would that name be picked up from the Assembly name?

@bruno-garcia
Copy link
Member

The snippet you added to the comment you edited is not needed because the SDK (code I pasted) does exactly the same.

It looks for SENTRY_RELEASE env var, it not set it read values from AssemblyInformationalVersionAttribute which can have a string (useful for adding a git commit hash which btw is automatically patched by sourcelink ) and if that was also not defined, uses the standard AssemblyInfo which is the format 0.0.0.0.

You can control these via Directory.Build.props so sounds like it's what you are doing.

@Kampfmoehre
Copy link

Thank you @bruno-garcia for the info. So would I need to set AssemblyInformationalVersionAttribute to [email protected] for the release to be app@version, or is the my-application part taken from somewhere else?

@bruno-garcia
Copy link
Member

Correct. It'll take what's in either of those assemblies and set as Release. In the order I mentioned.

@ltechera
Copy link

ltechera commented Apr 7, 2021

I just tried the template for ASP.NET Core 3.0 on preview 9 which is supposed to be the last one (3.0 will be released in a week) and it looks like this:

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        });

So until we support Generic host, please just add:

weBuilder.UseSentry();

and it should all work fine.

@bruno-garcia Didn't work for me.. Using this I get an error saying I need to have a Startup class in my console app project.
@xt0rted 's post did the trick!

@bruno-garcia
Copy link
Member

@ltechera Thanks for sharing that. This is something we could improve, adding to our backlog

@rh78
Copy link

rh78 commented Nov 10, 2021

Supporting this! It is missing.

@Bowenf3
Copy link

Bowenf3 commented Dec 20, 2021

Bump - running into issues with this.

@a-legotin
Copy link

any updates here?

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented May 19, 2022

Yes. I picked this up again a couple of weeks ago, starting with reviving #1015. There's a lot of work still to do, and I'm not sure exactly when it will be completed, but I will update this issue when it's ready.

In the meantime, you can use the current release as follows:

When using ASP.NET Core with the Web Host:

WebHost.CreateDefaultBuilder()
    .UseSentry(...);

When using ASP.NET Core with the Generic Host:

Host.CreateDefaultBuilder()
    .ConfigureWebHostDefaults(webBuilder =>
    {
        webBuilder.UseSentry(...);
    });

When using the Generic Host without ASP.NET Core:

Host.CreateDefaultBuilder()
    .ConfigureServices(services =>
    {
        services.AddHostedService<YourHostedService>();
    }
    .ConfigureLogging(loggingBuilder =>
    {
        loggingBuilder.AddSentry(...);
    });

The first two require Sentry.AspNetCore, but you only need Sentry.Extensions.Logging for the last one.

@ryanelian
Copy link

I just hit this issue when trying to configure Sentry and Serilog inside an extension method for IHostBuilder which is used in separate ASP.NET Core Web API project and Generic Host (Worker service) project.

It would be nice if I can UseSentry from the IHostBuilder to configure both ASP.NET Core and Worker service with one method...

@domagojmedo
Copy link

Bumping for support

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Nov 23, 2022

We haven't forgotten about this, there's just been a few other things prioritized higher. For now, use the workaround above. Thanks.

@PMExtra
Copy link

PMExtra commented Mar 22, 2023

@mattjohnsonpint Hurry up please, it's been almost 1 year.
In my project I have some other integrations that have to use IHostBuilder instead of IWebHostBuilder. So I can't integrate my service with SentryMiddleware. It means I have to manually capture exceptions and manually send to Sentry. It's so depressing.

@PMExtra

This comment was marked as resolved.

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Mar 22, 2023

Hi again. Sorry, but this has been de-prioritized because the implementation is quite challenging, and there are already valid ways to register Sentry.

As an example of the problem, consider that the SentryMiddleware is only valid within ASP.NET Core. It would not be useable with a generic host that wasn't hosting ASP.NET Core, because there's no request/response pipeline. So a UseSentry call at the IHostBuilder level would have to try to figure out if the host was a web host or not. It would need to work correctly in both cases, which makes it not necessarily related to ASP.NET Core at all, so likely it would be implemented in Sentry.Extensions.Logging (or a new package entirely). (See #1015 (comment)). But - if we don't implement it in Sentry.AspNetCore, then we have no clean way to register the middleware with the web host. We also lose ability to reference SentryAspNetCoreOptions.

Addressing your comment:

In my project I have some other integrations that have to use IHostBuilder instead of IWebHostBuilder.

If your project is referencing ASP.NET Core, then you can use the ConfigureWebHost extension method to get an IWebHostBuilder from an IHostBuilder.

If your project is not referencing ASP.NET Core, then you can't take advantage of Sentry's middleware anyway. You can initialize via .ConfigureLogging(loggingBuilder => loggingBuilder.AddSentry()) instead.

So I can't integrate my service with SentryMiddleware. It means I have to manually capture exceptions and manually send to Sentry.

I'm not sure what you mean by that. SentryMiddleware isn't something one integrates other things into. You can register your own middleware independently of SentryMiddleware. Can you please provide an example that highlights your use case? Thanks.

Also, the code you gave just appears to be inlining the internals of the existing UseSentry method, and you've already got IWebHostBuilder, so I'm not sure what you're illustrating there?

@PMExtra
Copy link

PMExtra commented Mar 24, 2023

@mattjohnsonpint

I'm using IHostBuilder rather than IWebHostBuilder with ASP.NET Core.

I have some other integrations such as UseAutofac() that have to depends on IHostBuilder rather than IWebHostBuilder.

When I called .ConfigureWebHostDefaults it throws an exception.

@mattjohnsonpint
Copy link
Contributor

Can you give an example of the exception?

Can you try ConfigureWebHost (without "Defaults")?

Thanks.

@mattjohnsonpint
Copy link
Contributor

Also, I'm not sure why it matters that you've got other integrations. You certainly can use both Host and WebHost properties of a WebApplicationBuilder. For example:

var builder = WebApplication.CreateBuilder(args);
builder.Host.UseAutofac();
builder.WebHost.UseSentry();
var app = builder.Build();

Also, I haven't used Autofac in ages, but the docs tell me its actually builder.Host.UseServiceProviderFactory(new AutofacServiceProviderFactory()); - Unless you're using something like ABP Framework.

@PMExtra
Copy link

PMExtra commented Mar 24, 2023

@mattjohnsonpint
Thanks! It's solved.
I'm confused why there be both builder.Host and builder.WebHost, and even they can work together at the same project.
I was wrong to think there is only builder.Host I can use.
And a more confusing thing is that there are actually IHostBuilder.ConfigureWebHost() and IHostBuilder.ConfigureWebHostDefaults() methods. And they will through exceptions.

@mattjohnsonpint
Copy link
Contributor

Glad that worked for you. 🙂

Yeah, it's even more confusing if you take into account all the naming changes that have happened over the years between versions. It certainly is challenging for us to support them all, but we do our best to keep up!

@mattjohnsonpint
Copy link
Contributor

Update

After much deliberation, trial, and error, we've decided not to do this.

The main reason is that the IHostBuilder API is an abstraction used on any type of host. A UseSentry API there would either be limited to a subset of features common to all hosts, or would have to make assumptions about what the real host was.

In other words, host.UseSentry() shouldn't assume that ASP.NET Core is being hosted. It would have to try to figure out if ASP.NET Core was present or not. Making that happen would require placing all the logic within Sentry.AspNetCore (as was done in #1015), and that would not be good for applications that are not using ASP.NET Core.

Additionally, it is too difficult to provide different types of options objects to the configuration delegates. We need SentryAspNetCoreOptions exposed for ASP.NET Core, and we use SentryLoggingOptions when registering with ILoggingBuilder.

Guidance for ASP.NET Core

Use the Sentry.AspNetCore nuget package, and register using an IWebHostBuilder, such as the one exposed by WebApplicationBuilder.WebHost. For example:

var builder = WebApplication.CreateBuilder(args);
builder.WebHost.UseSentry(...);

Related - we will also complete #2275 to make this clearer.

Guidance for Generic Hosts / Worker Services

Use the Sentry.Extensions.Logging package and register with the ILoggingBuilder instance, such as:

var hostBuilder = Host.CreateDefaultBuilder(args);
hostBuilder.ConfigureLogging(loggingBuilder =>
{
    loggingBuilder.AddSentry(...);
});

Related - we will also complete #2274 to make this clearer.

Thanks.

@mattjohnsonpint mattjohnsonpint closed this as not planned Won't fix, can't repro, duplicate, stale Mar 30, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Mobile & Cross Platform SDK Mar 30, 2023
bdach added a commit to bdach/osu-server-spectator that referenced this issue Feb 1, 2024
Before you open the diff, a few paragraphs of explanation.

For ASP.NET Core apps, there appear to be three disparate pathways to
getting events onto sentry.

- The first, and most direct one, is directly calling
  `SentrySdk.CaptureException()`. Generally does what you'd expect it
  to.

- The second is via `IWebHostBuilder.UseSentry()`. This is an
  ASP.NET-ism that works by injecting a sentry-provided middleware into
  the request handling stack. Thus, all requests that fail due to a
  thrown exception will be reported to sentry, as the middleware will
  catch the error, log it, and throw it back up to the rest of the
  ASP.NET stack to handle.

  However, note that *this does not apply to background workers*, as
  they are generally outside of this entire flow. Even if we weren't
  hand-rolling them via singletons instantiated in `Startup`,
  and instead using `IHostedService` / `BackgroundService`
  which is the most correct ASP.NET-ism for that, for a proper
  persistent background service *you can't throw exceptions because
  you'd kill both the background service, and the entire server
  with it*.

- Therefore, the third method, and the one officially recommended by the
  sentry team for background worker use cases
  (getsentry/sentry-dotnet#190 (comment))
  is to use sentry's extension of `Sentry.Extensions.Logging`. Doing
  this will middleman all log calls to `Microsoft.Extensions.Logging`
  and push all errors (and warnings too, I believe) to sentry.

In the context of all of the above,
ppy#215 becomes doubly
relevant; however, because that change requires more infra preparations
and we probably want sentry logging on the replay upload process *now*,
this is the minimal required change to make that happen.

A side effect of this change is that the replay upload errors - which
would be printed to stdout via `Console.WriteLine()` - will still be
printed there, but using ASP.NET's default logging format, which is a
little more... talkative, as the example below shows:

	fail: ScoreUploader[0]
	      Error during score upload
	      System.InvalidOperationException: nuh uh
		 at osu.Server.Spectator.Storage.ThrowingScoreStorage.WriteAsync(Score score) in /home/dachb/Documents/opensource/osu-server-spectator/osu.Server.Spectator/Storage/ThrowingScoreStorage.cs:line 12
		 at osu.Server.Spectator.Hubs.ScoreUploader.Flush() in /home/dachb/Documents/opensource/osu-server-spectator/osu.Server.Spectator/Hubs/ScoreUploader.cs:line 117

Additionally, note that we have two *other* background workers like
`ScoreUploader`, and they are: `MetadataBroadcaster` and
`BuildUserCountUpdater`. I don't consider them anywhere as key as replay
upload, therefore they are left as they are until we can get the full
logging unification changes in.
@Kampfmoehre
Copy link

Since this cost me some time to find, I want to add, when using Host.CreateApplicationBuilder(args); (.NET 7+) for non Web projects Sentry can be added like this

HostApplicationBuilder builder = Host.CreateApplicationBuilder(args);

builder.Logging.AddSentry(o =>
{
  o.Dsn = "...";
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests