HttpLoggingMiddleware could allow custom code to decide whether to log or not

[hugoqribeiro]

Background and Motivation

The HttpLoggingMiddleware added recently works based on configuration options in HttpLoggingOptions.

The currently implementation is basically all or nothing. You either add the middleware or not. Even if you can decide what is redacted or not and if request/response bodies are included.

It would be nice if HttpLoggingOptions included a delegate to let the application (custom code) decide whether to log or not for a specific request.

Proposed API

namespace Microsoft.AspNetCore.HttpLogging
{
    public sealed class HttpLoggingOptions
    {
+       public Func<HttpContext, bool> Selector { get; set; } = context => true;
    }
}

Usage Examples

• Log only failed requests.
• Log request body only for specific endpoints.

Alternative Designs

You can implement your own middleware but that beats the whole purpose of implementing this feature in the framework.
An alternative would be to make HttpLoggingMiddleware public and add some kind of extensibility to it. At least one could implement that custom middleware but reuse the framework implementation (e.g. request buffering).

Risks

The risk is that performance of HTTP logging would depend on the custom code implemented in the delegate.

[pranavkm]

Couldn't you wrap UseHttpLogging in a app.UseWhen to achieve this?

[hugoqribeiro]

UseWhen would problably work better for the "log only for specific endpoints".
It would not help for the "log only failed requests" right?

[BrennanConroy]

Similar to #31844

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[Tratcher]

Rather than on/off, you'd want something more granular like HttpLoggingFields (and return None for off).

namespace Microsoft.AspNetCore.HttpLogging
{
    public sealed class HttpLoggingOptions
    {
+       public Func<HttpContext, HttpLoggingFields>? FieldSelector { get; set; } 
    }
}

This is similar to the proposed endpoint attribute in #43222.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[Ole-Visma]

As mentioned in the description, it should be possible to log or not log a specific request. Not by route or smth static. Two requests to the same endpoint may be handled differently depending on a specific criteria, like the status code.

[halter73]

API Review Notes:

• Would the FieldSelector be called at the beginning of the request? Yes.
  • Wouldn't this be too early to "log only failed requests"? Yes.
  • Are we okay with this? Sounds like yes. We could possibly add an option to delay logs and also the call to the FieldSelector until after the response is produced at a later date.
• Should we register a service rather than add a settable Func to options? This would make it a little easier to resolve services and maintain. It's possible to resolve services using the options pattern, so this is a matter of style.
• What does the example usage look like?

builder.Services.AddHttpLogging(options =>
{
    //options.LoggingFields = FieldSelector.RequestPropertiesAndHeaders;

    options.FieldSelector = context =>
    {
          if (<some condition that means the request should not be logged>)
          {
               return HttpLoggingFields.None;
          }

          return null;
    };
});

• How do we combine it with HttpLoggingOptions.LoggingFields? Does it override it always? Should we make the return type nullable if we want the default behavior? Let's do that.
• Do we want to support multiple FieldSelectors? We think manually chaining the funcs is sufficient for this.

API Approved with nullable return type!

namespace Microsoft.AspNetCore.HttpLogging
{
    public sealed class HttpLoggingOptions
    {
+       public Func<HttpContext, HttpLoggingFields?>? FieldSelector { get; set; } 
    }
}

[BrennanConroy]

I was thinking about whether the Func should include the current field options, it would be easy to do that with WAB:

builder.Services.AddHttpLogging(options =>
{
    var defaultOptions = HttpLoggingFields.RequestPropertiesAndHeaders;
    options.LoggingFields = defaultOptions;

    options.FieldSelector = context =>
    {
          if (<some condition to remove request headers>)
          {
               return defaultOptions & ~HttpLoggingFields.RequestHeaders;
          }

          return null;
    };
});

But, not everyone uses WAB, and we are using IOptionsMonitor<HttpLoggingOptions> which means the default could change at any time. It seems like we should add the current options then:

+       public Func<HttpContext, HttpLoggingFields, HttpLoggingFields?>? FieldSelector { get; set; } 

Also, thinking about

We could possibly add an option to delay logs and also the call to the FieldSelector until after the response is produced at a later date.

It seems like we would need a way of expressing this in the FieldSelector call, so users would know that this was called after the response or not.
e.g.

+       public Func<HttpContext, bool, HttpLoggingFields, HttpLoggingFields?>? FieldSelector { get; set; }

And optionally adding a delegate so that the parameters can be named.

+       public FieldSelectorDelegate? FieldSelector { get; set; }
+       public delegate HttpLoggingFields? FieldSelectorDelegate(HttpContext context, bool afterResponse, HttpLoggingFields currentFields);

Or, adding a context object like we do in other middleware, in case we want to add more settings in the future, such as changing the response/request body log limits.

[Tratcher]

If you're going to include the current HttpLoggingFields as an input, then the output doesn't need to be nullable, you can return the original input.

+       public Func<HttpContext, HttpLoggingFields, HttpLoggingFields>? FieldSelector { get; set; } 

[Tratcher]

It seems like we would need a way of expressing this in the FieldSelector call, so users would know that this was called after the response or not.

You'd also need a separate option to enable the delayed call. Or even a two stage flow where you call it once, it says what to preserve, and then you call it again with the results and it decides if the logs should happen. This seems more complicated than we want to pursue right now.

And by the time you have this many parameters you should be passing a context object.

[BrennanConroy]

You'd also need a separate option to enable the delayed call.

Yes, I was assuming that since it was already called out by Stephen.

And by the time you have this many parameters you should be passing a context object.

Yes, that's what I said at the end of my comment.

This seems more complicated than we want to pursue right now.

I'm not saying we should add the delayed call right now. But we should design for potential future changes.

[BrennanConroy]

API Review Notes:

• We would like feature parity with the attributes in Make Http Logging Middleware Endpoint aware #43222, meaning request/response size limits should be settable
• Adding a context object makes this more future proof
• Media type options would be a pain to pass in and allow setting per endpoint
• Similar with request/response headers, would need to copy every time
• Just keep it simple with Fields and body size limits for now, it's a context so we can add things easily in the future!
• Calling into question why we even need this if you can fairly easily write a middleware to do the same thing (now that the attribute apis are approved)
  • We aren't doing anything fancy on the users' behalf that they couldn't do themselves
  • caveat, if this was called after response, it would be more useful (hard to do in middleware) since we would be buffering request/response and not logging until you made a decision

API needs feedback still

namespace Microsoft.AspNetCore.HttpLogging;

+public sealed class HttpLoggingContext
+{
+    public HttpLoggingContext();

+    public HttpContext HttpContext { get; set; }

+    public HttpLoggingFields LoggingFields { get; set; }

+    public int? RequestBodyLogLimit { get; }

+    public int? ResponseBodyLogLimit { get; }
+}

namespace Microsoft.AspNetCore.HttpLogging;

public sealed class HttpLoggingOptions
{
+    public Action<HttpLoggingContext>? HttpLoggingSelector { get; set; }
}

[adityamandaleeka]

Triage: it would be interesting to think about how you could do this in ILogger instead of here.

cc @JamesNK who is working on other logging things that may be related.

[Tratcher]

ILogger seems too late. Collecting these fields from the HttpContext and creating the log message, especially with the request body, is expensive.

[BrennanConroy]

I think this is replaced by #31844 (comment)
Which allows the user more granular control over what gets logged.

[adityamandaleeka]

Closing as duplicate ^