Add EnrichDiagnosticContextAsync to allow async calls

[hbunjes]

#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]

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]

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.

[nblumhardt]

I may be off track, but won't this be called after the LogCompletion calls in the try and catch/when blocks?

[sungam3r]

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

[Gonkers]

Is there any movement expected on this?