Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine 'ILogEventEnricher' abstraction with 'IDiagnosticContext' #222

Open
julealgon opened this issue Dec 8, 2020 · 0 comments
Open

Comments

@julealgon
Copy link

Is your feature request related to a problem? Please describe.
I just had to create a substantial amount of code to add many other properties from the current request to my logs. Since I knew upfront that I'd need a lot of different things added (and I wanted to potentially reuse these across many projects), I created a new interface, with the same signature of the Func used in the request logging options:

    public interface IDiagnosticContextEnricher
    {
        void Enrich(IDiagnosticContext diagnosticContext, HttpContext httpContext);
    }

Then I created several different implementations that populated different properties in IDiagnosticContext, and wrapped them all in a composite pattern:

    public sealed class CompositeDiagnosticContextEnricher : IDiagnosticContextEnricher
    {
        private readonly IEnumerable<IDiagnosticContextEnricher> enrichers;

        public CompositeDiagnosticContextEnricher(IEnumerable<IDiagnosticContextEnricher> enrichers)
        {
            this.enrichers = enrichers;
        }

        void IDiagnosticContextEnricher.Enrich(IDiagnosticContext diagnosticContext, HttpContext httpContext)
        {
            foreach (var enricher in this.enrichers)
            {
                enricher.Enrich(diagnosticContext, httpContext);
            }
        }
    }

And finally, I registered the composite in the container, then injected it into the Startup's Configure method:

        public void ConfigureServices(IServiceCollection services)
        {
            _ = services
                .AddMyCompositeHereWithSomeHelperCodeSinceMSContainerDoesntSupportComposites()
                .AddABunchOfOtherStuff();
        }

        public static void Configure(IApplicationBuilder app, IDiagnosticContextEnricher diagnosticContextEnricher)
        {
            _ = app
                .UseSerilogRequestLogging(o =>
                {
                    o.EnrichDiagnosticContext = diagnosticContextEnricher.Enrich;
                })
                .UseABunchOfOtherStuff(...);
        }

While doing all this, I strongly felt like I was "reinventing the wheel" so to speak: this seemed awfully similar to standard ILogEventEnricher. In a way, IDiagnosticContext is pretty much a "delayed, scoped enricher".

Describe the solution you'd like
I'd like a streamlined way to "enrich" logs that works consistently, no matter if I'm "enriching all logs" (the standard use for enrichers today), or if I'm "enriching aggregated request logs" (how IDiagnosticContext works in tandem with the final "Log" call inside the middleware).

I don't necessarily have a suggestion for the implementation quite yet, but the concept of a separate IDiagnosticContext abstraction seem ill-advised to me. I'd rather see the enrichment concept applied more globally and made work with both flows.

FAKE EDIT:

I do have something in mind for this which could potentially work: "allow combining logs when a log scope ends".

The way response logs work today is that they "store all properties to be added to the log" and then "log everything in a single event at the end". This is why a custom IDiagnosticContext abstraction is needed, to store these properties that are collected from multiple places and only generate a single event in the end by "fetching" the properties and enriching the event.

What if Serilog was able to "batch all events related to a given log scope until the scope is finished"? Say for example, that inside the request logging middleware, a log scope is created (using the BeginScope MEL API). Now, Serilog would have a custom scope implementation that would start "batching all calls to Log and storing all properties", and then once the scope is disposed, all those "batched log events" are "combined into a single event" and the properties are all merged together.

Ideally, the "event combination" configuration would not be in the BeginScope calls themselves, but somewhere in the logger configuration: imagine you could configure any given scope (by their name/value) to optionally aggregate all events on scope disposal. This would allow consumers to opt-in to event aggregation for any given sequence of Log calls, provided they all happen inside the scope. That would be quite powerful IMHO as that, to me, is semantically what is happening with request logging: you are "combining all logs that happen during the whole request into a single event". Even the "Elapsed" property could be automatically computed that way, by generating it as "how long was the scope alive" (from BeginScope call to Dispose). This would give the capability of automatically measuring the performance of any user-defined scope just by configuring the logger externally.

I could envision this being implemented as a custom sink, kinda similar to how the batching sink works, by temporarily storing the events and then emitting them once the scope is disposed (not sure how to provide the signal, though).

I think an approach such as this would be more general purpose and would fit the requirements of request logging, without introducing a separate IDiagnosticContext abstraction. One could then add any enrichers to the log and those enrichers would also be applied only at the log call (as they do today).

I understand this is a significant change (at least it feels like to me, since it opens up a big can of worms like "how do we combine the various logs messages?" or "what is the final 'logLevel' of the event if there are many different levels among the logs", or even "what happens if multiple logs have different values for the same properties, do they become arrays?"), but I thought I'd share anyways.

Describe alternatives you've considered
No alternatives. I coded a lot of stuff from scratch unfortunately, and that's what I feel would be nice to be avoided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant