Add exception to IDiagnosticContext

[angularsen]

Fixes: https://github.com/serilog/serilog-aspnetcore/issues/270 Related PR: https://github.com/serilog/serilog-aspnetcore/pull/271

For applications using middleware like Hellang.Middleware.ProblemDetails, where unhandled exceptions are swallowed, the application can pass on the exception to Serilog.AspNetCore's RequestLoggingMiddleware via IDiagnosticContext.

Changes

• Add IDiagnosticContext.SetException(Exception e)
• Add new overload DiagnosticContextCollector.TryComplete(out IEnumerable<LogEventProperty>, out Exception) to not make a breaking change.
• Add tests on setting and collecting the exception.

[nblumhardt]

Sorry about the long turnaround on this. Just to make sure I understand the scenario - could you alternatively place the ProblemDetails middleware in the pipeline before the Serilog request logging middleware? Thanks!

[angularsen]

@nblumhardt Yes, but then Serilog would not log the actual response, since ProblemDetails transforms exceptions to HTTP responses.

[nblumhardt]

Thanks; checked out the middleware, thought this over some more - I'm on board with it 👍

I'll need to go through the PR again and think some more about back/forwards compatibility, seems like it's on the right track.

[nblumhardt]

One thing you will miss out on via this method that I should mention: the Serilog request logging middleware uses an exception filter so that it can log exceptions with LogContext data from the throw site preserved. Catching the exceptions in the ProblemDetails middleware will mean that the exception filter won't apply, and so the log events will be missing this extra context information. Probably fine :-)

[angularsen]

Aha, I see. Would it be possible to pass the same data via the SetException() method? Is ambient log context available to read from maybe? It seems like a useful addition.

[nblumhardt]

It's possible to clone, preserve, and (temporarily) reinstate the ambient LogContext, but the mechanics would be complicated enough that I think we're better off ignoring it at this point.

[angularsen]

No rush, but if there is anything I can do to help move this forward, let me know. I'm happy to help.

[nblumhardt]

I think the main lingering questions are:

• [ ] Will the interface change cause too much breakage out in the ecosystem? It should probably not end up breaking Serilog.AspNetCore, since it uses implementations that are in this package, too, but other usage scenarios may not be so lucky
• [ ] Since the stored exception is last in wins, should any exception caught in the request handling middleware take precedence over the exception stored in the context?

We can discuss these in a bit more detail on the corresponding Serilog.AspNetCore PR; I think that apart from the minor doc comments, this one is ready to go 👍

[angularsen]

@nblumhardt Thanks, I've updated the xmldoc and added an obsolete attribute to TryComplete.

To address your questions:

1. The only issue with breakage I recognize is if there are nugets or packaged binaries that offer their own implementations of IDiagnosticContext. Those would have to implement the new SetException to avoid a runtime exception calling that method, but I think this is a common scenario with nuget upgrades. If you upgrade Serilog.Extensions.Hosting package, you generally should also upgrade any other packages that depend on it to avoid problems like this.
2. We can discuss last in wins more in the next PR, I need to better understand what scenarios would be likely.

[nblumhardt]

Brilliant, thanks @angularsen 👍

[nblumhardt]

Just realised that we should bump the package major version to reflect the breaking interface change; I'll do that directly now on dev, and also update the versioning of the downstream package.

[lonix1]

@angularsen I also use Hellang.Middleware.ProblemDetails middleware.

What should I do to integrate this PR's new functionality with my existing code?

[angularsen]

@lonix1 Good question, we use a global MVC exception filter to obtain the exception before it is passed back out the middleware chain.

A few other ways to do it:

• Add a middleware just before/after the MVC middleware to catch and rethrow the exception.
• Use a custom base class for all API controllers that implements IActionFilter, IAsyncActionFilter

MVC exception filter

To access exceptions thrown by API/MVC actions and set in IDiagnosticContext.

using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.Extensions.DependencyInjection;
using Serilog;

namespace Foo;

public class SerilogExceptionEnricherMvcFilter : IExceptionFilter
{
    public void OnException(ExceptionContext context)
    {
        // Enrich the request log event with exception info that is swallowed by Hellang.ProblemDetails middleware in order to produce
        // a ProblemDetails HTTP response before Serilog's RequestLoggingMiddleware has the chance to observe it.
        var dc = context.HttpContext.RequestServices.GetRequiredService<IDiagnosticContext>();
        dc.SetException(context.Exception);
    }
}

Add as global MVC filter in Startup.cs

Wherever you configure MVC in your IServiceCollection, insert the new filter.

public sealed class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {
        services
            .AddMvc(opt =>
            {
                // Global MVC filters.
                opt.Filters.Add<SerilogEnricherMvcFilter>();
            });
    }
}