serilog-web / classic

[Discontinued] Serilog web request logging and enrichment for classic ASP.NET applications
Apache License 2.0
79 stars 32 forks source link

Allow to pass an exception to the "logging module" through other means than HttpContext.AddError #48

Closed tsimbalar closed 5 years ago

tsimbalar commented 6 years ago

The current means for the module to log the errors is to take a look at Server.GetLastError() (this is actually how the SerilogWeb.Classic.WebAPI pushes unhandled exceptions to the logging module).

While it covers most scenarios, it is kind of problematic when using IIS Custom Error Pages and ASP.NET MVC together.

In MVC, the standard one way to handle errors is by using HandleErrorAttribute (or a subclass) that states which view should be shown depending on the thrown exception.

While this sets the proper HttpCode for the response (the generated log event has the proper status code), the Exception is actually lost and does not appear in the log. (see #29 where this exception is logged only when CustomErrors="Off" ).

When I write a custom IExceptionFilter / FilterAttribute , I can add the exception through a call to filterContext.HttpContext.AddError(myException). When I do that, the Exception properly appears in the logs, but the presence of this Exception in the list of errors seems to trigger the IIS Custom Errors mechanism, and no matter what, instead of showing a custom view (passed to the attribute), the IIS-custom error page that is configured for Status Code 500 is shown ...

It seems I can either choose between either using AuthorizeAttribute or having exceptions in my logs ... which is not ideal ...

Maybe we should need a way to report an exception to the Logging Module without going through HttpContext.AddError() ...

I am thinking that it could be something like :

This would allow to add write a custom AuthorizeAttribute that would store exceptions specifically for the logging module.

Optionnally SerilogWeb.Classic.WebApi could also use that channel to report to the logging module instead of using Context.AddError().

Does that make sense ?

tsimbalar commented 6 years ago

I think this would also be helpful to report errors in "stacks" on top of ASP.NET like :

without interfering too much into the hard-coupling IIS / ASP.NET

laurencee commented 6 years ago

I implemented something locally that seems ok which basically just follows what was already done for the stopwatch in WebRequestLoggingHandler. I think it just needs a nicer public api/wrapping with some update to the readme documentation to support the approach.

In my custom ExceptionFilterAttribute for OnException and OnExceptionAsync I added:

HttpContext.Current?.Items.Add(SerilogWebClassic.ErrorKey, context.Exception);

1 line modification to SerilogWeb.Classic.WebRequestLoggingHandler

var error = _application.Server.GetLastError() ?? _application.Context.Items[SerilogWebClassic.ErrorKey] as Exception;

I guess you could switch the order on the above if you want custom exception tracking to have a higher priority.

For the public library version, an extension method could be provided to avoid needing to know about the key details

public static void AddSerilogException(this HttpContext context, Exception ex)
{
    context.Items.Add(SerilogWebClassic.ErrorKey, ex);
}
tsimbalar commented 6 years ago

Hi @laurencee ,

so sorry for taking so long to react ...

Yes that looks exactly like what I had in mind !

In terms of priority, I think the custom exception should indeed have higher priority (later on, this could be customizable, but that's probably not necessary from the start)

Do you think you could find the time to create a Pull Request for this one ?

laurencee commented 6 years ago

@tsimbalar Hey sorry about taking a while to get back to you, we had actually moved over to net core so I forgot about looking at this.

I'll see if I can get a PR up for this next week or week after, quite a busy time leading up to holidays :)