Additional Func Fields

[nikolic-bojan]

Hi,

Could I suggest adding some additional functionality to provide some more extensibility to the logger? I would like to see a new property in GelfLoggerOptions, with a name AdditionalFunctionFields of type Dictionary<string, Func<GelfMessage, object>>. Purpose of this new functionality is to add more information in log that are derived from the values in the log message. Some of the possible scenarios:

1. We would like to introduce loglevel field with custom string value based on Level (we do lose some from LogLevel, but can live with that; maybe this is a hidden additional suggestion to keep this field in GelfMessage so it could be used for similar purpose)
2. We would like to log Exception type to a custom field based on Exception value

If you agree, I would fork and try to create solution which should be the least intrusive and send it to you for a review.

What do you think?

Thanks!

[mattwcole]

Hi, this feature request has actually come up a few times (#23, #26). I have an example here of how to dynamically add fields by decorating the logger provider, but it's quite a bit of code to set up and I'm definitely open to adding an easier solution. It would probably be worth checking other popular providers for Microsoft.Extensions.Logging to see if/how they implement something similar.

[nikolic-bojan]

Hi,

You are correct, this feature is pretty much on-pair with #26, but I think it is quite different from #23 which is more "dangerous" since it allows having the state in logger, which I do not think is a good idea.

Decorating logger doesn't feel right as then it would be a very custom solution and I would like you to think about extending your solution, which is very clean.

Most popular is Serilog with its Serilog.Extensions.Logging, but that one is already doing conversion of LogLevel to LogEventLevel and then the serilog-sinks-graylog kicks in with logging that converted level to _stringLevel in their GelfMessage... So much information transformed and lost in that process. Not to mention they consider 'None' level the same as 'Fatal'!

On the other hand, your library is beautifully simple and clean! By extending your GelfMessage to contain original information about log level and with these additional function properties (but to keep just GelfMessage as input side), consumers of your library would have just the right amount of extensibility to tweak their logs, but only based on the original data sent to log.

GelfMessage could contain all original data, except for state, I wouldn't open it that much, but both LogLevel, Exception and EventId could be there. That would allow complete control over how those properties are logged in GELF. What is great is that the mechanics for doing that would be the same.

My friend already submitted a PR (a bit impatient :) ), we spoke about the idea how to introduce change, but he is using the existing GelfMessage properties, but I would suggest adding those 3 from original log message.

Sorry for a very long text, but I am trying to convince you to give us green light for this extension :)

BR, Bojan

[mattwcole]

No convincing necessary, we will add support for this. I like your idea of having the raw log data available. Would some variation of this work:

public class GelfLoggerOptions
{
    ...

    /// <summary>
    /// Compute additional fields based on raw log data.
    /// </summary>
    public Func<LogLevel, EventId, Exception, Dictionary<string, object>>? AdditionalFieldsFactory { get; set; }

    ...
}

And then in GelfLogger we can pass all the data if a factory is present.

var additionalFields = _options.AdditionalFields
    .Concat(_options.AdditionalFieldsFactory?.Invoke(logLevel, eventId, exception)
            ?? Enumerable.Empty<KeyValuePair<string, object>>())
    .Concat(GetScopeAdditionalFields())
    .Concat(GetStateAdditionalFields(state));

Or if we want to include state and the formatter for more flexibility:

public class GelfLoggerOptions
{
    ...

    /// <summary>
    /// Compute additional fields based on raw log data.
    /// </summary>
    public IAdditionalFieldsFactory? AdditionalFieldsFactory { get; set; }

    ...
}

public interface IAdditionalFieldsFactory
{
    Dictionary<string, object> CreateFields<TState>(LogLevel logLevel, EventId eventId, TState state,
        Exception exception, Func<TState, Exception, string> formatter);
}

...

var additionalFields = _options.AdditionalFields
    .Concat(_options.AdditionalFieldsFactory?.CreateFields(logLevel, eventId, state, exception, formatter)
            ?? Enumerable.Empty<KeyValuePair<string, object>>())
    .Concat(GetScopeAdditionalFields())
    .Concat(GetStateAdditionalFields(state));

[nikolic-bojan]

I am totally fine with either, maybe more happy with the first option (without state), to keep it a bit more in control as state, but please you make that call.

Would you like some assistance on this or you would like to implement it?

Best regards, Bojan

[hercegyu]

First of all, I need to say I'm sorry for submitting PR at the beginning. Idea was, to show how small change can be and make this fantastic extension even better.

As Bojan pointed, you can take either solution.

Please let us know if we can help any?

Best regards, Ivan

[mattwcole]

@hercegyu no apology necessary, I'm very happy to have ideas contributed.

@nikolic-bojan great, lets go with the first option without state for now then.

If you guys are able to make the change that would be much appreciated!

[nikolic-bojan]

Great! Since @hercegyu already has PR in place, I will leave him to complete the implementation based on Option 1. Thanks!

[mattwcole]

Released as v2.1.0-pre1 (stable version will follow in a few days). Thanks for the contribution!