Make Http Logging Middleware Endpoint aware

[Kahbazi]

Background and Motivation

I need to log the body of every requests that are coming to the application, but I want to exclude some endpoints. For example I don't want to log the body of the login methods. I suggest adding metadata for enabling and disabling log for http logging middleware.

The new behavior would be like these: if endpoint has DisableLoggingAttribute the middleware would be skipped completely. when EnableLoggingAttribute is available the middleware use its LoggingFields to log the request/response and if there are no metadata available, it would use HttpLoggingOptions.LoggingFields.

Also should other HttpLoggingOptions properties be available in the metadata or is logging fields enough?

Proposed API

namespace Microsoft.AspNetCore.HttpLogging;

+ public class EnableLoggingAttribute : Attribute
+{
+    public EnableLoggingAttribute(HttpLoggingFields fields) { }
+    public HttpLoggingFields LoggingFields { get; }
+}

+ public class DisableLoggingAttribute : Attribute
+{
+}

namespace Microsoft.AspNetCore.Builder;

+public static class HttpLoggingEndpointConventionBuilderExtensions
+{
+    public static TBuilder EnableLogging<TBuilder>(this TBuilder builder, HttpLoggingFields loggingFields) where TBuilder : IEndpointConventionBuilder;
+    public static TBuilder DisableLogging<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
+}

Usage Examples

app.MapPost("/login", ... ).DisableLogging();
app.MapPost("/uploadFile", ... ).EnableLogging(HttpLoggingFields.RequestPropertiesAndHeaders);

Alternative Designs

Risks

[Tratcher]

Would there be a functional difference between EnableLoggingAttribute(None) and DisableLoggingAttribute?

[Kahbazi]

Would there be a functional difference between EnableLoggingAttribute(None) and DisableLoggingAttribute?

Hmm, No!

[wtgodbe]

Triage: We'll consider this for 8, endpoint-awareness in general is goodness for Middlewares.

[murad88]

For debugging, I need to temporarily enable logging for only one endpoint.
There is no such possibility in NET 6 :(

[tahq69]

Something like rote pattern would be nice to have.
For example, if I build API and has swagger on top of that, I definitely don't want to log swager endpoints. So it would be great if I could disable them like

builder.Services.AddHttpLogging(options =>
{
    options.LoggingFields = HttpLoggingFields.All;
    options.Disable.Add("/swagger*");
});

[Tratcher]

Something like rote pattern would be nice to have. For example, if I build API and has swagger on top of that, I definitely don't want to log swager endpoints. So it would be great if I could disable them like

builder.Services.AddHttpLogging(options =>
{
    options.LoggingFields = HttpLoggingFields.All;
    options.Disable.Add("/swagger*");
});

The point of using endpoints would be to add the enable/disable metadata directly to them and avoid re-implementing routing locally.

[stefer]

It is possible to use UseWhen to selectively add http logging to only some endpoints, like so:

            app.UseWhen(
                context => context.Request.Path.StartsWithSegments("/api"),
                builder => builder.UseHttpLogging());

[Tratcher]

It is possible to use UseWhen to selectively add http logging to only some endpoints, like so:

            app.UseWhen(
                context => context.Request.Path.StartsWithSegments("/api"),
                builder => builder.UseHttpLogging());

Yes

[Tratcher]

Also should other HttpLoggingOptions properties be available in the metadata or is logging fields enough?

RequestBodyLogLimit and ResponseBodyLogLimit would be easy options to include. Request/ResponseHeaders and MediaTypeOptions wouldn't be feasible to expose directly on an attribute. If we wanted to do those we'd need to shift to a named policy model. Those settings are far less endpoint specific so we might not need them.

Compare to #45732

[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.

[halter73]

API Review Notes:

• If you add [EnableLoggingAttribute] to the method, what happens? Is everything enabled? The default set of logging fields specified by options?
• Why is EnableLoggingAttribute.LoggingFields get-only?
• How does this combine with the just-approved HttpLoggingOptions.FieldSelector? Does it override it?
  • When people use the attribute, do they always want to override? Or do they want a default that could still be disabled by the func which is potentially more flexible?
• Should we go back and change HttpLoggingMiddleware could allow custom code to decide whether to log or not #39200 to make the field selector func and its return value non-nullable? Its initial value would be a func that reads the endpoint metadata and returns that or options.LoggingFields.

We can revisit this when we have answers to these questions and the people working on the feature are available for API review.

[Tratcher]

• If you add [EnableLoggingAttribute] to the method, what happens? Is everything enabled? The default set of logging fields specified by options?
• Why is EnableLoggingAttribute.LoggingFields get-only?

Ah, the definition above is missing the EnableLoggingAttribute constructor. The constructor should be required as to avoid this ambiguity. fixed

How does this combine with the just-approved HttpLoggingOptions.FieldSelector? Does it override it?

• When people use the attribute, do they always want to override? Or do they want a default that could still be disabled by the func which is potentially more flexible?

Precedence:

• FieldSelector
• Attribute
• Options

These do not combine; you only get the results from the first one that's available.

• Should we go back and change HttpLoggingMiddleware could allow custom code to decide whether to log or not #39200 to make the field selector func and its return value non-nullable? Its initial value would be a func that reads the endpoint metadata and returns that or options.LoggingFields.

I think the approved API is fine.

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

The func returns null when it doesn't want to make a decision, it falls back to the attribute and then options. Implementing the attribute check inside the default func seems unnecessary from a code hygiene perspective. At most, the property itself might be non-nullable with a default _ => null.

[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.

[BrennanConroy]

API review notes:

• DisableLoggingAttribute and DisableLogging method aren't actually needed, you can use HttpLoggingFields.None for the same effect
• Rename EnableLoggingAttribute to HttpLoggingAttribute
• Rename EnableLogging method to ...?
  • SetHttpLogging
  • WithHttpLogging
  • LogHttpFields
  • Log
  • LogHttp
• In the future if we want more settings in WithHttpLogging we could add an overload that takes HttpLoggingOptions.
  • FYI, if we did this, FieldSelector would be settable again 😆
• Precedence:
  • FieldSelector
  • Attribute/Endpoint extension method
  • Options

API Approved!

namespace Microsoft.AspNetCore.HttpLogging;

+ public sealed class HttpLoggingAttribute : Attribute
+{
+    public HttpLoggingAttribute(HttpLoggingFields loggingFields, int? requestBodyLogLimit = null, int? responseBodyLogLimit = null) { }
+    public HttpLoggingFields LoggingFields { get; }
+    public int? RequestBodyLogLimit { get; }
+    public int? ResponseBodyLogLimit { get; }
+}

namespace Microsoft.AspNetCore.Builder;

+public static class HttpLoggingEndpointConventionBuilderExtensions
+{
+    public static TBuilder WithHttpLogging<TBuilder>(this TBuilder builder, HttpLoggingFields loggingFields, int? requestBodyLogLimit = null, int? responseBodyLogLimit = null) where TBuilder : IEndpointConventionBuilder;
+}

[BrennanConroy]

In order to compile, a small modification to the approved API is needed because generics (Nullable<int>) aren't allowed in attributes.

+ public sealed class HttpLoggingAttribute : Attribute
+{
-    public HttpLoggingAttribute(HttpLoggingFields loggingFields, int? requestBodyLogLimit = null, int? responseBodyLogLimit = null) { }
+    public HttpLoggingAttribute(HttpLoggingFields loggingFields, int requestBodyLogLimit = -1, int responseBodyLogLimit = -1) { }
+    public HttpLoggingFields LoggingFields { get; }
+    public int RequestBodyLogLimit { get; }
+    public int ResponseBodyLogLimit { get; }
+}

namespace Microsoft.AspNetCore.Builder;

+public static class HttpLoggingEndpointConventionBuilderExtensions
+{
+    public static TBuilder WithHttpLogging<TBuilder>(this TBuilder builder, HttpLoggingFields loggingFields, int requestBodyLogLimit = -1, int responseBodyLogLimit = -1) where TBuilder : IEndpointConventionBuilder;
+}

[Tratcher]

Thoughts about the inconsistent parameter types between the attribute constructor, properties, and the extension method? Is that going to be confusing for people?

[BrennanConroy]

Assuming I write decent API docs I don't think it's a big issue.

[halter73]

I think it might be better to make the properties non-nullable too for consistency. We're going to have to explain what -1 means anyway.

[BrennanConroy]

Should WithHttpLogging also use non-nullable ints and use -1 to mean, use the default set in options?

[halter73]

I think that makes sense for consistency. Keeps the docs the same for everything, and I figure most people aren't going to write out -1 or null and use the parameter defaults anyway.

[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.

[halter73]

API Review Notes:

• WCF uses a pattern where attributes use Nullable<int> properties and no corresponding constructor parameter. You cannot set the property to null, but you could add bool properties like IsRequestBodyLogLimitSet.
  • @Tratcher and @mconnew prefer the extra properties because it's more explicit than -1.
  • The get-only properties will not show up in intellisense when defining the attribute.
  • The RequestBodyLogLimit and ResponseBodyLogLimit getters should throw an InvalidOperationException if unset.
• @BrennanConroy and @halter73 are poisoned by working on System.IO.Pipelines, so we think -1 for "unlimited" makes sense

API Approved with IsSet properties!

namespace Microsoft.AspNetCore.HttpLogging;

+ [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
+ public sealed class HttpLoggingAttribute : Attribute
+ {
+     public HttpLoggingAttribute(HttpLoggingFields loggingFields) { }
+     public HttpLoggingFields LoggingFields { get; }

+     public bool IsRequestBodyLogLimitSet { get; }
+     public int RequestBodyLogLimit { get; set; }

+     public bool IsResponseBodyLogLimitSet { get; }
+     public int ResponseBodyLogLimit { get; set; }
+ }

namespace Microsoft.AspNetCore.Builder;

+ public static class HttpLoggingEndpointConventionBuilderExtensions
+ {
+    public static TBuilder WithHttpLogging<TBuilder>(this TBuilder builder, HttpLoggingFields loggingFields, int? requestBodyLogLimit = null, int? responseBodyLogLimit = null) where TBuilder : IEndpointConventionBuilder;
+ }