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
84 stars 12 forks source link

Allow logging string as JSON object using the @ directive #46

Closed omidkrad closed 1 year ago

omidkrad commented 3 years ago

This is a feature request to add better support for JSON logging. My suggestion is to treat all string objects that are passed with the @ directive as JSON, and if it couldn't parse as JSON then record them as string. Logging without the @ directive should make it to log as string.

This makes the implementation more consistent with how seq-logging works in NodeJS that allows easy logging of JSON objects. It also allows the user to take care of the serialization to JSON which can work with both Newtonsoft and System.Text.Json.

nblumhardt commented 3 years ago

Thanks for the suggestion; I think #45 has a better path to implementation (it fits with the current API and internals); perhaps we reopen that one?

Taking the string-as-JSON route would pose more questions about dealing with invalid JSON, parsing performance, etc.

omidkrad commented 3 years ago

Sure, we can reopen #45. So there are 3 suggestions right now:

  1. Serialize the object honoring JSON formatting attributes. This requires adding configuration to AddSeq(...) method to select JSON library (Newtonsoft or Dotnet) including JsonSerializerOptions.
  2. Allow serializing JsonDocument. However, it looks like JsonDocument can be created only when deserializing a JSON string. So with that, the object needs to be serialized to JSON and then deserialized to JsonDocument before being passed to the logger. Because of that, I don't think this is very intuitive.
  3. Logging JSON string specified with @ as JSON object, and fall-back to string if cannot parse as JSON because of invalid JSON etc. I think this is the most intuitive one because first, it allows selecting between JSON and string by specifying the @ directive or not, and second it allows the user to use any JSON serialization method they want.
danbopes commented 2 years ago

Is there anyway I can simply pass a specific "don't parse" object, something like _log.BeginScope(new Dictionary<string, object>{ ("obj", new RawString(jsonEncodedString)) }. I hate how rigid this class is when it comes to formatting scope values.

danbopes commented 2 years ago

For those of you stumbling upon this, I literally had to convert it to a json string (Using the JSON utility that was provided by another library), then use json.net to convert it back to a dictionary, only to have seq RECONVERT it back to json...what a computationally expensive, and dumb thing, but it works:

https://stackoverflow.com/questions/11561597/deserialize-json-recursively-to-idictionarystring-object

using var scope = _log.BeginScope(new KeyValuePair<string, object?>[]
{
    new("@Message", jsonMsgStr != null ? JsonConvert.DeserializeObject<IDictionary<string, object>>(jsonMsgStr, new DictionaryConverter()) : null)
});
nblumhardt commented 2 years ago

Thanks @danbopes. We'll definitely loop back around to this one when we're able to 👍

danbopes commented 2 years ago

@nblumhardt Appreciate it.

For a quick and dirty fix, I really think any string value when the property has an @ before it, should not be treated as a literal string.

By specifying @, you're signaling that you want to attempt to destructure the value, and if it's a string, then I think you can safely assume that the string is in the correct format (Formatted as json).

nblumhardt commented 2 years ago

Thanks for your reply. Unfortunately, it's a very common pattern in the wild for people to notice the @ in log statements and just do it everywhere (despite this not making much sense). I think the ship has already sailed on that one, and introducing new behavior behind @ now could cause a lot of unexpected breakage.

Something along the lines of:

using var scope = _log.BeginScope(new KeyValuePair<string, object?>[]
{
    new("@Message", new JsonString(jsonMsgStr))
});

could work (ignoring the problems of where to put the JsonString type) 🤔

tobiaszuercher commented 2 years ago

any progress on this?

nblumhardt commented 2 years ago

Not recently, @tobiaszuercher, but this package is on our list for a refresh soon, so we'll keep this in mind.

nblumhardt commented 1 year ago

Some work-in-progress! https://github.com/datalust/seq-extensions-logging/pull/51

nblumhardt commented 1 year ago

This is now published in 6.1.0 :tada: