serilog / serilog-aspnetcore

Serilog integration for ASP.NET Core
Apache License 2.0
1.31k stars 205 forks source link

Add EnrichDiagnosticContextAsync to allow async calls #346

Open hbunjes opened 1 year ago

hbunjes commented 1 year ago

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.

nblumhardt commented 1 year ago

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?

hbunjes commented 1 year ago

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.

sungam3r commented 6 months ago

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