Layer enrichment & redaction on top of HttpLogging #4056
Conversation
Tratcher
force-pushed
the
tratcher/httplogging
branch
from
609da5d to
b37e1ce
Compare
| [Required] | ||
| [SuppressMessage("Usage", "CA2227:Collection properties should be read only", | ||
| Justification = "Options pattern.")] | ||
| public ISet<string> ExcludePathStartsWith { get; set; } = new HashSet<string>(); |
There was a problem hiding this comment.
This functionality doesn't seem like it belongs on RedactionOptions in an ideal world. Filtering (in whatever form we think is most appropriate) feels like something that would be available in the base layer without having opted into any redaction. Should I think of this as refactoring is still in progress or we believe this needs to stay here because of various time or design constraints?
There was a problem hiding this comment.
It's certainly a design point that needs to be resolved. For now it's here because it's an option that the extensions middleware has and the aspnetcore one doesn't. I wouldn't move it to aspnetcore as is, this is an anti-pattern that ends up duplicating routing. I do understand it was done this way because it's easy and config compatible.
We will revisit this detail once we agree on the larger design.
There was a problem hiding this comment.
What would be a config-friendly way of doing this which is not an anti-pattern?
There was a problem hiding this comment.
dotnet/aspnetcore#43642 would allow you to apply metadata (like the logging attribute) to arbitrary route paths and be more config friendly. I'll ping them to see how that's coming along.
...icrosoft.AspNetCore.Telemetry.Middleware/Logging/Internal/HttpLoggingRedactionInterceptor.cs
Outdated
Show resolved
Hide resolved
...icrosoft.AspNetCore.Telemetry.Middleware/Logging/Internal/HttpLoggingRedactionInterceptor.cs
Outdated
Show resolved
Hide resolved
| if (logContext.TryOverride(HttpLoggingFields.RequestHeaders)) | ||
| { | ||
| // TODO: HttpLoggingOptions.Request/ResponseHeaders are ignored which could be confusing. | ||
| // Do we try to reconcile that with LoggingRedactionOptions.RequestHeadersDataClasses? |
There was a problem hiding this comment.
I think this behavior as-is seems pretty reasonable and we'd document something like "Invoking AddHttpLoggingRedaction() offers more sophisticated redaction customization options for path and headers. The basic header configuration settings in HttpLoggingOptions will no longer be used when this is enabled."
...icrosoft.AspNetCore.Telemetry.Middleware/Logging/Internal/HttpLoggingRedactionInterceptor.cs
Outdated
Show resolved
Hide resolved
...icrosoft.AspNetCore.Telemetry.Middleware/Logging/Internal/HttpLoggingRedactionInterceptor.cs
Outdated
Show resolved
Hide resolved
...icrosoft.AspNetCore.Telemetry.Middleware/Logging/Internal/HttpLoggingRedactionInterceptor.cs
Outdated
Show resolved
Hide resolved
...icrosoft.AspNetCore.Telemetry.Middleware/Logging/Internal/HttpLoggingRedactionInterceptor.cs
Outdated
Show resolved
Hide resolved
|
I presume a bunch of stuff would still need to be deleted when enabling this stuff right? Some extension methods, the option type, etc? |
| /// <remarks> | ||
| /// Default set to <see cref="HttpRouteParameterRedactionMode.Strict"/>. | ||
| /// </remarks> | ||
| [Experimental] |
There was a problem hiding this comment.
This whole type needs to be marked [Experimental]
| /// <param name="services">The service collection.</param> | ||
| /// <param name="configureRedaction">Configures the redaction options.</param> | ||
| /// <returns>The original service collection.</returns> | ||
| public static IServiceCollection AddHttpLoggingRedaction(this IServiceCollection services, Action<LoggingRedactionOptions> configureRedaction) |
There was a problem hiding this comment.
Mark any new functions with [Experimental]
Yeah, I haven't purged the old code yet, I'll do that before undrafting. For now I want to keep it focused on the new design. |
|
Making a new PR for release/8.0 |
ghost
locked as resolved and limited conversation to collaborators
This is a proof of concept that re-layers onto the new extensibility in dotnet/aspnetcore#48679.
In theory this replaces the local logging middleware.
Doesn't compile until the aspnetcore PR gets merged
Microsoft Reviewers: Open in CodeFlow