serilog-aspnetcore icon indicating copy to clipboard operation
serilog-aspnetcore copied to clipboard

Add EnrichDiagnosticContextAsync to allow async calls

Open hbunjes opened this issue 2 years ago • 5 comments

https://github.com/serilog/serilog-aspnetcore/issues/345

If you want to add properties to the diagnostic context that require async calls (e.g. reading request body), EnrichDiagnosticContextAsync allows to hand over your own async method.

hbunjes avatar Sep 20 '23 07:09 hbunjes

Thanks for sending this!

I think this feature would be find if it cleanly slotted in, but as you found in the implementation, it's not completely equivalent to the existing EnrichDiagnosticContext option 🤔 - not sure how much that matters, but does suggest people might observe some unexpected differences in collected data when switching between the synchronous and asynchronous overloads.

The EnrichDiagnosticContext option we have right now also avoids adding overhead when the request completion event's level is not enabled, which is harder to do for the async version.

Just zooming out, the alternative implementation of this scenario might look like:

class RequestBodyEnrichmentMiddleware(RequestDelegate next, IDiagnosticContext diagnosticContext)
{
    public async Task InvokeAsync(HttpContext context)
    {
        await EnrichFromRequestAsync(context.Request);
        await next(context);
    }

    async Task EnrichFromRequestAsync(HttpRequest request)
    {
        // diagnosticContext.Set(...)
    }
}

Perhaps giving that example in the README would go far enough?

nblumhardt avatar Sep 21 '23 00:09 nblumhardt

Nicholas, thank you for your valuable feedback. I totally get your point.

The reason I think this is still a good place to integrate is that you already have some good features like "GetLevel" in here and it's just a good match to have the logging information at one point.

I've changed the implementation to behave like the sync call. It's always called in finally block (and makes sure it doesn't create new Exceptions, just like the second call of "LogCompletion" in the exception filter clause). It's also taking the information if the method should be called based on GetLevel and current log level.

I'll check why two tests fail (it seems I've introduced a bug between my last tests and commiting the solution) and update the PR then.

The bug is fixed. In case an exception occured in one of the next middlewares or the request the logger was not initialized correctly.

hbunjes avatar Sep 27 '23 20:09 hbunjes

I would prefer to moce from public Action<IDiagnosticContext, HttpContext>? EnrichDiagnosticContext { get; set; } to public Func<IDiagnosticContext, HttpContext, Task>? EnrichDiagnosticContext { get; set; }

sungam3r avatar Mar 19 '24 12:03 sungam3r