Skip to content

Commit cff966f

Browse files
lucas-zimermanlucas-zimerman
authored
Fix: Avoid replacing Transaction Name on ASP.NET Core by null or empty (#1215)
* avoid TransactionName as null.
1 parent f820e11 commit cff966f

File tree

7 files changed

+153
-29
lines changed

7 files changed

+153
-29
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
### Fixes
66

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

910
### Features

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ public static IWebHost BuildWebHost(string[] args) =>
3131

3232
options.MaxBreadcrumbs = 200;
3333

34+
options.TracesSampleRate = 1.0;
35+
3436
// Set a proxy for outgoing HTTP connections
3537
options.HttpProxy = null; // new WebProxy("https://localhost:3128");
3638

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env)
4646

4747
app.UseStaticFiles();
4848

49+
app.UseSentryTracing();
50+
4951
app.UseMvc(routes =>
5052
{
5153
routes.MapRoute(

‎src/Sentry.AspNetCore/ScopeExtensions.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,14 @@ public static void Populate(this Scope scope, HttpContext context, SentryAspNetC
8787
scope.SetTag("route.area", area);
8888
}
8989

90-
scope.TransactionName = context.TryGetTransactionName();
90+
// Transaction Name may only be available afterward the creation of the Transaction.
91+
// In this case, the event will update the transaction name if captured during the
92+
// pipeline execution, allowing it to match the correct transaction name as the current
93+
// active transaction.
94+
if (string.IsNullOrEmpty(scope.TransactionName))
95+
{
96+
scope.TransactionName = context.TryGetTransactionName();
97+
}
9198
}
9299
catch (Exception e)
93100
{

‎src/Sentry.AspNetCore/SentryTracingMiddleware.cs‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ namespace Sentry.AspNetCore
1414
/// </summary>
1515
internal class SentryTracingMiddleware
1616
{
17-
private const string UnknownRouteTransactionName = "Unknown Route";
17+
internal const string UnknownRouteTransactionName = "Unknown Route";
1818
private const string OperationName = "http.server";
1919

2020
private readonly RequestDelegate _next;
@@ -68,12 +68,11 @@ public SentryTracingMiddleware(
6868
// again, to account for the other middlewares that may have
6969
// ran after ours.
7070
var transactionName =
71-
context.TryGetTransactionName() ??
72-
UnknownRouteTransactionName;
71+
context.TryGetTransactionName();
7372

7473
var transactionContext = traceHeader is not null
75-
? new TransactionContext(transactionName, OperationName, traceHeader)
76-
: new TransactionContext(transactionName, OperationName);
74+
? new TransactionContext(transactionName ?? string.Empty, OperationName, traceHeader)
75+
: new TransactionContext(transactionName ?? string.Empty, OperationName);
7776

7877
var customSamplingContext = new Dictionary<string, object?>(3, StringComparer.Ordinal)
7978
{
@@ -112,13 +111,13 @@ public async Task InvokeAsync(HttpContext context)
112111
}
113112

114113
var transaction = TryStartTransaction(context);
114+
var initialName = transaction?.Name;
115115

116116
// Expose the transaction on the scope so that the user
117117
// can retrieve it and start child spans off of it.
118118
hub.ConfigureScope(scope =>
119119
{
120120
scope.Transaction = transaction;
121-
scope.OnEvaluating += (_, _) => scope.Populate(context, _options);
122121
});
123122

124123
Exception? exception = null;
@@ -134,22 +133,36 @@ public async Task InvokeAsync(HttpContext context)
134133
{
135134
if (transaction is not null)
136135
{
137-
// The routing middleware may have ran after ours, so
138-
// try to get the transaction name again.
139-
if (context.TryGetTransactionName() is { } transactionName)
136+
// The Transaction name was altered during the pipeline execution,
137+
// That could be done by user interference or by some Event Capture
138+
// That triggers ScopeExtensions.Populate.
139+
if (transaction.Name != initialName)
140140
{
141-
if (!string.Equals(transaction.Name, transactionName, StringComparison.Ordinal))
142-
{
143-
_options.LogDebug(
144-
"Changed transaction name from '{0}' to '{1}' after request pipeline executed.",
145-
transaction.Name,
146-
transactionName);
147-
}
141+
_options.LogDebug(
142+
"transaction name set from '{0}' to '{1}' during request pipeline execution.",
143+
initialName,
144+
transaction.Name);
145+
}
146+
// try to get the transaction name.
147+
else if (context.TryGetTransactionName() is { } transactionName &&
148+
!string.IsNullOrEmpty(transactionName))
149+
{
150+
_options.LogDebug(
151+
"Changed transaction '{0}', name set to '{1}' after request pipeline executed.",
152+
transaction.SpanId,
153+
transactionName);
148154

149155
transaction.Name = transactionName;
150156
}
151157

152158
var status = SpanStatusConverter.FromHttpStatusCode(context.Response.StatusCode);
159+
160+
// If no Name was found for Transaction, fallback to UnknownRoute name.
161+
if (transaction.Name == string.Empty)
162+
{
163+
transaction.Name = UnknownRouteTransactionName;
164+
}
165+
153166
if (exception is null)
154167
{
155168
transaction.Finish(status);

‎src/Sentry/TransactionTracer.cs‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -222,19 +222,14 @@ public ISpan StartChild(string operation) =>
222222
/// <inheritdoc />
223223
public void Finish()
224224
{
225-
try
226-
{
227-
Status ??= SpanStatus.UnknownError;
228-
EndTimestamp = DateTimeOffset.UtcNow;
225+
Status ??= SpanStatus.UnknownError;
226+
EndTimestamp = DateTimeOffset.UtcNow;
229227

230-
// Client decides whether to discard this transaction based on sampling
231-
_hub.CaptureTransaction(new Transaction(this));
232-
}
233-
finally
234-
{
235-
// Clear the transaction from the scope
236-
_hub.ConfigureScope(scope => scope.ResetTransaction(this));
237-
}
228+
// Clear the transaction from the scope
229+
_hub.ConfigureScope(scope => scope.ResetTransaction(this));
230+
231+
// Client decides whether to discard this transaction based on sampling
232+
_hub.CaptureTransaction(new Transaction(this));
238233
}
239234

240235
/// <inheritdoc />

‎test/Sentry.AspNetCore.Tests/ScopeExtensionsTests.cs‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,62 @@ public ScopeExtensionsTests()
2929
_ = _httpContext.Request.Returns(_httpRequest);
3030
}
3131

32+
private class Fixture
33+
{
34+
public const string ControllerName = "Ctrl";
35+
public const string ActionName = "Actn";
36+
37+
public readonly Scope Scope = new(new SentryOptions());
38+
public HttpContext HttpContext { get; } = Substitute.For<HttpContext>();
39+
40+
public Fixture GetSut(bool addTransaction = true)
41+
{
42+
if (addTransaction)
43+
{
44+
Scope.Transaction = Substitute.For<ITransaction>();
45+
}
46+
47+
var routeFeature = new RoutingFeature
48+
{
49+
RouteData = new RouteData
50+
{
51+
Values =
52+
{
53+
{"controller", ControllerName},
54+
{"action", ActionName}
55+
}
56+
}
57+
};
58+
var features = new FeatureCollection();
59+
features.Set<IRoutingFeature>(routeFeature);
60+
HttpContext.Features.Returns(features);
61+
HttpContext.Request.Method.Returns("GET");
62+
return this;
63+
}
64+
65+
public Fixture GetSutWithEmptyRoute(bool addTransaction = true)
66+
{
67+
if (addTransaction)
68+
{
69+
Scope.Transaction = Substitute.For<ITransaction>();
70+
}
71+
var routeFeature = new RoutingFeature
72+
{
73+
RouteData = new RouteData
74+
{
75+
Values = { { "", null } }
76+
}
77+
};
78+
var features = new FeatureCollection();
79+
features.Set<IRoutingFeature>(routeFeature);
80+
HttpContext.Features.Returns(features);
81+
HttpContext.Request.Method.Returns("GET");
82+
return this;
83+
}
84+
}
85+
86+
private readonly Fixture _fixture = new();
87+
3288
[Fact]
3389
public void Populate_Request_Method_SetToScope()
3490
{
@@ -268,6 +324,7 @@ public void Populate_PayloadExtractors_DoesNotConsiderInvalidResponse(object exp
268324
[Fact]
269325
public void Populate_RouteData_SetToScope()
270326
{
327+
// Arrange
271328
const string controller = "Ctrl";
272329
const string action = "Actn";
273330
var routeFeature = new RoutingFeature()
@@ -279,8 +336,10 @@ public void Populate_RouteData_SetToScope()
279336
_ = _httpContext.Features.Returns(features);
280337
_ = _httpContext.Request.Method.Returns("GET");
281338

339+
// Act
282340
_sut.Populate(_httpContext, SentryAspNetCoreOptions);
283341

342+
// Assert
284343
Assert.Equal($"GET {controller}.{action}", _sut.TransactionName);
285344
}
286345

@@ -364,5 +423,50 @@ public void Populate_TraceIdentifier_WhenRequestIdDoesNotMatch_SetAsTag()
364423

365424
Assert.Equal(expected, _sut.Tags[nameof(HttpContext.TraceIdentifier)]);
366425
}
426+
427+
[Fact]
428+
public void Populate_TransactionAndTransactionNameIsNull_TransactionNameReplaced()
429+
{
430+
// Arrange
431+
var sut = _fixture.GetSut(addTransaction: false);
432+
var scope = sut.Scope;
433+
var expectedTransactionName = $"GET {Fixture.ControllerName}.{Fixture.ActionName}";
434+
435+
// Act
436+
scope.Populate(_fixture.HttpContext, SentryAspNetCoreOptions);
437+
438+
// Assert
439+
Assert.Equal(expectedTransactionName, scope.TransactionName);
440+
}
441+
442+
[Fact]
443+
public void Populate_TransactionIsNullAndRouteNotFound_TransactionNameAsNull()
444+
{
445+
// Arrange
446+
var sut = _fixture.GetSutWithEmptyRoute(addTransaction: false);
447+
var scope = sut.Scope;
448+
449+
// Act
450+
scope.Populate(_fixture.HttpContext, SentryAspNetCoreOptions);
451+
452+
// Assert
453+
Assert.Null(scope.TransactionName);
454+
}
455+
456+
[Fact]
457+
public void Populate_TransactionNameSet_TransactionNameSkipped()
458+
{
459+
// Arrange
460+
var sut = _fixture.GetSut();
461+
var scope = sut.Scope;
462+
var expectedRoute = "MyRoute";
463+
scope.Transaction.Name = expectedRoute;
464+
465+
// Act
466+
scope.Populate(_fixture.HttpContext, SentryAspNetCoreOptions);
467+
468+
// Assert
469+
Assert.Equal(expectedRoute, scope.TransactionName);
470+
}
367471
}
368472
}

0 commit comments

Comments
 (0)