getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
575 stars 205 forks source link

How to use Serilog + Sentry + AspNetCore ? #257

Closed PureKrome closed 4 years ago

PureKrome commented 5 years ago

Hi 👋 I'm trying to understand the steps involved to leverging Sentry.IO with an ASP.NET Core 2.2 website BUT using Serilog as the logging mechanism.

Currently, we're using Serilog to log Info+ messages to : Console or . Works great.

But we'd also like to use Sentry.io as a sink for our manual errors or (unexpected) exceptions that could get thrown.

What is the trick to setting this up please?

(for bonus points, we define our serilog settings in our appsettings.xxxx.config file(s) so we don't hardcode anything in the code but use the specific environment files to handle specific sinks and settings -per environment-).

xt0rted commented 5 years ago

👋 I used the Serilog integration in a small test project (ended up sticking with the built-in logging) and found the sample project here a good starting point if you haven't seen it yet.

https://github.com/getsentry/sentry-dotnet/tree/master/samples/Sentry.Samples.AspNetCore.Serilog

PureKrome commented 5 years ago

Looking at that example, I can't see where there's any specific ASPNET Core setup/settings?

               // Add Sentry integration with Serilog
                    // Two levels are used to configure it.
                    // One sets which log level is minimally required to keep a log message as breadcrumbs
                    // The other sets the minimum level for messages to be sent out as events to Sentry
                    .WriteTo.Sentry(s =>
                     {
                         s.MinimumBreadcrumbLevel = LogEventLevel.Debug;
                         s.MinimumEventLevel = LogEventLevel.Error;
                     }))

That's pretty generic I thought -> meaning I could do that on a console app / background tasks.

                // Add Sentry integration
                // It can also be defined via configuration (including appsettings.json)
                // or coded explicitly, via parameter like:
                // .UseSentry("dsn") or .UseSentry(o => o.Dsn = ""; o.Release = "1.0"; ...)
                .UseSentry()

Again ... this feels like it's pretty agnostic and not specific to ASPNET Core.

My goal is to add ASPNET core data to any errors ... and define the sentry sink via configuration. (right now, i have sentry defined in the appsettings.release.config file and it works great .. but is just missing any ASPNET core stuff, like the request payload).

bruno-garcia commented 5 years ago

Hey @PureKrome,

I'm dogfooding the SDK in NuGetTrends and there we got both Serilog and ASP.NET Core so hopefully a useful example.

The packages:

From Sentry you need two: Sentry.AspNetCore and Sentry.Serilog. From Serilog it's up to you of course:

https://github.com/NuGetTrends/nuget-trends/blob/7e5ef0c0d5688c115e9bef1f93df187b9da0a871/src/NuGetTrends.Scheduler/NuGetTrends.Scheduler.csproj#L20-L26

We're also using appsettings.json for everything. Sentry is only enabled in Production there:

https://github.com/NuGetTrends/nuget-trends/blob/7e5ef0c0d5688c115e9bef1f93df187b9da0a871/src/NuGetTrends.Scheduler/appsettings.Production.json#L2-L7

Here you can add anything that would go into:

.UseSentry(o =>
{
    o.Anything
}

That is, anything the framework can take via the configuration system. The Func<SentryEvent, SentryEvent> BeforeSend obviously you can't configure via json.

Note the Serilog config is also all there.

Sentry's Debug (internal logging, to see what the SDK is doing) is enabled, but filtered out only for Info. So my Serilog logs that go to ELK include sentry's Info logs or higher. Since I use this site to dogfood the SDK, I'm curious to see the SDK's execution logs but you might want to just leave the default which doesn't add any self log for Sentry aka: Debug=false.

The Serilog config for Sentry is configuring the Sentry Serilog Sink (Sentry.Serilog):

https://github.com/NuGetTrends/nuget-trends/blob/7e5ef0c0d5688c115e9bef1f93df187b9da0a871/src/NuGetTrends.Scheduler/appsettings.Production.json#L30-L34

I just lower the values here since default would be Info for breadcrumb and Error for event.

Both integrations talk to each other. Meaning, if there's an unhandled exception in the middleware pipeline, the log entries that Serilog had for that request will be sent as breadcrumbs. With request data like headers for example.

If you _logger.LogError(..), within a request, it will also include the request data.

I hope this helps clarify how to configure and how it works. Let me know if something isn't clear.

bruno-garcia commented 5 years ago

With regards to RequestPayload, that's an opt-in:

https://github.com/getsentry/sentry-dotnet/blob/172269a5d769a732cce1b2390492f4b5a0d65f0d/src/Sentry.AspNetCore/SentryAspNetCoreOptions.cs#L37-L44

Headers should always be included. If not, please file a bug and if you can, please help us with a repro.

PureKrome commented 5 years ago

Okay. wow. What an awesome and helpful reply @bruno-garcia !

I had to read it a few times, double check the links to grok things .. but yep. Totally get it.

I've also got a test webapp (file -> new -> aspnet core 2.2 api app) to test this out and it's more or less working. Well, I can get it working 100% following what you've done, but I was hoping for something slightly different. As such, i'm not sure if this should be a different issue or not.

In your example, you're configuring the Sentry sink via configuration. This is awesome and something that I always do because it means I can then define any sink configuration via environmental variables if required and it can easily work.

When I've looked at your configuration, it's the following:

      {
        "Name": "Sentry",
        "Args": {
          "MinimumBreadcrumbLevel": "Debug",
          "MinimumEventLevel": "Warning"
        }
      }

This makes sense ... except ... I've always thought that a sink configuration should include all the options possible to setup/initialize the sink. As such, this has no argument for the Dsn ? I would have thought that I could define most of the common setup properties here, like MaxRequestBodySize or AttachStackTrace etc. I can see that there's already 2x properties defined: MinBreadcrumbs and MinEventLvl.

If I try the following, nothing is logged sentry:

           {
                "Name": "Sentry",
                "Args": {
                    "Dsn": "https://<snipped>"
                }
            }

but ... if i add the following in, it works now:

    "Sentry": {
        "Dsn": "https://<snipped>"
    },

It feels disjointed having to split these two out. I feel like .. if i'm creating the sink via WriteTo (which is what that is there for) I should be able to define most initialization properties there.

So ... a summary based on your awesome post:

I hope this make sense and interest you ....

bruno-garcia commented 5 years ago

The issue here is that we're using 2 separate integrations. Both can actually initialize the SDK but you only want (need) the SDK initialized once.

If you were using only Serilog, outside an ASP.NET Core app, you could define everything in the Serilog config. But if you want to capture request data, capture exceptions in the middleware pipeline, capture errors handled by the error page, and all the other features. then you need Sentry.AspNetCore.

The config blocks Sentry and Serilog are separate because they rely on the ASP.NET Core configuration system. and the packages don't depend on each other. The way they can 'share state' is because both of them depend on Sentry package which has AsyncLocal and the Sentry.AspNetCore pushes a scope for each web request. So now your _logger.LogX entries can be kept per-request and sent as breadcrumbs for example, or include request data.

So it's not like you are configuring the Sentry Serilog Sink outside the Serilog config. You are configuring the Sentry Serilog sink just right. All it's specific to it are those two logging levels. You are configuring "Sentry" via the root Sentry node in the configuration file which can live without Serilog. Still captures unhandled exceptions, including request data. In fact, if you didn't have Serilog, it would capture automatically the log messages as breadcrumbs and/or errors because it integrates with Microsoft.Extensions.Logging out of the box.

Wrt configuring via Serilog: SentrySerilogOptions also derives from SentryOptions so it does have everything to init Sentry:

https://github.com/getsentry/sentry-dotnet/blob/46bf5548d347ad1e206aefa3ea91cc46ab02004f/src/Sentry.Serilog/SentrySerilogOptions.cs#L11

In this example we have only Serilog, without any other integration. Here we initialize it all via the Sink:

https://github.com/getsentry/sentry-dotnet/blob/46bf5548d347ad1e206aefa3ea91cc46ab02004f/samples/Sentry.Samples.Serilog/Program.cs#L16-L25

I understand it's not ideal that the ASP.NET Core and Serilog configuration for Sentry are in different parts of the configuration system but I hope it's at least now clear why it is this way.

klemmchr commented 4 years ago

Not ideal is a quite understatement as Microsoft.Extensions.Logging lives from the availability that I can just plug and unplug any logging frameworks I want to. Sentry is in the end a logging framework like log4net, NLog and Serilog are. If I plug in log4net with Sentry it's working fine. Same for NLog. It should be possible to use Serilog and Sentry side by side without any hassle. Also it shouldn't be needed to configure Sentry in Serilog because this defeats the purpose. If I want to change my logging framework I would need to reintegrate Sentry back from Serilog to it's common integration. That's not convenient.

bruno-garcia commented 4 years ago

@chris579 could you please elaborate what's the hassle?

The requirement to add a dedicated Sink is because Serilog removes the Microsoft.Extensions.Logging backend.

shouldn't be needed to configure Sentry in Serilog because this defeats the purpose.

You expect to set the Serilog levels inside the Sentry configuration? That means a dependency to Serilog.AspNetCore from Sentry.AspNetCore which will make users of any other logging framework unhappy.

All you need to do "extra" is to add Sentry.Serilog and "add the sink" to Serilog as per Serilog's requirement. Nothing else (unless you don't want the default log levels).