fix: Sentry Tracing middleware crashes ASP.NET Core in .NET 10 in version 6.0.0-rc1 and earlier #4747
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4747 +/- ##
==========================================
+ Coverage 73.69% 73.70% +0.01%
==========================================
Files 476 476
Lines 17443 17441 -2
Branches 3453 3452 -1
==========================================
+ Hits 12854 12855 +1
+ Misses 3741 3738 -3
Partials 848 848 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jamescrosswell
changed the title
jamescrosswell
changed the title
jamescrosswell
changed the title
|
@sentry review |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| { | ||
| var sentryClient = Substitute.For<ISentryClient>(); | ||
| var options = new SentryAspNetCoreOptions | ||
| { | ||
| Dsn = ValidDsn, | ||
| TracesSampleRate = 1, | ||
| AutoRegisterTracing = false | ||
| }; | ||
|
|
||
| var hub = new Hub(options, sentryClient); | ||
|
|
||
| var server = new TestServer(new WebHostBuilder() | ||
| .UseSentry() | ||
| .ConfigureServices(services => | ||
| { | ||
| services.RemoveAll(typeof(Func<IHub>)); | ||
| services.AddSingleton<Func<IHub>>(() => hub); | ||
| services.AddRouting(); | ||
| }).Configure(app => | ||
| { | ||
| app.UseRouting(); | ||
| app.UseSentryTracing(); | ||
| app.UseEndpoints(routes => | ||
| { | ||
| routes.Map("/", _ => Task.CompletedTask); | ||
| routes.Map("/crash", async _ => | ||
| { | ||
| await Task.Yield(); | ||
| throw new Exception(); | ||
| }); | ||
| }); | ||
| })); | ||
|
|
||
| var client = server.CreateClient(); | ||
|
|
||
| // Act | ||
| try | ||
| { | ||
| await client.GetStringAsync("/crash"); | ||
| } | ||
| // Expected error. | ||
| catch | ||
| { | ||
| // ignored | ||
| } | ||
|
|
||
| // Assert | ||
| // Make sure Kestrel is still alive by making another request | ||
| var response = await client.GetAsync("/"); | ||
| response.StatusCode.Should().Be(System.Net.HttpStatusCode.OK); | ||
| } |
There was a problem hiding this comment.
This test validates that the server remains responsive after an exception, which is good. However, it lacks assertions to verify that the exception was properly recorded in Sentry and bound to the transaction. Consider adding assertions similar to the existing Transaction_binds_exception_thrown test to ensure that hub.ExceptionToSpanMap contains the exception and that the transaction was finished with an error status. This would provide more comprehensive validation of the exception handling flow.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: test/Sentry.AspNetCore.Tests/SentryTracingMiddlewareTests.cs#L619-L669
Potential issue: This test validates that the server remains responsive after an
exception, which is good. However, it lacks assertions to verify that the exception was
properly recorded in Sentry and bound to the transaction. Consider adding assertions
similar to the existing `Transaction_binds_exception_thrown` test to ensure that
`hub.ExceptionToSpanMap` contains the exception and that the transaction was finished
with an error status. This would provide more comprehensive validation of the exception
handling flow.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2766746
There was a problem hiding this comment.
True.
When removing the re-throw entirely
sentry-dotnet/src/Sentry.AspNetCore/SentryTracingMiddleware.cs
Lines 143 to 145 in bd7b3d0
no test in
SentryTracingMiddlewareTests fails.
However, other tests are failing
Sentry.AspNetCore.Tests.IntegrationsTests.CaptureException_UseExceptionHandler_SetRouteDataFromInitialRequestSentry.Google.Cloud.Functions.Tests.IntegrationTests.SentryIntegrationTest_CaptureUnhandledException
Is it worth adding an assertion to SentryTracingMiddlewareTests, too?
There was a problem hiding this comment.
It's not really the point of this fix. We could address it separately if we think we're lacking test coverage.
|
@Flash0ver Do you happen to know when this fix will be included in a release (or release candidate)? We’re currently blocked on our .NET 10 upgrade because of this crash, so any timing info would really help. |
This was included in RC2, which just went out today: |
Fixes #4735: