Skip to content
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

Stop tracking handled exceptions, or add an opt out #2645

Closed
stevendarby opened this issue Jul 28, 2022 · 6 comments
Closed

Stop tracking handled exceptions, or add an opt out #2645

stevendarby opened this issue Jul 28, 2022 · 6 comments

Comments

@stevendarby
Copy link

Is your feature request related to a problem?
I use the exception handling middleware in ASP.NET Core - UseExceptionHandler()

When I handle an exception in the ExceptionHandler delegate, the exception is tracked twice in application insights. This isn't good. A lot of the exceptions handled here are custom exceptions thrown precisely so they can be handled here and transformed into a response. I don't want these handled exceptions to be tracked at all and certainly not twice.

The first tracking is because it is logged with ILogger in the middleware. See this issue I raised about that (dotnet/aspnetcore#42950 ) because I didn't think it should be logging handled exceptions as Error with a message that they're unhandled. Based on the response I will have to disable this by setting logging to None for that namespace.

The second tracking I believe is due to the fact the exception middleware then writes a diagnostic with an event name of [...].HandledException

https://github.com/dotnet/aspnetcore/blob/9649fc7c4db4846ce47b8450516191595b640ae5/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs#L162-L166

App Insights appears to track these exceptions when it receives this diagnostic event.

else if (this.trackExceptions && value.Key == "Microsoft.AspNetCore.Diagnostics.HandledException")
{
httpContext = this.httpContextFetcherDiagExceptionHandled.Fetch(value.Value) as HttpContext;
exception = this.exceptionFetcherDiagExceptionHandled.Fetch(value.Value) as Exception;
if (httpContext != null && exception != null)
{
this.OnDiagnosticsHandledException(httpContext, exception);
}
}

Describe the solution you'd like.
I would like handled exceptions not to be tracked. I don't think an exception handled in a regular try/catch would be tracked by app insights, and I think this is just an extension of that same principle.

If you think the current behaviour is correct, I would like to understand why you think so. And as an alternative, could there be a separate option in addition to TrackExceptions, such as TrackHandledExceptions, so I can disable it without disabling tracking of unhandled exceptions?

Describe alternatives you've considered.
Setting TrackExceptions to false, but this would disable unhandled exception tracking too.

Additional context.
Add any other context about the feature request here.

@cijothomas
Copy link
Contributor

cijothomas commented Jul 29, 2022

else if (this.trackExceptions && value.Key == "Microsoft.AspNetCore.Diagnostics.HandledException")

^ This could be a bug, and that whole block can be removed?

@cijothomas
Copy link
Contributor

@vishweshbankwar FYI, as you were looking for this/similar for OpenTelemetry as well. (I think OpenTelemetry would also have same issue, but need to test)

@cijothomas
Copy link
Contributor

I would like handled exceptions not to be tracked. I don't think an exception handled in a regular try/catch would be tracked by app insights, and I think this is just an extension of that same principle.

💯 agree! Let us check if there was any specific reason this was enabled. From a quick glance, it looks like we should remove it..

@stevendarby
Copy link
Author

Hi, found this related issue:
microsoft/ApplicationInsights-aspnetcore#861

I may be reading this wrong, but the comments on the issue & the closing PR suggest that if you’re using the app insights logging provider, TrackExceptions can/should be set to false to avoid duplicate tracking of exceptions.

So maybe I can just do this, and it doesn’t matter that it will also no longer track unhandled exceptions, because the ASP.NET Core framework will do that by logging an error via ILogger somewhere? Will try to investigate this point.

Even if so, this issue might still be valid for people who don’t use the logging provider - and so want TrackExceptions on - but don’t want to track handled exceptions.

@cijothomas
Copy link
Contributor

TrackExceptions is false by default now (ever since ILogger intergation was automatically enabled). So if you still see duplicate, need to investigate a bit more.

@github-actions
Copy link

This issue is stale because it has been open 300 days with no activity. Remove stale label or this will be closed in 7 days. Commenting will instruct the bot to automatically remove the label.

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

No branches or pull requests

2 participants