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

Add diagnostics exception handling metrics #48648

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jun 7, 2023

Fixes #48625

Adds a new meter and counter. The counter is used by developer exception middleware and exception handler middleware.

Result of handler:

  • Aborted - Error occurred because the request was aborted (OperationCanceledException + RequestAborted.IsCancellationRequested)
  • Skipped - Response has already started. Middleware can't do anything with the response so skips doing anything.
  • Handled
    • ExceptionHandlerMiddleware has some interesting rules here. An exception is considered handled if an IExceptionHandler returns true, IProblemDetailsService returns true, or the status code is anything other than 404. If an IExceptionHandler or IProblemDetailsService return true then the type name is set for the handler tag.
    • DeveloperExceptionMiddleware never reports exceptions it catches as handled
  • Unhandled - All of the above are false.

Note: The exception-name is still enriched onto the main HTTP request counter. Other detail (result, handler) is only available on the new counter.

@JamesNK
Copy link
Member Author

JamesNK commented Jun 7, 2023

cc @geeknoid @tekian

@@ -36,6 +36,7 @@ public class ExceptionHandlerMiddleware
options,
diagnosticListener,
Enumerable.Empty<IExceptionHandler>(),
new DummyMeterFactory(),
Copy link
Member Author

@JamesNK JamesNK Jun 7, 2023

Choose a reason for hiding this comment

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

I hate public middleware. We end up having to screw around with constructors and DI.

Any objection to creating an issue to make them obsolete and eventually remove them?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it means we only need to worry about API compat on AddX and UseX extension methods that might exist for each middleware.

Copy link
Member

@Tratcher Tratcher Jun 7, 2023

Choose a reason for hiding this comment

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

Yes obsolete them as Error and make them throw, but we've about given up removing APIs due to binary compat.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it means we only need to worry about API compat on AddX and UseX extension methods that might exist for each middleware.

Yes. The important thing is the constructor isn't public. It is easy to modify dependencies.

@JamesNK JamesNK added blocked The work on this issue is blocked due to some dependency area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 7, 2023
@@ -159,13 +156,16 @@ public async Task Invoke(HttpContext context)
WriteDiagnosticEvent(_diagnosticSource, eventName, new { httpContext = context, exception = ex });
}

_metrics.RequestException(exceptionName, ExceptionResult.Unhandled, handler: null);
Copy link
Member

@Tratcher Tratcher Jun 7, 2023

Choose a reason for hiding this comment

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

Handled? I guess not since it was not handled by the app, just default logic.

Copy link
Member Author

@JamesNK JamesNK Jun 9, 2023

Choose a reason for hiding this comment

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

I put Unhandled here because it's literally right below a diagnostic event that reports the exception as unhandled:

const string eventName = "Microsoft.AspNetCore.Diagnostics.UnhandledException";
if (_diagnosticSource.IsEnabled(eventName))
{
WriteDiagnosticEvent(_diagnosticSource, eventName, new { httpContext = context, exception = ex });
}

src/Middleware/Diagnostics/src/DiagnosticsMetrics.cs Outdated Show resolved Hide resolved

namespace Microsoft.AspNetCore.Diagnostics;

internal sealed class DummyMeterFactory : IMeterFactory
Copy link
Member

Choose a reason for hiding this comment

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

Not needed if you make the public middleware constructor throw.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a big deal to add this.

Making the public middleware ctor throw would require breaking change process. I'd prefer to leave it working and go with obsoleting and eventually removing it. Less disruptive.

@JamesNK JamesNK removed the blocked The work on this issue is blocked due to some dependency label Jun 9, 2023
@JamesNK
Copy link
Member Author

JamesNK commented Jun 9, 2023

API review: #48670

@JamesNK JamesNK force-pushed the jamesnk/diagnostics-metrics branch from 5ea33e0 to b615955 Compare June 9, 2023 06:56
@JamesNK JamesNK enabled auto-merge (squash) June 9, 2023 07:16
@JamesNK JamesNK force-pushed the jamesnk/diagnostics-metrics branch from b615955 to c657f73 Compare June 9, 2023 07:53
@JamesNK JamesNK merged commit f4efbe3 into main Jun 9, 2023
@JamesNK JamesNK deleted the jamesnk/diagnostics-metrics branch June 9, 2023 09:22
@ghost ghost added this to the 8.0-preview6 milestone Jun 9, 2023
@JamesNK JamesNK added the blog-candidate Consider mentioning this in the release blog post label Jun 30, 2023
@ghost
Copy link

ghost commented Jun 30, 2023

@JamesNK, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware blog-candidate Consider mentioning this in the release blog post
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More detailed metrics around the ExceptionHandler middleware
3 participants