Add SentryOptions.AutoRegisterTracing (#2871)

jamescrosswell · web-flow · commit 65a7bba00a73 · 2023-11-22T13:06:57.000+13:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### Fixes
 
 - `CaptureFailedRequests` and `FailedRequestStatusCodes` are now getting respected by the Cocoa SDK. This is relevant for MAUI apps where requests are getting handled natively. ([#2826](https://github.com/getsentry/sentry-dotnet/issues/2826))
+- Added `SentryOptions.AutoRegisterTracing` for users who need to control registration of Sentry's tracing middleware ([#2871](https://github.com/getsentry/sentry-dotnet/pull/2871))
 
 ### Dependencies
 
diff --git a/src/Sentry.AspNetCore/SentryAspNetCoreOptions.cs b/src/Sentry.AspNetCore/SentryAspNetCoreOptions.cs
@@ -1,3 +1,4 @@
+using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Http;
 using Sentry.Extensibility;
@@ -65,6 +66,21 @@ public class SentryAspNetCoreOptions : SentryLoggingOptions
     /// </remarks>
     public bool AdjustStandardEnvironmentNameCasing { get; set; } = true;
 
+    /// <summary>
+    /// <para>
+    /// When true (by default) Sentry automatically registers its tracing middleware immediately after
+    /// `EndpointRoutingApplicationBuilderExtensions.UseRouting`.
+    /// </para>
+    /// <para>
+    /// If you need to control when Sentry's tracing middleware is registered, you can set
+    /// <see cref="AutoRegisterTracing"/> to false and call
+    /// <see cref="SentryTracingMiddlewareExtensions.UseSentryTracing"/> yourself, sometime after calling
+    /// `EndpointRoutingApplicationBuilderExtensions.UseRouting` and before calling
+    /// `EndpointRoutingApplicationBuilderExtensions.UseEndpoints`.
+    /// </para>
+    /// </summary>
+    public bool AutoRegisterTracing { get; set; } = true;
+
     /// <summary>
     /// Creates a new instance of <see cref="SentryAspNetCoreOptions"/>.
     /// </summary>
diff --git a/src/Sentry.AspNetCore/SentryTracingBuilder.cs b/src/Sentry.AspNetCore/SentryTracingBuilder.cs
@@ -49,12 +49,13 @@ public IApplicationBuilder Use(Func<RequestDelegate, RequestDelegate> middleware
         {
             var options = InnerBuilder.ApplicationServices.GetService<IOptions<SentryAspNetCoreOptions>>();
             var instrumenter = options?.Value.Instrumenter ?? Instrumenter.Sentry;
-            if (instrumenter == Instrumenter.Sentry)
+            var autoRegisterTracing = options?.Value.AutoRegisterTracing ?? true;
+            if (instrumenter == Instrumenter.Sentry && autoRegisterTracing)
             {
-                InnerBuilder.Use(middleware).UseSentryTracing();
+                InnerBuilder.Use(middleware).UseSentryTracingInternal();
                 return this; // Make sure we return the same builder (not the inner builder), for chaining
             }
-            this.StoreInstrumenter(instrumenter); // Saves us from having to resolve the options to make this check again
+            this.StoreRegistrationDecision(false); // Saves us from having to resolve the options to make this check again
         }
 
         InnerBuilder.Use(middleware);
diff --git a/src/Sentry.AspNetCore/SentryTracingMiddlewareExtensions.cs b/src/Sentry.AspNetCore/SentryTracingMiddlewareExtensions.cs
@@ -1,5 +1,8 @@
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
 using Sentry;
 using Sentry.AspNetCore;
+using Sentry.Extensibility;
 using Sentry.Internal.Extensions;
 
 // ReSharper disable once CheckNamespace
@@ -11,32 +14,30 @@ namespace Microsoft.AspNetCore.Builder;
 [EditorBrowsable(EditorBrowsableState.Never)]
 public static class SentryTracingMiddlewareExtensions
 {
+    internal const string AlreadyRegisteredWarning = "Sentry tracing middleware was already registered. This call to UseSentryTracing is unnecessary.";
     private const string UseSentryTracingKey = "__UseSentryTracing";
-    private const string InstrumenterKey = "__SentryInstrumenter";
+    private const string ShouldRegisterKey = "__ShouldRegisterSentryTracing";
 
     internal static bool IsSentryTracingRegistered(this IApplicationBuilder builder)
         => builder.Properties.ContainsKey(UseSentryTracingKey);
-    internal static void StoreInstrumenter(this IApplicationBuilder builder, Instrumenter instrumenter)
-        => builder.Properties[InstrumenterKey] = instrumenter;
+    internal static void StoreRegistrationDecision(this IApplicationBuilder builder, bool shouldRegisterSentryTracing)
+        => builder.Properties[ShouldRegisterKey] = shouldRegisterSentryTracing;
 
     internal static bool ShouldRegisterSentryTracing(this IApplicationBuilder builder)
     {
         if (builder.Properties.ContainsKey(UseSentryTracingKey))
         {
+            // It's already been registered
             return false;
         }
-        if (builder.Properties.TryGetTypedValue(InstrumenterKey, out Instrumenter instrumenter))
+        if (builder.Properties.TryGetTypedValue(ShouldRegisterKey, out bool shouldRegisterSentryTracing))
         {
-            return instrumenter == Instrumenter.Sentry;
+            return shouldRegisterSentryTracing;
         }
         return true;
     }
 
-    /// <summary>
-    /// Adds Sentry's tracing middleware to the pipeline.
-    /// Make sure to place this middleware after <c>UseRouting(...)</c>.
-    /// </summary>
-    public static IApplicationBuilder UseSentryTracing(this IApplicationBuilder builder)
+    internal static IApplicationBuilder UseSentryTracingInternal(this IApplicationBuilder builder)
     {
         // Don't register twice
         if (builder.IsSentryTracingRegistered())
@@ -48,4 +49,21 @@ public static IApplicationBuilder UseSentryTracing(this IApplicationBuilder buil
         builder.UseMiddleware<SentryTracingMiddleware>();
         return builder;
     }
+
+    /// <summary>
+    /// Adds Sentry's tracing middleware to the pipeline.
+    /// Make sure to place this middleware after <c>UseRouting(...)</c>.
+    /// </summary>
+    public static IApplicationBuilder UseSentryTracing(this IApplicationBuilder builder)
+    {
+        if (!builder.IsSentryTracingRegistered())
+        {
+            return builder.UseSentryTracingInternal();
+        }
+        // Warn on multiple calls
+        var log = builder.ApplicationServices.GetService<ILoggerFactory>()
+            ?.CreateLogger<SentryTracingMiddleware>();
+        log?.LogWarning(AlreadyRegisteredWarning);
+        return builder;
+    }
 }
diff --git a/test/Sentry.AspNetCore.Tests/ApiApprovalTests.Run.Core3_1.verified.txt b/test/Sentry.AspNetCore.Tests/ApiApprovalTests.Run.Core3_1.verified.txt
@@ -60,6 +60,7 @@ namespace Sentry.AspNetCore
     {
         public SentryAspNetCoreOptions() { }
         public bool AdjustStandardEnvironmentNameCasing { get; set; }
+        public bool AutoRegisterTracing { get; set; }
         public bool FlushOnCompletedRequest { get; set; }
         public bool IncludeActivityData { get; set; }
         public Sentry.Extensibility.RequestSize MaxRequestBodySize { get; set; }
diff --git a/test/Sentry.AspNetCore.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt b/test/Sentry.AspNetCore.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt
@@ -60,6 +60,7 @@ namespace Sentry.AspNetCore
     {
         public SentryAspNetCoreOptions() { }
         public bool AdjustStandardEnvironmentNameCasing { get; set; }
+        public bool AutoRegisterTracing { get; set; }
         public bool FlushOnCompletedRequest { get; set; }
         public bool IncludeActivityData { get; set; }
         public Sentry.Extensibility.RequestSize MaxRequestBodySize { get; set; }
diff --git a/test/Sentry.AspNetCore.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt b/test/Sentry.AspNetCore.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt
@@ -60,6 +60,7 @@ namespace Sentry.AspNetCore
     {
         public SentryAspNetCoreOptions() { }
         public bool AdjustStandardEnvironmentNameCasing { get; set; }
+        public bool AutoRegisterTracing { get; set; }
         public bool FlushOnCompletedRequest { get; set; }
         public bool IncludeActivityData { get; set; }
         public Sentry.Extensibility.RequestSize MaxRequestBodySize { get; set; }
diff --git a/test/Sentry.AspNetCore.Tests/ApiApprovalTests.Run.Net4_8.verified.txt b/test/Sentry.AspNetCore.Tests/ApiApprovalTests.Run.Net4_8.verified.txt
@@ -60,6 +60,7 @@ namespace Sentry.AspNetCore
     {
         public SentryAspNetCoreOptions() { }
         public bool AdjustStandardEnvironmentNameCasing { get; set; }
+        public bool AutoRegisterTracing { get; set; }
         public bool FlushOnCompletedRequest { get; set; }
         public bool IncludeActivityData { get; set; }
         public Sentry.Extensibility.RequestSize MaxRequestBodySize { get; set; }
diff --git a/test/Sentry.AspNetCore.Tests/ApplicationBuilderExtensionsTests.cs b/test/Sentry.AspNetCore.Tests/ApplicationBuilderExtensionsTests.cs
@@ -11,6 +11,8 @@ private class Fixture
         public ISentryEventExceptionProcessor SentryEventExceptionProcessor { get; set; } = Substitute.For<ISentryEventExceptionProcessor>();
         public SentryAspNetCoreOptions SentryAspNetCoreOptions { get; set; } = new();
 
+        public Dictionary<string, object> Properties { get; } = new();
+
         public IApplicationBuilder GetSut()
         {
             var provider = Substitute.For<IServiceProvider>();
@@ -39,6 +41,7 @@ public IApplicationBuilder GetSut()
                     : Enumerable.Empty<ISentryEventExceptionProcessor>());
 
             var sut = Substitute.For<IApplicationBuilder>();
+            sut.Properties.Returns(Properties);
             _ = sut.ApplicationServices.Returns(provider);
             return sut;
         }
diff --git a/test/Sentry.AspNetCore.Tests/SentryTracingBuilderTests.cs b/test/Sentry.AspNetCore.Tests/SentryTracingBuilderTests.cs
@@ -0,0 +1,118 @@
+#if NETCOREAPP3_0_OR_GREATER
+using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.Hosting;
+using Microsoft.AspNetCore.TestHost;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using Sentry.AspNetCore.TestUtils;
+
+namespace Sentry.AspNetCore.Tests;
+
+public class SentryTracingBuilderTests
+{
+    class Fixture
+    {
+        public Action<IServiceCollection> ConfigureServices { get; set; }
+        public Action<IApplicationBuilder> Configure { get; set; }
+        public Action<SentryAspNetCoreOptions> ConfigureOptions { get; set; } = _ => { };
+
+        public (IServiceCollection services, IApplicationBuilder builder) GetSut()
+        {
+            IServiceCollection servicesCollection = null;
+            IApplicationBuilder applicationBuilder = null;
+            _ = new TestServer(new WebHostBuilder()
+                .UseDefaultServiceProvider(di => di.EnableValidation())
+                .UseSentry(ConfigureOptions)
+                .ConfigureServices(services =>
+                {
+                    ConfigureServices?.Invoke(services);
+                    servicesCollection = services;
+                })
+                .Configure(app =>
+                {
+                    Configure?.Invoke(app);
+                    applicationBuilder = app;
+                }));
+            return (servicesCollection, applicationBuilder);
+        }
+    }
+
+    private readonly Fixture _fixture = new();
+
+    [Fact]
+    public void UseRouting_AutoRegisterTracingDisabled_SentryTracingNotRegistered()
+    {
+        // Arrange
+        _fixture.ConfigureServices = services => services.AddRouting();
+        _fixture.Configure = applicationBuilder => applicationBuilder.UseRouting();
+        _fixture.ConfigureOptions = options => options.AutoRegisterTracing = false;
+
+        // Act - implicit
+        var (_, builder) = _fixture.GetSut();
+
+        // Assert
+        builder.IsSentryTracingRegistered().Should().BeFalse();
+    }
+
+    [Fact]
+    public void UseRouting_OtelInstrumentation_SentryTracingNotRegistered()
+    {
+        // Arrange
+        _fixture.ConfigureServices = services => services.AddRouting();
+        _fixture.Configure = applicationBuilder => applicationBuilder.UseRouting();
+        _fixture.ConfigureOptions = options => options.Instrumenter = Instrumenter.OpenTelemetry;
+
+        // Act - implicit
+        var (_, builder) = _fixture.GetSut();
+
+        // Assert
+        builder.IsSentryTracingRegistered().Should().BeFalse();
+    }
+
+    [Fact]
+    public void UseRouting_SentryTracingRegisteredWithoutWarning()
+    {
+        // Arrange
+        var logger = Substitute.For<ILogger<SentryTracingMiddleware>>();
+        var loggerFactory = Substitute.For<ILoggerFactory>();
+        loggerFactory.CreateLogger<SentryTracingMiddleware>().Returns(logger);
+        _fixture.ConfigureServices = services =>
+        {
+            services.AddSingleton(loggerFactory);
+            services.AddRouting();
+        };
+        _fixture.Configure = applicationBuilder => applicationBuilder.UseRouting();
+
+        // Act
+        var (_, builder) = _fixture.GetSut();
+
+        builder.IsSentryTracingRegistered().Should().BeTrue();
+        logger.Received(0).LogWarning(SentryTracingMiddlewareExtensions.AlreadyRegisteredWarning);
+    }
+
+    [Fact]
+    public void UseSentryTracing_AutoRegisterTracing_Warning()
+    {
+        // Arrange
+        var logger = Substitute.For<ILogger<SentryTracingMiddleware>>();
+        var loggerFactory = Substitute.For<ILoggerFactory>();
+        loggerFactory.CreateLogger<SentryTracingMiddleware>().Returns(logger);
+        _fixture.ConfigureServices = services =>
+        {
+            services.AddSingleton(loggerFactory);
+            services.AddRouting();
+        };
+        _fixture.Configure = applicationBuilder =>
+        {
+            applicationBuilder.UseRouting();
+            applicationBuilder.UseSentryTracing();
+        };
+
+        // Act
+        var _ = _fixture.GetSut();
+
+        // Assert
+        logger.Received(1).LogWarning(SentryTracingMiddlewareExtensions.AlreadyRegisteredWarning);
+    }
+}
+#endif

Original file line number	Diff line number	Diff line change
`@@ -49,12 +49,13 @@ public IApplicationBuilder Use(Func<RequestDelegate, RequestDelegate> middleware`
`49`	`49`	`{`
`50`	`50`	`var options = InnerBuilder.ApplicationServices.GetService<IOptions<SentryAspNetCoreOptions>>();`
`51`	`51`	`var instrumenter = options?.Value.Instrumenter ?? Instrumenter.Sentry;`
`52`		`- if (instrumenter == Instrumenter.Sentry)`
	`52`	`+ var autoRegisterTracing = options?.Value.AutoRegisterTracing ?? true;`
	`53`	`+ if (instrumenter == Instrumenter.Sentry && autoRegisterTracing)`
`53`	`54`	`{`
`54`		`- InnerBuilder.Use(middleware).UseSentryTracing();`
	`55`	`+ InnerBuilder.Use(middleware).UseSentryTracingInternal();`
`55`	`56`	`return this; // Make sure we return the same builder (not the inner builder), for chaining`
`56`	`57`	`}`
`57`		`- this.StoreInstrumenter(instrumenter); // Saves us from having to resolve the options to make this check again`
	`58`	`+ this.StoreRegistrationDecision(false); // Saves us from having to resolve the options to make this check again`
`58`	`59`	`}`
`59`	`60`
`60`	`61`	`InnerBuilder.Use(middleware);`
Original file line number	Diff line number	Diff line change
`@@ -60,6 +60,7 @@ namespace Sentry.AspNetCore`
`60`	`60`	`{`
`61`	`61`	`public SentryAspNetCoreOptions() { }`
`62`	`62`	`public bool AdjustStandardEnvironmentNameCasing { get; set; }`
	`63`	`+ public bool AutoRegisterTracing { get; set; }`
`63`	`64`	`public bool FlushOnCompletedRequest { get; set; }`
`64`	`65`	`public bool IncludeActivityData { get; set; }`
`65`	`66`	`public Sentry.Extensibility.RequestSize MaxRequestBodySize { get; set; }`
Original file line number	Diff line number	Diff line change
`@@ -11,6 +11,8 @@ private class Fixture`
`11`	`11`	`public ISentryEventExceptionProcessor SentryEventExceptionProcessor { get; set; } = Substitute.For<ISentryEventExceptionProcessor>();`
`12`	`12`	`public SentryAspNetCoreOptions SentryAspNetCoreOptions { get; set; } = new();`
`13`	`13`
	`14`	`+ public Dictionary<string, object> Properties { get; } = new();`
	`15`	`+`
`14`	`16`	`public IApplicationBuilder GetSut()`
`15`	`17`	`{`
`16`	`18`	`var provider = Substitute.For<IServiceProvider>();`
`@@ -39,6 +41,7 @@ public IApplicationBuilder GetSut()`
`39`	`41`	`: Enumerable.Empty<ISentryEventExceptionProcessor>());`
`40`	`42`
`41`	`43`	`var sut = Substitute.For<IApplicationBuilder>();`
	`44`	`+ sut.Properties.Returns(Properties);`
`42`	`45`	`_ = sut.ApplicationServices.Returns(provider);`
`43`	`46`	`return sut;`
`44`	`47`	`}`