-
-
Notifications
You must be signed in to change notification settings - Fork 229
fix: Sentry Tracing middleware crashes ASP.NET Core in .NET 10 in version 6.0.0-rc1 and earlier #4747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Sentry Tracing middleware crashes ASP.NET Core in .NET 10 in version 6.0.0-rc1 and earlier #4747
Changes from 4 commits
aa5e656
98ab32e
afcce19
bd7b3d0
9e6e066
2fe74d5
1f1ecb1
f27fb1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -614,6 +614,60 @@ public async Task Transaction_binds_exception_thrown() | |||||||
| Assert.Equal(SpanStatus.InternalError, span?.Status); | ||||||||
| } | ||||||||
|
|
||||||||
| [Fact] | ||||||||
| public async Task ExceptionThrownAsync_DoesNotCrashKestrel() | ||||||||
| { | ||||||||
| 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(); | ||||||||
jamescrosswell marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| 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); | ||||||||
| } | ||||||||
|
Comment on lines
619
to
668
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 🤖 Prompt for AI Agent Did we get this right? 👍 / 👎 to inform future reviews. Reference_id: 2766746
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. sentry-dotnet/src/Sentry.AspNetCore/SentryTracingMiddleware.cs Lines 143 to 145 in bd7b3d0
no test in SentryTracingMiddlewareTests fails.
However, other tests are failing
Is it worth adding an assertion to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not really the point of this fix. We could address it separately if we think we're lacking test coverage. |
||||||||
|
|
||||||||
| [Fact] | ||||||||
| public async Task Transaction_TransactionNameProviderSetSet_TransactionNameSet() | ||||||||
| { | ||||||||
|
|
||||||||
Uh oh!
There was an error while loading. Please reload this page.