Add RequestHost & RequestScheme to log properties

[tomkerkhove]

Add RequestHost & RequestScheme to log properties and list all options in docs

[tomkerkhove]

Fixed, thanks @sungam3r

[nblumhardt]

Thanks @tomkerkhove!

These are useful, and there will be a lot of users who'd benefit from them, but others may not (and/or may need other information about the request).

Perhaps, to prevent the request completion events from getting bloated, we allow customization of the set of logged properties through the options object?

E.g.:

app.UseSerilogRequestLogging(opts => {
    opts.ExtendDiagnosticContext = (diagnosticContext, httpContext) => {
        diagnosticContext.Set("RequestHost", httpContext.Request.Host.Value);
        diagnosticContext.Set("RequestScheme", httpContext.Request.Scheme);
    };
});

(Not sure about the names/details, but the two delegate parameters would be an IDiagnosticContext and an HttpContext.)

Thoughts?

[adamchester]

Sounds good @nblumhardt.

What about before & after ‘next’ middleware has been executed? Would only “after” be enough? What about when an exception happens?

[nblumhardt]

I think after would be sufficient for the cases I can think of; it's not all that common to remove information from the request/context as it executes, but information is sometimes added.

Immediately before retrieving the context values in LogCompletion seems the natural place to invoke the callback:

serilog-aspnetcore/src/Serilog.AspNetCore/AspNetCore/RequestLoggingMiddleware.cs

Line 79 in 7116569

if (!collector.TryComplete(out var collectedProperties))

If an exception occurred, we'd have a chance to pass it through, too (as we would the timing info). Do we try to make the argument list a bit broader than just dc and httpContext, or is that bloat? The exception seems somewhat compelling - but it's hard to draw the line...

[tomkerkhove]

What if, instead of using the following:

app.UseSerilogRequestLogging(opts => {
    opts.ExtendDiagnosticContext = (diagnosticContext, httpContext) => {
        diagnosticContext.Set("RequestHost", httpContext.Request.Host.Value);
        diagnosticContext.Set("RequestScheme", httpContext.Request.Scheme);
    };
});

You can just provide a func that returns a dictionary and input is httpContext?

app.UseSerilogRequestLogging(options => {
    options.GatherInformationBeforeCall = (httpContext) => {
return new Dictionary<string, string>{{"RequestHost", httpContext.Request.Host.Value}};
    };
    options.GatherInformationAfter = (httpContext) => {
return new Dictionary<string, string>{{"RequestHost", httpContext.Request.Host.Value}};
    };
});

Not required, otherwise I'll have a look at the suggested scenario.

[nblumhardt]

Thanks for the suggestion, @tomkerkhove!

There are a couple of complications with using dictionaries here that I think we should avoid; first, they're rather large to allocate, but also, IDiagnosticContext.Set() has an optional third parameter to control capturing, which wouldn't be represented in the two-element key-value pairs in the dictionary.

The before/after call split is tricky, I'd lean towards keeping it to a single callback to minimize the (ever-growing :-)) surface area, unless we've got a compelling use case for a before hook.

Semi-related, would it be misleading to call it EnrichDiagnosticContext? Enrich is the term Serilog uses for this, but it's generally in relation to events/ILogEventEnrichers. Still seems like it's worth considering in this case 🤔

[tomkerkhove]

I've aligned with your suggested but I'm not an expert on IDiagnosticContext, will it automatically add it to the output or do we have to do something else?

Was going to update the tests but they are still TODO 😅

[nblumhardt]

Fantastic - thanks Tom! Just made some nitpicky edits to the README and examples, but this looks good, let's ship it :-)

[tomkerkhove]

My pleasure!

A few questions though:

• I didn't test this as I wasn't sure that this would be enough? Will the diagnostic context automatically add the properties or?
• How did you add commits to my fork? Just curious as I'm interested to see how I could do that during reviews as well

[nblumhardt]

Cool! :-)

Yes, the diagnostic context will add properties just like that 👍

Some testing on the dev branch build (it's 3.2.0-dev-* on NuGet) will be needed before we stamp RTM on it, but the bar for testing isn't too high for pre-release builds. If you spot ways we could improve it, there's heaps of room for iteration.

The files in a PR have an 'edit' button that will simplify editing someone's fork in a PR (they need to have left the "Allow edits from maintainers" checkbox ticked when creating the PR). It's also possible to push directly to the PR source branch in their repo via the same permissions, using regular git commands.

I use it quite a bit for nitpicky stuff that generates busywork - it can be ideal for avoiding trivial review cycles. Hope you find it handy! :-)

[tomkerkhove]

Awesome, thanks! Have a lot of small comments as well and this way I can help them, thanks for the tip!

[skomis-mm]

I think after would be sufficient for the cases I can think of; it's not all that common to remove information from the request/context as it executes, but information is sometimes added.

@nblumhardt Just for the record - the one is ForwardedHeadersMiddleware that rewrites headers collection. There is no way to enrich with original headers no matter where UseSerilogRequestLogging(..) is placed unless using custom middleware that enriches collector before next() call.
Not sure if its worth to be fixed.. :)