datalust / seq-extensions-logging

Add centralized log collection to ASP.NET Core apps with one line of code.
https://datalust.co/seq
Apache License 2.0
89 stars 13 forks source link

Support enrichers #28

Closed ipzKellyR closed 3 months ago

ipzKellyR commented 5 years ago

The setup for this is great because its one line and then add a config section, but using this method you cannot easily add enrichers. Am I mistaken?

nblumhardt commented 5 years ago

Thanks for the note. This package wraps up and completely internalizes these bits of Serilog, and it would probably be confusing to expose interfaces like ILogEventEnricher, but, some cut-down enrichment functionality seems useful enough to include.

Which enrichers in particular did you have in mind?

ipzKellyR commented 5 years ago

would it be possible to allow someone to pass enrichers in as options? In my case I want to use custom enrichers.

nblumhardt commented 5 years ago

Probably not via ILogEventEnricher, but something like Func<(string, object)> could be used to provide simple enrichment from ambient state, without necessitating exposure of types like LogEvent, LogEventPropertyValue and so on. Would that work in your case?

    .AddSeq(..., () => ("ThreadId", Thread.CurrentThread.ManagedThreadId))

(We'd need to figure out a workable API to allow for multiple enrichers to be passed, but the above might illustrate the basic idea..)

danbopes commented 2 years ago

@nblumhardt Any chance we can get this implemented. I hate to switch my entire logger over to serilog just to add this feature, but having hostname or the managedthreadid on each log entry would be super helpful.

nblumhardt commented 2 years ago

Thanks for the nudge, @danbopes; I'm taking another look at this now.

danbopes commented 2 years ago

@nblumhardt I ended up adding serilog but only as a logging provider to .net core, which honestly wasn't as bad as I thought. This case I could already leverage all of the .net core ILogger features without having to rewrite a ton of code:

builder.Services.AddLogging(loggingBuilder =>
{
    var serverUrl = builder.Configuration.GetValue<string>("Logging:Seq:ServerUrl");
    var apiKey = builder.Configuration.GetValue<string>("Logging:Seq:ApiKey");

    var logger = new LoggerConfiguration()
        .Enrich.WithAssemblyInformationalVersion()
        .Enrich.WithMachineName()
        .MinimumLevel.Verbose()
        .WriteTo.Console(Serilog.Events.LogEventLevel.Information)
        .WriteTo.Seq(serverUrl, apiKey: apiKey)
        .CreateLogger();

    loggingBuilder.AddSerilog(logger, true);
});

My only pain point now, is simply having to convert json back to a structure to be reencoded with json again.

nblumhardt commented 2 years ago

Thanks for the update @danbopes. UseSerilog() still supports .NET's ILogger, just in case that helps (UseSerilog() is generally a bit more predictable when it comes to levels/filters).

BrianVallelunga commented 2 years ago

Call it an enricher if you want, but it would be nice to be able to just add a few items to the global logging scope supported by ILogger's BeginScope(). I'm specifically looking to add something like a version number or application name. Just static data at the app's start, nothing more.

nblumhardt commented 2 years ago

I've kicked off some work on this; I think because it'll be best if accessible via JSON configuration, it's a good time to overhaul how the sink is configured, so that's the first cab off the rank. I'll keep this thread updated as I go :-)

w5l commented 1 year ago

What speaks against simply opening up ILogEventEnricher (or a variant of it) to dependency injection? We could then register any enricher implementation with DI and have it injected, keeping configuration simple. Adding a default implementation of EnvironmentValueEnricher and/or StaticValueEnricher to the library would cover most people's requirements.

services.AddSingleton<ILogEventEnricher>(() => new StaticValueEnricher("..."));

This matches part of the ITelemetryProcessor pattern in use by Azure AppInsights, see Filter and preprocess telemetry in the Application Insights SDK. Even more powerful, they allow post processing and filtering of the entire log message. Something like a basic ILogProcessor.Process(LogEvent event, Func<LogEvent> next); would cover pretty much every possible use case here.

Would you be open to a PR in either of these directions? Pretty much all code required to support this is already there (ILogEventEnricher, FixedPropertyEnricher, the SafeAggregateEnricher even already takes a collection). It's just all internal right now, for no obvious reason.

nblumhardt commented 1 year ago

@w5l this is already implemented via ReadFrom.Services(serviceCollection), check out the Serilog.AspNetCore examples to see how it's used (IIRC).

ReadFrom.Services() supports sinks, filters, enrichers, and destructuring policies. HTH!

nblumhardt commented 1 year ago

@w5l sorry, I see I'm in datalust-extensions-logging and not serilog-extensions-logging, which I also maintain 😅

w5l commented 1 year ago

No worries, a little shock there thinking I've overseen something glaringly obvious 🙈 but yeah, like the people before me in this thread I was hoping to have this supported in the datalust-seq version without using full-blown serilog.

amitjaura commented 5 months ago

@nblumhardt - Just checking if this is in the pipeline. It will be nice to have enrichers setup along with Seq. I like the idea of

.AddSeq(..., () => ("ThreadId", Thread.CurrentThread.ManagedThreadId))

nblumhardt commented 5 months ago

Thanks for the nudge @amitjaura, I'll try to loop back to this soon.

KodrAus commented 3 months ago

I'm having a look at implementing this over the week. This particular codebase is pretty new to me but I think the main thing we want to avoid is exposing any of the vendored Serilog internals through public APIs. That makes the more general middleware approach of ILogProcessor.Process(LogEvent event, Func<LogEvent> next); which I otherwise quite like less appealing.

Ignoring configuration for now, I think our two main options are:

  1. Func<(string, object)> or Func<IReadOnlyDictionary<string, object>> for an enricher that returns state to the caller to enrich the underlying log event with.
  2. Action<IPropertyCollection> where IPropertyCollection wraps up the LogEvent and exposes some niceties like AddIfAbsent. It would also do the translation of an object into a PropertyValue.

I think my preference is for 2. here. It gives us more flexibility to enhance the API over time and potentially avoids unnecessary allocation for enrichers that want to deal with multiple properties.

I am also happy with the original Func<(string, object)> approach, which is nicely minimal, and prevents the API from evolving past its original scope.

Thoughts?

nblumhardt commented 3 months ago

I'm also pretty keen on (2) 👍

KodrAus commented 3 months ago

I've opened up #61 that supports this using a variant of 2. I described earlier. It adds an optional IEnumerable<Action<EnrichingEvent>> enrichers parameter to AddSeq, where EnrichingEvent is a wrapper around LogEvent that exposes some of its functionality. With a collection expression it looks something like this:

AddSeq(enrichers: [
    // Simple scalar value
    (evt) => evt.AddOrUpdateProperty("A", 1),
    // Destructured object
    (evt) => evt.AddOrUpdateProperty("B", new { a: 1 }, true)
]))