diff --git a/CHANGELOG.md b/CHANGELOG.md index bfd53f7c45..e957f06a7b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/samples/Sentry.Samples.AspNetCore.Mvc/Program.cs b/samples/Sentry.Samples.AspNetCore.Mvc/Program.cs index 7aabe01832..a21d0c516a 100644 --- a/samples/Sentry.Samples.AspNetCore.Mvc/Program.cs +++ b/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"); diff --git a/samples/Sentry.Samples.AspNetCore.Mvc/Startup.cs b/samples/Sentry.Samples.AspNetCore.Mvc/Startup.cs index e14e698f4d..d7f65b81cb 100644 --- a/samples/Sentry.Samples.AspNetCore.Mvc/Startup.cs +++ b/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( diff --git a/src/Sentry.AspNetCore/ScopeExtensions.cs b/src/Sentry.AspNetCore/ScopeExtensions.cs index a191e7b01a..2e21efdaed 100644 --- a/src/Sentry.AspNetCore/ScopeExtensions.cs +++ b/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) { diff --git a/src/Sentry.AspNetCore/SentryTracingMiddleware.cs b/src/Sentry.AspNetCore/SentryTracingMiddleware.cs index 854f2775db..33e35b37d5 100644 --- a/src/Sentry.AspNetCore/SentryTracingMiddleware.cs +++ b/src/Sentry.AspNetCore/SentryTracingMiddleware.cs @@ -14,7 +14,7 @@ namespace Sentry.AspNetCore /// 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(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); diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 6b7a0ecf58..b8137fecf0 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -222,19 +222,14 @@ public ISpan StartChild(string operation) => /// 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)); } /// diff --git a/test/Sentry.AspNetCore.Tests/ScopeExtensionsTests.cs b/test/Sentry.AspNetCore.Tests/ScopeExtensionsTests.cs index 7e854d9419..8e54589dea 100644 --- a/test/Sentry.AspNetCore.Tests/ScopeExtensionsTests.cs +++ b/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(); + + public Fixture GetSut(bool addTransaction = true) + { + if (addTransaction) + { + Scope.Transaction = Substitute.For(); + } + + var routeFeature = new RoutingFeature + { + RouteData = new RouteData + { + Values = + { + {"controller", ControllerName}, + {"action", ActionName} + } + } + }; + var features = new FeatureCollection(); + features.Set(routeFeature); + HttpContext.Features.Returns(features); + HttpContext.Request.Method.Returns("GET"); + return this; + } + + public Fixture GetSutWithEmptyRoute(bool addTransaction = true) + { + if (addTransaction) + { + Scope.Transaction = Substitute.For(); + } + var routeFeature = new RoutingFeature + { + RouteData = new RouteData + { + Values = { { "", null } } + } + }; + var features = new FeatureCollection(); + features.Set(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); + } } }