mattwcole / gelf-extensions-logging

GELF provider for Microsoft.Extensions.Logging
MIT License
109 stars 42 forks source link

Provide a way for short_message to be used as message title #40

Closed sarmis closed 4 years ago

sarmis commented 5 years ago

https://docs.graylog.org/en/3.1/pages/gelf.html#gelf-payload-specification

In gelf, the short_message field is considered to be the message title and the (optional) full_message to contain the full message.

This is helpfull when viewing multiple messages in graylog since by default the short_message is used as title. One approach would be to check for the existing of a short_message field in additionalFields and if found set that as the GelfMessage.ShortMessage

mattwcole commented 5 years ago

Hi, thanks for pointing this out. I think this should be possible. I probably chose short_message instead of full_message because it is mandatory. We just need a way to pass the title down. Yes we could support setting the title in additionalFields, and maybe also with log scopes.

using(_logger.BeginScope("Title goes here")) {
    _logger.LogInfo("Message goes here");
    _logger.LogInfo("And another one here");
}

I'll have some time to look at this within the next couple of weeks, but if you need the change quickly, feel free to put up a pull request.

sarmis commented 5 years ago

After carefully thinking the options regarding seperating short_message, full_message and additional fields, and taking into account that, while gelf supports these properties, other loggers may not and there should be possible to produce decent looking messages both in gelf, in source code and in console I would like to suggest the following (and I'm willing to write it and test it)

Log*(<short_message with {fields}>:<full_message with {fields}> ( {fields} that do not appear in short/full messages ) )

in short, treat the first double colon (:) as separator for short_message / full_message and the last brackets ((/)) as separator for fields that should only be send as additional fields.

imho, this approach has the benefit of being pretty in source code, in console logs and in graylog.

If you are ok with this approach, I would like to contribute it to your project since I believe that it would be useful for others.

For backwards compatibility we could include a flag as additional trigger that will enable this kind of processing

For example Log("Error {errno}: Cannot Find {item_id} ({req_id} {user_id})", 123, 5001, 1233, 344) would appear as Error 123: Cannot find 5001 (1233 344) in any non-structured logger but in GELF loggers would appear as

short_message: Error 123
full_message: Cannot find 5001
fields
  - errno: 123
  - item_id: 5001
  - req_id: 1233
  - user_id: 344
mattwcole commented 5 years ago

I’d be more inclined to use log scopes over parsing the title from the message. I believe this is the intended mechanism for passing data to the logger. We may also be able to include extension methods for ILogger allowing title to be added to the scope easily.

public static void LogInfo(this ILogger logger, string message, string title) {
    using(logger.BeginScope(title)) {
        logger.LogInfo(message);
    }
}
sarmis commented 5 years ago

Ok, I understand your considerations regarding parsing the message, and what you propose seems fine I will give it a try during the week

mattwcole commented 5 years ago

I had a go at implementing this today. What I found is that Graylog appears to treat full_message just like any other additional field. When I send both short_message and full_message this is how they appear in Graylog (v3.1):

image

short_message is displayed as "message", and full_message is a field. For this reason I don't think we need to extend the functionality of the library since you can achieve the same result with an additional field in the log scope.

public static void LogInformation(this ILogger logger, string shortMessage, string fullMessage) {
    using(logger.BeginScope(("full_message", fullMessage))) {
        logger.LogInformation(shortMessage);
    }
}

Happy to revisit if there are some scenarios where full_message is preferable over an additional field.