serilog / serilog-aspnetcore

Serilog integration for ASP.NET Core
Apache License 2.0
1.31k stars 205 forks source link

Make possible of RequestLoggingMiddleware to Log Headers as optional #343

Open rodrigopinto opened 1 year ago

rodrigopinto commented 1 year ago

Is your feature request related to a problem? Please describe. No.

Describe the solution you'd like The RequestLoggingMiddleware.cs has its standard HTTP request attributes that are logged for every request. The HTTP Headers from the request/response are not part of it.

Considering asp net core 7, the standard HTTP logging allows to add headers to the logging middleware and this is not available on Serilog middleware.

Would be great to have Headers as optional, to be added to the list of fields logged. It could be disabled by default to be backward compatible.

For that work, would require adding to RequestLoggingOptions the Headers attribute with some sort of configuration similarly the standard HTTP logging has.

Below is a pseudo-code, just to give an idea.

//... PSEUDO CODE

// RequesHeaders allow list should be configured on the `UseSerilogRequestLogging` call.
public List<RequestHeaders> RequestHeaders { get; set; }

static LogEventProperty GetHeadersProperties(){
   var requestHeaders = httpContext?.Request?.Headers;

   List<object> headers = [];

   foreach (var headerName in RequestHeaders()) {
      if (requestHeaders.TryGetValue(headerName, out var headerValue))
      {
         headers.Add(headerName, headerValue)
      }

  }

  new LogEventProperty("Headers", new ScalarValue(headers)),
}

public RequestLoggingOptions()
    {
        GetLevel = DefaultGetLevel;
        MessageTemplate = DefaultRequestCompletionMessageTemplate;
        GetMessageTemplateProperties = DefaultGetMessageTemplateProperties;
        HeadersProperties = GetHeadersProperties;
    }

The configuration of which headers would be logged should happen on the UseSerilogRequestLogging. Similar to the template that can be customized, the headers list could configured the allow list no the same moment.

Describe alternatives you've considered

Using the Enricher is an option but not ideal, as Enrichers are applied any time you use a logger instance. Also, it is possible to make EnrichDiagnosticContext on the configuration of the middleware, but I think this logic could be part of the standard of the library instead of putting some logic on the builder setup.

nblumhardt commented 1 year ago

Makes sense, thanks for the suggestion.

Currently, I believe you can actually do this using GetMessageTemplateProperties() - since the returned values are named rather than positional, you could pull out a Headers property there, alongside whatever the template needs, and have it attached to the result (without any impact on the message).

Seems like this could be improved/streamlined a bit, e.g. by replacing GetMessageTemplateProperties with GetCompletionEventProperties or similar. Needs a little bit of thought 🤔