getsentry / sentry-dotnet

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

Duplicate invocations of Sentry SDK initialization conflict and fail silently #832

Closed aleksihakli closed 3 years ago

aleksihakli commented 3 years ago

Environment

How do you use Sentry?

Sentry SaaS (sentry.io)

Which SDK and version?

.NET 5 and Sentry SDK version 3.0.6

Steps to Reproduce

Use both

Only configure options for the later one (see below).

Expected Result

Later invocation either

Reconfiguration or duplicate SDK configuration should IMO either always produce an error on misconfiguration or correctly reconfigure the options, but never fail silently.

Actual Result

The first invocation sets the SDK options and the options in the second invocation are ignored. Sentry does not e.g. enable debugging or performance tracing mode if options clash in the correct fashion.

EDIT: the logging docs do mention that the SDK should only be initialised once. However, the ordering and semantics of initialisations should perhaps be made clearer in the docs?

https://docs.sentry.io/platforms/dotnet/guides/extensions-logging/#options-and-initialization


The following code DOES NOT work:

using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;

namespace Example
{
    public class Program
    {
        public static void Main(string[] args)
        {
            CreateHostBuilder(args).Build().Run();
        }

        public static IHostBuilder CreateHostBuilder(string[] args) =>
            Host.CreateDefaultBuilder(args)
                .ConfigureAppConfiguration((hostingContext, config) =>
                {
                    config.AddEnvironmentVariables();
                })
                .ConfigureLogging((c, l) =>
                {
                    l.AddConfiguration(c.Configuration);
                    l.AddSentry();
                })
                .ConfigureWebHostDefaults(webBuilder =>
                {
                    webBuilder.UseSentry(o =>
                    {
                        o.Debug = true;
                        o.TracesSampleRate = 1.0;
                    });
                    webBuilder.UseStartup<Startup>();
                });
    }
}

image

The following code DOES work:

.ConfigureWebHostDefaults(webBuilder =>
{
    webBuilder.UseSentry(o =>
    {
        o.Debug = true;
        o.TracesSampleRate = 1.0;
    });
    webBuilder.UseStartup<Startup>();
})
.ConfigureLogging((c, l) =>
{
    l.AddConfiguration(c.Configuration);
    l.AddSentry();
})

The following code also DOES work and produces the same end result:

.ConfigureLogging((c, l) =>
{
    l.AddConfiguration(c.Configuration);
    l.AddSentry(o =>
    {
        o.Debug = true;
        o.TracesSampleRate = 1.0;
    });
})
.ConfigureWebHostDefaults(webBuilder =>
{
    webBuilder.UseSentry();
    webBuilder.UseStartup<Startup>();
})

Configuring options in all invocations DOES work correctly but is error prone as forgetting to add options in one place and remembering the strict invocation ordering produces easy-to-make and hard-to-debug bugs:

.ConfigureLogging((c, l) =>
{
    l.AddConfiguration(c.Configuration);
    l.AddSentry(o =>
    {
        o.Debug = true;
        o.TracesSampleRate = 0.05;
    });
})
.ConfigureWebHostDefaults(webBuilder =>
{
    webBuilder.UseSentry(o =>
    {
        o.Debug = true;
        o.TracesSampleRate = 0.05;
    });
    webBuilder.UseStartup<Startup>();
})

I'm assuming that the correct way to initialize the SDK with both error tracing and logging would be something like the following to avoid duplicate initializations? There's not a whole lot of documentation on this.

.ConfigureWebHostDefaults(webBuilder =>
{
    webBuilder.UseStartup<Startup>();
    // Initializing Sentry for error and performance tracing
    // https://docs.sentry.io/platforms/dotnet/guides/aspnetcore/
    webBuilder.UseSentry(o =>
    {
        o.Debug = true;
        o.TracesSampleRate = 1.0;
    });
})
.ConfigureLogging((c, l) =>
{
    l.AddConfiguration(c.Configuration);
    // Adding Sentry integration to Microsoft.Extensions.Logging for Entity Framework integration
    // Note that the SDK only needs to be initialized once
    // https://docs.sentry.io/platforms/dotnet/guides/extensions-logging/#options-and-initialization
    l.AddSentry(o => o.InitializeSdk = false);
});

image

bruno-garcia commented 3 years ago

Thanks for raising the detailed issue.

The logging integration is automatically hooked up also through UseSentry. That's why there's no documentation trying to point you into those double settings you've experimented. Did you have problems with getting events through the logger otherwise?

Agreed that it should reconfigure the SDK at least.

aleksihakli commented 3 years ago

Did you have problems with getting events through the logger otherwise?

Well, we are using Serilog, more on that later, and I'm not 100% sure on the .NET logging system best practices with the multiple integrations, so that's part of the reason on why I'm asking.


@bruno-garcia do you mean that the webBuilder.UseSentry() also instruments the logging, and the l.AddSentry() invocation is obsolete here?

It would be nice to understand more explicitly from the docs on what are the recommended settings when multiple integrations are in use so that no duplicate initializations are done but all the necessary instrumentations are enabled. The things I'm wondering are:

  1. Do I need to setup all the integrations one by one so that they are instrumented correctly?
  2. Do I need to use options.InitializeSdk = false; in all other places when one of the integrations is already set up?
  3. Does the SDK gather EF database requests yet? Does this only happen when Sentry.Extensions.Logging is configured, what does the instrumentation look like for that?

I could of course read through the source code of the Sentry .NET SDK implementation to get clarity on these but I'm configuring infrastructure and Sentry SDKs for a bunch of other projects as well and it would be nice to understand the best practices for multiple instrumentations more clearly, as now I feel like I'm guessing for the right configurations.

We also have another project using Serilog and I'm wondering on what are the best practices in setting up the combinations of packages. The ones that we commonly use are:

Here's an example of Serilog setup on which I'm also wondering about the correctness:

using System;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Hosting;
using Serilog;
using Serilog.Events;

namespace Example
{
    public class Program
    {
        public static int Main(string[] args)
        {
            Log.Logger = new LoggerConfiguration()
                .MinimumLevel.Debug()
                .MinimumLevel.Override("Microsoft", LogEventLevel.Information)
                .Enrich.FromLogContext()
                .WriteTo.Console(outputTemplate:
                  "[{Level}] {Message:lj} {Properties:j}{NewLine}{Exception}")
                .WriteTo.Sentry(o =>
                {
                     // Serilog Sentry integration https://docs.sentry.io/platforms/dotnet/guides/serilog/
                     // Sentry performance tracing configuration
                    o.TracesSampleRate = 0.02;
                })
                .CreateLogger();

            try
            {
                Log.Information("Starting web host");
                CreateHostBuilder(args).Build().Run();
                return 0;
            }
            catch (Exception ex)
            {
                Log.Fatal(ex, "Host terminated unexpectedly");
                return 1;
            }
            finally
            {
                Log.CloseAndFlush();
            }
        }

        public static IHostBuilder CreateHostBuilder(string[] args) =>
            Host.CreateDefaultBuilder(args)
                .UseSerilog()
                .ConfigureAppConfiguration((hostingContext, config) =>
                {
                    config.AddEnvironmentVariables();
                })
                .ConfigureWebHostDefaults(webBuilder =>
                {
                    webBuilder.UseStartup<Startup>();

                    // Adding Sentry integration to server error tracking
                    // https://docs.sentry.io/platforms/dotnet/aspnetcore/
                    // Startup.cs also invokes app.UseSentryTracing()
                    webBuilder.UseSentry(o =>
                    {
                        // Sentry SDK is already initialized previously in Serilog configuration
                        o.InitializeSdk = false;
                    });
                });
    }
}
bruno-garcia commented 3 years ago

Did you have problems with getting events through the logger otherwise?

Well, we are using Serilog, more on that later, and I'm not 100% sure on the .NET logging system best practices with the multiple integrations, so that's part of the reason on why I'm asking.

Sounds like we need to improve our docs on the matter. Until then, to clarify: Serilog does require you add the Serilog package and call a method:

https://docs.sentry.io/platforms/dotnet/guides/serilog/#configuration

.WriteTo.Sentry(o => o.Dsn = "https://examplePublicKey@o0.ingest.sentry.io/0")

Be that ASP.NET Core or WPF or any type of app.

For Microsoft.Extensions.Logging, that's dependant by ASP.NET Core so simply calling UseSentry we can already get breadcrumbs and events from logging.

There's a note on the docs:

This package extends Sentry.Extensions.Logging. This means that besides the ASP.NET Core related features, through this package you'll also get access to all the framework's logging integration and also the features available in the main Sentry SDK.

@bruno-garcia do you mean that the webBuilder.UseSentry() also instruments the logging, and the l.AddSentry() invocation is obsolete here?

If you're just using plain ASP.NET Core (which includes Microsoft.Extensions.Logging out of the box) then yes, only UseSentry does everything. If you use Serilog for example, that replaces the inner workings of the logging infrastructure with itself (aka the logging backend) then you need to add Sentry.Serilog too on top of it.

NuGetTrends uses Sentry on ASP.NET Core with Serilog:

Some relevant parts for reference:

https://github.com/dotnet/nuget-trends/blob/dac67d1bd4707a94063b843571127eb055a4cc4f/src/NuGetTrends.Scheduler/Program.cs#L63-L76

https://github.com/dotnet/nuget-trends/blob/dac67d1bd4707a94063b843571127eb055a4cc4f/src/NuGetTrends.Scheduler/appsettings.Production.json#L15

https://github.com/dotnet/nuget-trends/blob/dac67d1bd4707a94063b843571127eb055a4cc4f/src/NuGetTrends.Scheduler/appsettings.Production.json#L31-L36

It would be nice to understand more explicitly from the docs on what are the recommended settings when multiple integrations are in use so that no duplicate initializations are done but all the necessary instrumentations are enabled. The things I'm wondering are:

  1. Do I need to setup all the integrations one by one so that they are instrumented correctly?

I hope I clarified this above. Generally you can add packages and integrate things as needed (consider ASP.NET Core includes Microsoft.Extensions.Logging as mentioned above so that you don't need to add twice manually.

  1. Do I need to use options.InitializeSdk = false; in all other places when one of the integrations is already set up?

If you don't provide a dsn for an integration it will not try to init Sentry. It will do its job though (i.e: Call Sentry.AddBreadcrumb or Sentry.CaptureException) for as long as Sentry is initialized already. This flag is useful if you want to have integrations but don't want to use them to init because you're going to call Sentry.Init by yourself at the start of the app.

  1. Does the SDK gather EF database requests yet? Does this only happen when Sentry.Extensions.Logging is configured, what does the instrumentation look like for that?

EF Core uses ILogger like any other part of ASP.NET Core, and Sentry.Extensions.Logging captures those crumbs and events without really caring if it's EF Core or something else.

As mentioned above, if install Sentry.AspNetCore and call UseSentry everything just works. THere's no need for any other configuration.

This is the most basic ASP.NET Core app you can have which shows the simplest Sentry integration:

https://github.com/getsentry/sentry-dotnet/tree/main/samples/Sentry.Samples.AspNetCore.Basic

I could of course read through the source code of the Sentry .NET SDK implementation to get clarity on these but I'm configuring infrastructure and Sentry SDKs for a bunch of other projects as well and it would be nice to understand the best practices for multiple instrumentations more clearly, as now I feel like I'm guessing for the right configurations.

For sure, we need to make sure these things are clear. The docs might need to have more clarity then though the points above are mostly in the docs (at least those that I linked to the docs). We'd appreciate some PRs there if you'd like to contribute. You docs repo is here: https://github.com/getsentry/sentry-docs and we have docs on how to contribute to the docs :) https://docs.sentry.io/contributing/

We also have another project using Serilog and I'm wondering on what are the best practices in setting up the combinations of packages. The ones that we commonly use are:

  • Sentry.AspNetCore
  • Sentry.Extensions.Logging
  • Sentry.Serilog

If you're using Sentry.AspNetCore you can pretend Sentry.Extensions.Logging doesn't exist since that depends on it and already configures it.

Serilog as mentioned above is its own thing and needs to be added. Refer to NuGetTrends examples linked above.

Here's an example of Serilog setup on which I'm also wondering about the correctness:

using System;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Hosting;
using Serilog;
using Serilog.Events;

namespace Example
{
    public class Program
    {
        public static int Main(string[] args)
        {
            Log.Logger = new LoggerConfiguration()
                .MinimumLevel.Debug()
                .MinimumLevel.Override("Microsoft", LogEventLevel.Information)
                .Enrich.FromLogContext()
                .WriteTo.Console(outputTemplate:
                  "[{Level}] {Message:lj} {Properties:j}{NewLine}{Exception}")
                .WriteTo.Sentry(o =>
                {
                     // Serilog Sentry integration https://docs.sentry.io/platforms/dotnet/guides/serilog/
                     // Sentry performance tracing configuration
                    o.TracesSampleRate = 0.02;
                })
                .CreateLogger();

            try
            {
                Log.Information("Starting web host");
                CreateHostBuilder(args).Build().Run();
                return 0;
            }
            catch (Exception ex)
            {
                Log.Fatal(ex, "Host terminated unexpectedly");
                return 1;
            }
            finally
            {
                Log.CloseAndFlush();
            }
        }

        public static IHostBuilder CreateHostBuilder(string[] args) =>
            Host.CreateDefaultBuilder(args)
                .UseSerilog()
                .ConfigureAppConfiguration((hostingContext, config) =>
                {
                    config.AddEnvironmentVariables();
                })
                .ConfigureWebHostDefaults(webBuilder =>
                {
                    webBuilder.UseStartup<Startup>();

                    // Adding Sentry integration to server error tracking
                    // https://docs.sentry.io/platforms/dotnet/aspnetcore/
                    // Startup.cs also invokes app.UseSentryTracing()
                    webBuilder.UseSentry(o =>
                    {
                        // Sentry SDK is already initialized previously in Serilog configuration
                        o.InitializeSdk = false;
                    });
                });
    }
}

By reading your configuration it all looks good. But it also means that since Sentry.AspNetCore isn't initializing Sentry, configuration used on appsettings.json will not be picked up. You need to make your configuration available to the integration that's initializing Sentry in your case Serilog.

ASP.NET Core related integration will be picked up though. Only the main Sentry package stuff (that goes through Init that won't take effect if you give an integration that has InitializeSdk = false.

aleksihakli commented 3 years ago

Thanks for the comprehensive answer @bruno-garcia, this makes the correct configuration path a lot easier to understand!

I'm happy with the answers and my issue is solved, but there are two separate things I'd like to comment on, on the design level:

  1. Configuring multiple target platforms and different SDKs, and
  2. Configuring multiple instrumentations for .NET.

1. Configuring multiple target platforms and different SDKs

We are actually using the SENTRY_DSN, SENTRY_RELEASE and SENTRY_ENVIRONMENT variables at the moment because they are universal and easy to supply into all runtimes where we are utilizing Sentry SDK (e.g. Python and JavaScript in addition to .NET).

Only traces sample rate config is missing from the current environment variable based configuration.

I think it would be nice to have e.g. SENTRY_TRACESSAMPLERATE environment variable available for toggling on the tracing universally for all SDKs, so that there would be no need to do code level configuration manually.


2. Configuring the multiple instrumentations for .NET

Because I have to initialize .NET Core AND Serilog instrumentations for the .NET SDK there is also fairly high risk for making config errors on the code level unless I supply the exact same configuration keys for all of the initializations, if only the first SDK initialization is meaningful. I'm doing the same instrumentations and writing instructions for a bunch of projects with multiple devs participating so I can't assume everyone will get familiar with the inner workings of the SDK.

Of course this is already a recognized issue here, and it's of course possible to write config or initialization code that avoids these errors. However, I don't think that's ideal developer experience for people just looking to correctly configure error and performance tracing with minimal effort.

Using environment variable based configuration, one option would be to use Sentry__TracesSampleRate environment variable with .NET which would translate to Sentry:TracesSampleRate config flag with ASP.NET Core integration, right? That would be picked up by all the different SDK initialization and only have to be configured once in one place instead of multiple places in code (if errors are to be avoided). Of course supplying all of the config keys such as Sentry__Dsn and Sentry__Environment would be viable for .NET Core as well, but these configurations only work for the .NET side using the AddEnvironmentVariables invocation in ConfigureAppConfiguration, and even that's dependant on the prefix that AddEnvironmentVariables is using, so it's error prone and not portable to e.g. Python or JS SDKs.

Using file based configuration would either require keeping the configuration in the repository or baking it into the target build artifacts or runtime environment, and I don't think that's ideal because development and operations / SRE can have different configuration targets and needs that differ based on runtime environments (local vs. QA vs. production environments for example). Packaging file based configurations to e.g. Docker images or injecting them to runtime environments can be a hurdle, and environment variable based configuration is much more common with e.g. PaaS platforms.


SENTRY_TRACESSAMPLERATE flag would actually solve both the .NET SDK and multiple target platform SDK issues in a clean way, because

  1. it could be used as a cross platform standard configuration key, and
  2. by loading config from common standard environment variable there would be no need to supply the tracing sample rate configuration to SDK initialization and instrumentation calls in the correct order either, if one is using the current examples from the Sentry docs, where the initialization may be done through multiple .UseSentry() and .WriteTo.Sentry() configuration paths, which are ordering dependant.