sentry-dotnet icon indicating copy to clipboard operation
sentry-dotnet copied to clipboard

DefaultTags vs SetTag inconsistency

Open tengl opened this issue 4 years ago • 2 comments

Please mark the type framework used:

  • [ ] ASP.NET MVC
  • [ ] ASP.NET Web API (OWIN)
  • [x] ASP.NET Core
  • [ ] WPF
  • [ ] WinForms
  • [ ] Xamarin
  • [ ] Other:

Please mark the type of the runtime used:

  • [ ] .NET Framework
  • [ ] Mono
  • [x] .NET Core
  • Version: 3.1

Please mark the NuGet packages used:

  • [ ] Sentry
  • [ ] Sentry.Serilog
  • [ ] Sentry.NLog
  • [ ] Sentry.Log4Net
  • [x] Sentry.Extensions.Logging
  • [x] Sentry.AspNetCore
  • Version: 2.1.8

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)

tengl avatar Jan 12 '21 08:01 tengl

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:

  • Code reading Options DefaultTags should move from the Sentry.AspNetCore package to Sentry.Extensions.Logging so it works on both IHostBuilder and ILoggerFactory.
  • On Sentry.AspNetCore, when initializing via IHostBuilder we should make sure to invoke the ConfigureScope callbacks registered. Similarly how it works on ILoggerFactory.AddSentry.

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?

bruno-garcia avatar Jan 12 '21 16:01 bruno-garcia

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.

tengl avatar Jan 13 '21 11:01 tengl