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

Support JSON with objects (not only flat key-value) #723

Closed
di97mni opened this issue May 31, 2015 · 33 comments
Closed

Support JSON with objects (not only flat key-value) #723

di97mni opened this issue May 31, 2015 · 33 comments

Comments

@di97mni
Copy link

di97mni commented May 31, 2015

Version: NLog 4.0 RC

From this post:

This configuration

<target name="jsonFile" xsi:type="File" fileName="${basedir}/logs/${shortdate}.json">
    <layout xsi:type="JsonLayout">
        <attribute name="time" layout="${longdate}" />
        <attribute name="level" layout="${level:upperCase=true}"/>
        <attribute name="message" layout="${message}" />
        <attribute name="properties" layout="${all-event-properties}" />
    </layout>
</target>

Generates this. Notice properties

{
  "time": "2015-05-30 17:11:50.7131",
  "level": "INFO",
  "message": "Start",
  "properties": "CallerMemberName=Main, CallerFilePath=C:\\Temp\\NLogJson\\ConsoleApplication1\\ConsoleApplication1\\Program.cs, CallerLineNumber=13"
}

Instead of a key value string, create a JSON array.

{
  "time": "2015-05-30 17:27:55.0730",
  "level": "INFO",
  "message": "Start",
  "properties": {
    "CallerMemberName": "Main",
    "CallerFilePath": "C:\\Temp\\NLogJson\\ConsoleApplication1\\ConsoleApplication1\\Program.cs",
    "CallerLineNumber": "13"
  }
}

Maybe this is also needed for other event properties?

@304NotModified
Copy link
Member

note: the proposal isn't a JSON array, but a JSON object.

@304NotModified 304NotModified changed the title Support JSON array for properties Support JSON with objects (not only flat key-value) May 31, 2015
@di97mni
Copy link
Author

di97mni commented May 31, 2015

Ah, you're right. Of course it is. My mistake.

@304NotModified
Copy link
Member

This is difficult issue, because the JSONlayout isn't aware of the ${all-event-properties} and visa versa.

Any idea how we could solve this?

@VictorNicollet
Copy link

I can imagine three ways of doing this.

The dirtiest but easiest version would be to allow a parameter ${all-event-properties:format=JSON} where the output is {"key":value} instead of the standard key=value format. Then, a raw tag in the XML configuration file would tell the JSON layout that the value should not be output as string, but rather as raw JSON (which may include a syntax check).

    <attribute name="properties" layout="${all-event-properties}" raw />

A cleaner (but quite limited) alternative would be to recognize that some layout renderers (such as ${event-properties}) act as dictionaries ; ${event-properties:item=X}` is not specifying a rendering option, but a key to be rendered. Such dictionaries could be displayed with:

    <attributes name="properties" layout="${event-properties}" each="item" />

At the layout renderer level, a new function would be required to enumerate those values:

    [EnumerableParameter]
    [RequiredParameter]
    public string Item { get; set; }

    public override IEnumerable<string> GetEach(string which, LogEventInfo logEvent) 
    {
        if (which == "Item") 
            // enumerate all possible values of "Item", based on the values provided 
            // for the other parameters
        else
            throw new ArgumentException("Not enumerable: " + which, "which");
    }

Thus, <attributes> would instantiate the layout renderer with whatever parameters are provided in the layout=, call GetEach with the value provided in each=, and then output key-value pairs by setting Item to each value returned by GetEach.

The cleanest would be to allow renderers to return a certain number of non-string types, including JSON ones (arrays, dictionaries) but also non-JSON (DateTime), when those types are a better representation of what is being output. The layout itself would then decide whether to convert these to strings, JSON, XML, and so on. This might be a bit overengineered, though.

@304NotModified
Copy link
Member

Thanks! I will think about these proposals.

@s-sreenath
Copy link
Contributor

The dirtiest but easiest version would be to allow a parameter ${all-event-properties:format=JSON} where the output is {"key":value}

I was looking into this item and in my opinion the first solution looks more clean as well as matching other formatting layouts which we already have in place. For Eampple we specify "toString", "mm/dd/yyyy" for dates.

@304NotModified
Copy link
Member

Thanks Sree,

Yes I like that one also the best one. But do we need the raw attribute? It can be a property of the layout renderer?

@s-sreenath
Copy link
Contributor

@304NotModified I am not sure how the raw attribute will be used with toJSON formatting. What I was thinking is if the format is set to toJSON, then we can render as depicted in the initial comment of this ticket.

For Serialization following comes to my mind.

@s-sreenath
Copy link
Contributor

Do any one have any other suggestions on this?

@304NotModified
Copy link
Member

note to others: this has been discussed on the dev chat.

The proposal of @page-not-found by including the javascript serializer code from coreFX is the best one :)

So we will implement this in NLog core (not web). No need for dependency injection now

@304NotModified
Copy link
Member

@page-not-found are you still working on this? I would to release this in 4.3 (1-2 weeks from now)

@s-sreenath
Copy link
Contributor

Yes was working in this, but did not find time. May be we can think this
for version 5?
On Jan 16, 2016 19:26, "Julian Verdurmen" notifications@github.com wrote:

@page-not-found https://github.com/Page-Not-Found are you still working
on this? I would to release this in 4.3 (1-2 weeks from now)


Reply to this email directly or view it on GitHub
#723 (comment).

@s-sreenath
Copy link
Contributor

@304NotModified Do you think other wise? I am in middle of the implementation, trying to get the the unit testing completed.

@304NotModified
Copy link
Member

@page-not-found any success on this?

@s-sreenath
Copy link
Contributor

With the way it is done right now. It is getting tough to put in any new
change but I am almost there to completion stage just not able to find
enough time to do self review and unit testing.
On Feb 3, 2016 6:12 PM, "Julian Verdurmen" notifications@github.com wrote:

@page-not-found https://github.com/Page-Not-Found any success on this?


Reply to this email directly or view it on GitHub
#723 (comment).

@304NotModified
Copy link
Member

Well you can PR it and we can review it of course. Not problem that it's WIP. If you write 1 unit test, then we can easily create more.

@s-sreenath
Copy link
Contributor

I will try to get this out this week.
On Feb 4, 2016 5:42 AM, "Julian Verdurmen" notifications@github.com wrote:

Well you can PR it and we can review it of course. Not problem that it's
WIP. If you write 1 unit test, then we can easily create more.


Reply to this email directly or view it on GitHub
#723 (comment).

@304NotModified
Copy link
Member

Cool :)

@s-sreenath
Copy link
Contributor

I am making some progress here. Would be able to get some base beta version my upcoming week. Sorry I am keep extending the time, is because with the current code it is getting harder to add additional things to one area without affecting others.

@304NotModified
Copy link
Member

This is supported by nesting JsonLayouts. See https://github.com/NLog/NLog/wiki/JsonLayout#advanced-examples

@gpolaert
Copy link

Hi @304NotModified,
I've closed this issue, but did you solve it? I try to generate a Json object from the properties array set in my code.

How can I achieve that with the all-event-properties layout render? I don't want to specify all the potential properties in a nested layout.

Thanks ;)

@304NotModified
Copy link
Member

@gpolaert

This is solved in two ways:

  1. You can define an object with nested <attribute
  2. and/or you can use encode="false" and ${Json-Encode-Layout-Renderer}

Too bad, the all-event-properties isn't aware of JSON, so it's printing a (non-JSON) string.

We like to fix this, but we need an approach which works for (almost) every layout renderer. If you think this is needed/desired, please create a new issue. :)

@gpolaert
Copy link

Thanks. I think it's very useful features, especially if you have a log management solution :) (I'm coming from Logmatic.io ^^)

I don't know .Net to be honest but I will check how can we handle that.
If I have a proposal, I will open an issue. What are the main difficulties about serialize in Json the renderer?

@jholovacs
Copy link

I don't see how this is supported by JsonNestingLayouts, and the example in the link doesn't address the situation very well. The example specifies an exception, that has two well-known properties, not a dynamic list of key-value pairs of type string, object. Can we either A) update the documentation with an example of how to make this work, or B) reopen this issue?

@304NotModified
Copy link
Member

Please open a new issue on github or (preferred) stackoverflow.

@jholovacs
Copy link

@304NotModified
Copy link
Member

304NotModified commented May 18, 2017

Ah, I now fully understand your question,

There is a new option includeAllProperties on the JsonLayout

e.g.

    <target xsi:type="ColoredConsole" name="coloredConsole">
      <layout xsi:type="JsonLayout"  includeAllProperties="true"> 
        <attribute name="timestamp" layout="${longdate}"/>
        <attribute name="level" layout="${level:uppercase=true}"/>
        <attribute name="exception" layout="${onexception:${exception:format=tostring}}" />
    
      </layout>
    </target>

You could also skip some properties with excludeProperties="exclude1, exclude2"

@304NotModified
Copy link
Member

also posted on StackOverflow

@jholovacs
Copy link

jholovacs commented May 18, 2017

I'm still getting this sort of thing:

{ "timestamp": "2017-05-18 15:47:50.1177", "level": "ERROR", "message": "PushAdvertiserConversionRule", "properties": "properties=System.Collections.Generic.Dictionary`2[System.String,System.Object], tags=System.Collections.Generic.List`1[System.String]", "properties": "System.Collections.Generic.Dictionary`2[System.String,System.Object]", "tags": "System.Collections.Generic.List`1[System.String]" }

it's basically taking the object and doing .ToString() on it, instead of serializing to JSON.

@304NotModified
Copy link
Member

Unfortunately, it was thinking that was fixed already.

@304NotModified
Copy link
Member

We have a ijsonserializer, that one should be used

@jholovacs
Copy link

Thanks

@304NotModified
Copy link
Member

@jholovacs created a new issue for it. #2127

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

6 participants