aspnet / Diagnostics

[Archived] Diagnostics middleware for reporting info and handling exceptions and errors in ASP.NET Core, and diagnosing Entity Framework Core migrations errors. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
213 stars 111 forks source link

Populate the exception handler feature #437

Closed martin308 closed 6 years ago

martin308 commented 6 years ago

Populate the IExceptionHandlerFeature when swallowing exceptions in the DeveloperExceptionPageMiddleware, following the same pattern as ExceptionHandlerMiddleware.

This is useful for building error reporting middleware that needs to send exceptions from development.

Outstanding questions:

dnfclas commented 6 years ago

CLA assistant check
All CLA requirements met.

Eilon commented 6 years ago

I don't think this change is correct. The only reason that the Exception Handler middleware sets the feature is that it then goes and runs the middleware pipeline again so it needs to make the error details available to them.

The developer exception page, on the other hand, fully handles the error, so nothing else should see the error.

@Tratcher - did I get that right?

Tratcher commented 6 years ago

@Eilon's right. Only middleware in front of this one in the pipeline would be able to see this feature, and not until the exception page had already been written.

martin308 commented 6 years ago

Is that not the same case with ExceptionHandlerMiddleware?

@Tratcher this is exactly the scenario that I want to cover. I am writing error reporting middleware which runs first in the stack using an IStartupFilter to notify of any exceptions that occur in the stack. I can check the IExceptionHandlerFeature feature and still report the exceptions when ExceptionHandlerMiddleware is being used but not DeveloperExceptionPageMiddleware

Tratcher commented 6 years ago

ExceptionHandlerMiddleware calls into a full request pipeline to render the error page and uses the feature to pass the exception up the stack, not down the stack to components before it.

Have you considered using DiagnosticSource or Logging to discover exceptions from the application? All of the exception handlers should write to these.

The other consideration is if your component is intended for use during development or production. ExceptionHandlerMiddleware is designed for production, where DeveloperExceptionPageMiddleware is strictly for development scenarios.

martin308 commented 6 years ago

Is having the outermost middleware detect features from inner middleware break the design pattern you have here?

I had considered DiagnosticSource and Logging but at that point you have lost all of the http context information that makes error reporting in web applications quite useful. Unless I am missing something here?

I want the component to support as many user configurations as possible while making it as easy as possible to see exceptions out of the box (falling into the pit of success).

I want users to be able to add the code below and have things work without having to explain why having certain middleware in certain orders will mean that exceptions will not be detected.

public void ConfigureServices(IServiceCollection services)
{
  services.AddBugsnag(options => {
    options.ApiKey = "123456";
  });
}
martin308 commented 6 years ago

Ah I do see the diagnostics source populates the http context! I will dig more into that if you think this will break the pattern that you have established

Tratcher commented 6 years ago

The diagnostics source will give you more reliable results than this change, and it's already available in the released packages. I'm going to close this PR for now. Let us know if you want to revisit this.