HttpLogging redaction and enrichment

[Tratcher]

This adds extensibility to the logging middleware so that what gets logged can be granularly customized per request.

Description

See the API review notes at #31844 (comment)

Fixes #31844, contributes to dotnet/extensions#4330
Fixes #49989, CopyTo/Async not logging the request body.
Fixes #49063, Clarify when the request body is not fully logged.

IHttpLoggingInterceptor:

• This is a new interface that can have multiple instances registered and injected into the logging middleware. The callbacks run before the normal request/response logging and enable adjusting what should be logged per request, as well as adding custom fields.
• ValueTask OnRequestAsync(HttpLoggingInterceptorContext logContext);
• ValueTask OnResponseAsync(HttpLoggingInterceptorContext logContext);

Duration:

• This is a new field that can be logged, reported as milliseconds since the middleware first saw the request. This is logged once at the end of the response.

info: Microsoft.AspNetCore.HttpLogging.HttpLoggingMiddleware[2]
      Response:
      Content-Type: RedactedHeader
      ResponseEnrichment: Stuff
      StatusCode: 200
info: Microsoft.AspNetCore.HttpLogging.HttpLoggingMiddleware[8]
      Duration: 0.3027ms

Combine Logs:
bool HttpLoggingOptions.CombineLogs { get; set; } has been added to support writing one unified log of request, request body, response, and response body data at the end of the response. The extensions components require this.

Exception handling:

• The middleware has been updated to ensure that all data collected so far is logged even in the event of an exception thrown from the middleware pipeline. This is especially important for the new CombineLogs option since the request data won't have been logged before calling next. Note that since this middleware does not handle the exception, it does not log it so as to avoid duplicating it in the logs.

[Tratcher]

Checking for changes to API baseline files release/8.0
Found changes in 1 API baseline files
##[error]Detected modification to baseline API files. PublicAPI.Shipped.txt files should only be updated after a major release. See /docs/APIBaselines.md for more information.
error : Detected modification to baseline API files. PublicAPI.Shipped.txt files should only be updated after a major release. See /docs/APIBaselines.md for more information.
##[error]Modified API baseline files:
error : Modified API baseline files:
##[error]src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt
error : src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt

https://dev.azure.com/dnceng-public/public/_build/results?buildId=384096&view=logs&j=efda9e5f-cf98-536e-4181-9b6c13ac35b3&t=dca754b7-8b76-5f52-60ef-6b0e8f7eb325&l=801

@wtgodbe any idea why it's complaining about modifying the Unshipped file? Is the release branch more strict about that?

edit Figured this out, the error message is just incomplete. It doesn't want us modifying even the Unshipped file in release branches since we shouldn't be adding APIs in servicing. However that doesn't account for rc2 work which is going into release/8.0 until rc2 branches. I'll clarify the error in the script, but it won't fix it, we'll need to override it.

[JamesNK]

Why is the response body captured if there is an interceptor? Is there a situation where this wouldn't be wanted?

Add comment?

[Tratcher]

The response body isn't captured until after interceptors run, but we need to hook it in advance so that an interceptor can decide to capture the body or not.

[JamesNK]

That seems like a limitation of interceptors. Someone might want to redact fields with an interceptor, but an interceptor forces the body to be captured which can have a big performance overhead.

Have you considered a property on the interceptor interface to control whether the body is captured? bool CanCaptureResponseBody or bool SuppressCaptureResponseBody

[Tratcher]

We're not always capturing the response body, we're only shimming it so we can see when it starts and make the decision to capture it. This should not cause significant overhead.

[Tratcher]

I've updated the description to include two recent requirements changes based on feedback from the Extensions folks, CombineLogs and exception handling.

[Tratcher]

@wtgodbe ready to merge.

[ghost]

@Tratcher, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!