Fix: Avoid replacing Transaction Name on ASP.NET Core by null or empty

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Fixes
 
+- Avoid replacing Transaction Name on ASP.NET Core by null or empty ([#1215](https://github.com/getsentry/sentry-dotnet/pull/1215))
 - Ignore DiagnosticSource Integration if no Sampling available ([#1238](https://github.com/getsentry/sentry-dotnet/pull/1238)) 
 
 ### Features

samples/Sentry.Samples.AspNetCore.Mvc/Program.cs

@@ -31,6 +31,8 @@ public static IWebHost BuildWebHost(string[] args) =>
 
                     options.MaxBreadcrumbs = 200;
 
+                    options.TracesSampleRate = 1.0;
+
                     // Set a proxy for outgoing HTTP connections
                     options.HttpProxy = null; // new WebProxy("https://localhost:3128");
 

samples/Sentry.Samples.AspNetCore.Mvc/Startup.cs

@@ -46,6 +46,8 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env)
 
             app.UseStaticFiles();
 
+            app.UseSentryTracing();
+
             app.UseMvc(routes =>
             {
                 routes.MapRoute(

src/Sentry.AspNetCore/ScopeExtensions.cs

@@ -87,7 +87,14 @@ public static void Populate(this Scope scope, HttpContext context, SentryAspNetC
                     scope.SetTag("route.area", area);
                 }
 
-                scope.TransactionName = context.TryGetTransactionName();
+                // Transaction Name may only be available afterward the creation of the Transaction.
+                // In this case, the event will update the transaction name if captured during the
+                // pipeline execution, allowing it to match the correct transaction name as the current
+                // active transaction.
+                if (string.IsNullOrEmpty(scope.TransactionName))
+                {
+                    scope.TransactionName = context.TryGetTransactionName();
+                }
             }
             catch (Exception e)
             {

src/Sentry.AspNetCore/SentryTracingMiddleware.cs

@@ -14,7 +14,7 @@ namespace Sentry.AspNetCore
     /// </summary>
     internal class SentryTracingMiddleware
     {
-        private const string UnknownRouteTransactionName = "Unknown Route";
+        internal const string UnknownRouteTransactionName = "Unknown Route";
         private const string OperationName = "http.server";
 
         private readonly RequestDelegate _next;
@@ -68,12 +68,11 @@ public SentryTracingMiddleware(
                 // again, to account for the other middlewares that may have
                 // ran after ours.
                 var transactionName =
-                    context.TryGetTransactionName() ??
-                    UnknownRouteTransactionName;
+                    context.TryGetTransactionName();
 
                 var transactionContext = traceHeader is not null
-                    ? new TransactionContext(transactionName, OperationName, traceHeader)
-                    : new TransactionContext(transactionName, OperationName);
+                    ? new TransactionContext(transactionName ?? string.Empty, OperationName, traceHeader)
+                    : new TransactionContext(transactionName ?? string.Empty, OperationName);
 
                 var customSamplingContext = new Dictionary<string, object?>(3, StringComparer.Ordinal)
                 {
@@ -112,13 +111,13 @@ public async Task InvokeAsync(HttpContext context)
             }
 
             var transaction = TryStartTransaction(context);
+            var initialName = transaction?.Name;
 
             // Expose the transaction on the scope so that the user
             // can retrieve it and start child spans off of it.
             hub.ConfigureScope(scope =>
             {
                 scope.Transaction = transaction;
-                scope.OnEvaluating += (_, _) => scope.Populate(context, _options);
             });
 
             Exception? exception = null;
@@ -134,22 +133,36 @@ public async Task InvokeAsync(HttpContext context)
             {
                 if (transaction is not null)
                 {
-                    // The routing middleware may have ran after ours, so
-                    // try to get the transaction name again.
-                    if (context.TryGetTransactionName() is { } transactionName)
+                    // The Transaction name was altered during the pipeline execution,
+                    // That could be done by user interference or by some Event Capture
+                    // That triggers ScopeExtensions.Populate.
+                    if (transaction.Name != initialName)
                     {
-                        if (!string.Equals(transaction.Name, transactionName, StringComparison.Ordinal))
-                        {
-                            _options.LogDebug(
-                                "Changed transaction name from '{0}' to '{1}' after request pipeline executed.",
-                                transaction.Name,
-                                transactionName);
-                        }
+                        _options.LogDebug(
+                            "transaction name set from '{0}' to '{1}' during request pipeline execution.",
+                            initialName,
+                            transaction.Name);
+                    }
+                    // try to get the transaction name.
+                    else if (context.TryGetTransactionName() is { } transactionName &&
+                             !string.IsNullOrEmpty(transactionName))
+                    {
+                        _options.LogDebug(
+                            "Changed transaction '{0}', name set to '{1}' after request pipeline executed.",
+                            transaction.SpanId,
+                            transactionName);
 
                         transaction.Name = transactionName;
                     }
 
                     var status = SpanStatusConverter.FromHttpStatusCode(context.Response.StatusCode);
+
+                    // If no Name was found for Transaction, fallback to UnknownRoute name.
+                    if (transaction.Name == string.Empty)
+                    {
+                        transaction.Name = UnknownRouteTransactionName;
+                    }
+
                     if (exception is null)
                     {
                         transaction.Finish(status);

src/Sentry/TransactionTracer.cs

@@ -222,19 +222,14 @@ public ISpan StartChild(string operation) =>
         /// <inheritdoc />
         public void Finish()
         {
-            try
-            {
-                Status ??= SpanStatus.UnknownError;
-                EndTimestamp = DateTimeOffset.UtcNow;
+            Status ??= SpanStatus.UnknownError;
+            EndTimestamp = DateTimeOffset.UtcNow;
 
-                // Client decides whether to discard this transaction based on sampling
-                _hub.CaptureTransaction(new Transaction(this));
-            }
-            finally
-            {
-                // Clear the transaction from the scope
-                _hub.ConfigureScope(scope => scope.ResetTransaction(this));
-            }
+            // Clear the transaction from the scope
+            _hub.ConfigureScope(scope => scope.ResetTransaction(this));
+
+            // Client decides whether to discard this transaction based on sampling
+            _hub.CaptureTransaction(new Transaction(this));
         }
 
         /// <inheritdoc />

test/Sentry.AspNetCore.Tests/ScopeExtensionsTests.cs

@@ -29,6 +29,62 @@ public ScopeExtensionsTests()
             _ = _httpContext.Request.Returns(_httpRequest);
         }
 
+        private class Fixture
+        {
+            public const string ControllerName = "Ctrl";
+            public const string ActionName = "Actn";
+
+            public readonly Scope Scope = new(new SentryOptions());
+            public HttpContext HttpContext { get; } = Substitute.For<HttpContext>();
+
+            public Fixture GetSut(bool addTransaction = true)
+            {
+                if (addTransaction)
+                {
+                    Scope.Transaction = Substitute.For<ITransaction>();
+                }
+
+                var routeFeature = new RoutingFeature
+                {
+                    RouteData = new RouteData
+                    {
+                        Values =
+                        {
+                            {"controller", ControllerName},
+                            {"action", ActionName}
+                        }
+                    }
+                };
+                var features = new FeatureCollection();
+                features.Set<IRoutingFeature>(routeFeature);
+                HttpContext.Features.Returns(features);
+                HttpContext.Request.Method.Returns("GET");
+                return this;
+            }
+
+            public Fixture GetSutWithEmptyRoute(bool addTransaction = true)
+            {
+                if (addTransaction)
+                {
+                    Scope.Transaction = Substitute.For<ITransaction>();
+                }
+                var routeFeature = new RoutingFeature
+                {
+                    RouteData = new RouteData
+                    {
+                        Values = { { "", null } }
+                    }
+                };
+                var features = new FeatureCollection();
+                features.Set<IRoutingFeature>(routeFeature);
+                HttpContext.Features.Returns(features);
+                HttpContext.Request.Method.Returns("GET");
+                return this;
+            }
+        }
+
+        private readonly Fixture _fixture = new();
+
         [Fact]
         public void Populate_Request_Method_SetToScope()
         {
@@ -268,6 +324,7 @@ public void Populate_PayloadExtractors_DoesNotConsiderInvalidResponse(object exp
         [Fact]
         public void Populate_RouteData_SetToScope()
         {
+            // Arrange
             const string controller = "Ctrl";
             const string action = "Actn";
             var routeFeature = new RoutingFeature()
@@ -279,8 +336,10 @@ public void Populate_RouteData_SetToScope()
             _ = _httpContext.Features.Returns(features);
             _ = _httpContext.Request.Method.Returns("GET");
 
+            // Act
             _sut.Populate(_httpContext, SentryAspNetCoreOptions);
 
+            // Assert
             Assert.Equal($"GET {controller}.{action}", _sut.TransactionName);
         }
 
@@ -364,5 +423,50 @@ public void Populate_TraceIdentifier_WhenRequestIdDoesNotMatch_SetAsTag()
 
             Assert.Equal(expected, _sut.Tags[nameof(HttpContext.TraceIdentifier)]);
         }
+
+        [Fact]
+        public void Populate_TransactionAndTransactionNameIsNull_TransactionNameReplaced()
+        {
+            // Arrange
+            var sut = _fixture.GetSut(addTransaction: false);
+            var scope = sut.Scope;
+            var expectedTransactionName = $"GET {Fixture.ControllerName}.{Fixture.ActionName}";
+
+            // Act
+            scope.Populate(_fixture.HttpContext, SentryAspNetCoreOptions);
+
+            // Assert
+            Assert.Equal(expectedTransactionName, scope.TransactionName);
+        }
+
+        [Fact]
+        public void Populate_TransactionIsNullAndRouteNotFound_TransactionNameAsNull()
+        {
+            // Arrange
+            var sut = _fixture.GetSutWithEmptyRoute(addTransaction: false);
+            var scope = sut.Scope;
+
+            // Act
+            scope.Populate(_fixture.HttpContext, SentryAspNetCoreOptions);
+
+            // Assert
+            Assert.Null(scope.TransactionName);
+        }
+
+        [Fact]
+        public void Populate_TransactionNameSet_TransactionNameSkipped()
+        {
+            // Arrange
+            var sut = _fixture.GetSut();
+            var scope = sut.Scope;
+            var expectedRoute = "MyRoute";
+            scope.Transaction.Name = expectedRoute;
+
+            // Act
+            scope.Populate(_fixture.HttpContext, SentryAspNetCoreOptions);
+
+            // Assert
+            Assert.Equal(expectedRoute, scope.TransactionName);
+        }
     }
 }