Skip to content

Conversation

@aritchie
Copy link
Contributor

Resolves #4027

ASP.NET Core's AspNetCoreExceptionProcessor was overwriting the Mechanism and thereby clearing any exception data that may have been recorded.

@jamescrosswell jamescrosswell marked this pull request as draft April 13, 2025 22:05
@aritchie
Copy link
Contributor Author

@jamescrosswell how come this was turned into a draft? The CI failures here don't have anything to do with the PR itself. I've merged in the latest main in case I'm missing something needed for the PR to be finished

@aritchie aritchie marked this pull request as ready for review April 15, 2025 17:47
@jamescrosswell jamescrosswell marked this pull request as draft April 15, 2025 23:04
@jamescrosswell jamescrosswell marked this pull request as ready for review April 16, 2025 21:26
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good... I think we should update the expectations for our unit tests to match the code changes though.

There's this one - could may be tweak it and/or add another test case:

[Collection(nameof(SentrySdkCollection))]
public class AspNetCoreIntegrationTests : SerilogAspNetSentrySdkTestFixture
{
[Fact]
public async Task UnhandledException_MarkedAsUnhandled()
{
var handler = new RequestHandler
{
Path = "/throw",
Handler = _ => throw new Exception("test")
};
Handlers = new[] { handler };
Build();
await HttpClient.GetAsync(handler.Path);
Assert.Contains(Events, e => e.Logger == "Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware");
Assert.Collection(Events, @event => Assert.Collection(@event.SentryExceptions, x => Assert.False(x.Mechanism?.Handled)));
}
}

@aritchie
Copy link
Contributor Author

@jamescrosswell What's your thinking for the tweak though? The code doesn't change the behaviour. It just prevents the previous data from being completely overwritten. I'm not even sure what I would be adjusting here.

@jamescrosswell
Copy link
Collaborator

@jamescrosswell What's your thinking for the tweak though? The code doesn't change the behaviour. It just prevents the previous data from being completely overwritten. I'm not even sure what I would be adjusting here.

At the moment, the delegate for handler in that test just throws an exception. If instead we throw, catch, add some exception.Data and rethrow, then we would expect the exception data to be retained in the SentryException.Mechanism (not overwritten).

@aritchie
Copy link
Contributor Author

@jamescrosswell I just added some basic tests directly around the AspNetCoreExceptionProcessor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exception.Data missing

3 participants