getsentry / sentry-dotnet

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

DefaultTags vs SetTag inconsistency #733

Open tengl opened 3 years ago

tengl commented 3 years ago

Please mark the type framework used:

Please mark the type of the runtime used:

Please mark the NuGet packages used:

I'm a bit confused on how to use DefaultTags and SetTag/SetTags. I need some tags to be sent on all events to identify the environment etc.

If I use the following code to set tags in a generic console host it will include the tag when I use ConfigureScope but not the DefaultTags.

Host.CreateDefaultBuilder(args)
  .ConfigureLogging((hostContext, logging) =>
  {
      logging.AddSentry(options =>
      {
          options.ConfigureScope(scope =>
          {
            scope.SetTag("This tag", "is included");
          });

          options.DefaultTags.Add("Default tag", "Is not included");

          // Need to bind options in AddSentry
          hostContext.Configuration.GetSection("Sentry").Bind(options);
      });
  }).ConfigureServices((hostContext, services) => ...)

If I use the following code to set tags in a ASP.NET Core project it is the other way around, it will include the DefaultTags but not the tags set in ConfigureScope. I can see why the tags on scope might not work here, because the middleware has a different scope. But according to the documentation the scope should be cloned from the previous scope.

Host.CreateDefaultBuilder()
    .ConfigureLogging((hostContext, logging) =>
    {
        logging.ClearProviders();
        logging.AddSimpleConsole();
    })
    .ConfigureWebHostDefaults(webBuilder =>
    {
        webBuilder.UseSentry((hostContext, o) =>
        {
            o.ConfigureScope(scope =>
            {
                scope.SetTag("This tag", "is not included");
            });

            o.DefaultTags.Add("Default tag", "is included");

            // No need to bind options in UseSentry
        });
        webBuilder.UseStartup<Startup>();
    });

Am I doing something wrong or is it a bug? If it is intended to be like this, can you update the documentation so that it is clear when to use DefaultTags or scope?

I would like the code to be similar in all projects (within AddSentry and UseSentry). (I also noticed that in AddSentry I need to bind options but not in UseSentry)

bruno-garcia commented 3 years ago

I can see how this is confusing and we can probably improve this. I'll start explaining why this is happening:

 logging.AddSentry(options =>
      {
          options.ConfigureScope(scope =>
          {
            scope.SetTag("This tag", "is included");
          });

          options.DefaultTags.Add("Default tag", "Is not included");

          // Need to bind options in AddSentry
          hostContext.Configuration.GetSection("Sentry").Bind(options);
      });

In this example, you're enabling Sentry via AddSentry on the ILoggerFactorywhich does invoke theConfigureScopecallback you've added. But Doesn't do anything withDefaultTags` (which was added to the options object but right now it seems to be used only by the ASP.NET Core integration).

We could fix this by making sure DefaultTags is also added when using the AddSentry hook in the ILoggerFactory. At least it would be consistent: You can add tags either way and it'll work as expected.

webBuilder.UseSentry((hostContext, o) =>
{
    o.ConfigureScope(scope =>
    {
        scope.SetTag("This tag", "is not included");
    });

    o.DefaultTags.Add("Default tag", "is included");

});
webBuilder.UseStartup<Startup>();

Here I'd expect ConfigureScope to be invoked, seems like a bug that it's not. And DefaultTags as it was added exactly for this use case (to bind via appsettings.json and got tested on ASP.NET Core).

So the actions for us here to improve are:

One more thing I noticed, on the ILoggerFactory you have to bind the settings manually: hostContext.Configuration.GetSection("Sentry").Bind(options);

Can we move that to Sentry.Extensions.Logging too and have it bind automatically same as ASP.NET Core?

Is this something you could contribute a PR?

tengl commented 3 years ago

Great @bruno-garcia Thanks for confirming my suspicion. I'd like to contribute a PR but I don't have the time at the moment. It's not urgent for us since there is a way around it. I will look into in later, if no one else does it before me.