You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@logging.apache.org by mdvorak <gi...@git.apache.org> on 2017/09/12 15:14:21 UTC

[GitHub] logging-log4j2 pull request #110: JsonLayout support for custom key-value pa...

GitHub user mdvorak opened a pull request:

    https://github.com/apache/logging-log4j2/pull/110

    JsonLayout support for custom key-value pairs being inserted into JSON

    _Note: This is not ready PR, there are no unit tests for new feature - please review before i'll invest more time into it._
    
    * Changed `AbstractJacksonLayout.convertMutableToLog4jEvent` to `protected Object wrapLogEvent(LogEvent event)`
    * Added Extras as `KeyValuePair[]` to `JsonLayout`
    * When extras as specified, `LogEvent` is wrapped in new `LogEventWithExtras` class, which merges `LogEvent` serialized data with provided extras
    
    This is generally wanted feature, for example see here:
    https://github.com/ggrandes/log4j2-simplejson
    https://github.com/majikthys/log4j2-logstash-jsonevent-layout
    
    How it works:
    ```xml
    <JsonLayout>
      <KeyValuePair key="hostName" value="${hostName}"/>
    </JsonLayout>
    ```
    Inserts into each generated JSON `hostName` key:
    ```json
    {
    "timeMillis":1505228896690,
    "thread":"main",
    "level":"INFO",
    "loggerName":"org.springframework....",
    "message":"Refreshing ...",
    "endOfBatch":false,
    "loggerFqcn":"org....",
    "contextMap":{},
    "threadId":1,
    "threadPriority":5,
    "hostName":"wp21257b"
    }
    ```
    
    This is accomplished by tricks with Jackon json serializer:
    ```java
    public static class LogEventWithExtras {
        // ....
        @JsonUnwrapped // Deserializes actual LogEvent into root object
        public Object getLogEvent() { return logEvent; }
    
        @JsonAnyGetter // Adds everything from the map into root object
        public Map<String, Object> getExtras() { return extras; }
    }
    ```

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/mdvorak/logging-log4j2 jsonlayout-extras29

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/logging-log4j2/pull/110.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #110
    
----
commit ba78e4b08727d2ce558120731d40133122a9fb05
Author: Michal Dvorak (cen38289) <mi...@csas.cz>
Date:   2017-09-12T12:31:52Z

    JsonLayout support for custom key-value pairs being inserted into JSON
    
    Changed AbstractJacksonLayout.convertMutableToLog4jEvent to protected Object wrapLogEvent(LogEvent event)
    Added Extras as KeyValuePair[] to JsonLayout
    When extras as specified, LogEvent is wrapped in new LogEventWithExtras class, which merges LogEvent serialized data with provided extras

----


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    Link to JIRA issue:
    https://issues.apache.org/jira/browse/LOG4J2-1694


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by dankirkd <gi...@git.apache.org>.

Github user dankirkd commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    Does this support variable replacement.
    
    i.e. in PatternLayout one can do something like this:
    
    `<Pattern>%d{ISO8601}{UTC} %d{Z}{UTC} [CID:%X{conversation-id}] [%t] %-5p %c{1.} - %m%n</Pattern>`
    
    But it seems that 
    
    `<KeyValuePair key="CID" value="%X{conversation-id}"/>`
    
    or even
    
    `<KeyValuePair key="CID" value="${conversation-id}"/>`
    
    doesn't work.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    @mikaelstaldal Yeah i figured after digging deeper. Implemented it with correct support.
    
    Only thing that bugs me is that dynamically created LinkedHashMap - thats wasteful and expensive (I know Jackson alone is not exactly garbage-free, but still).
    
    I'm thinking about using `ThreadLocal`s, but I understand there are both potentional memory leaks, and Log4j alone has special policies about that (`log4j2.enable.threadlocals`), and I don't have atm time to dive into that. Any ideas? Or do you consider it fine as it is? 


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    @mikaelstaldal I've looked at `GelfLayout`, and it resolves `${}` expressions during serialization. Is this really nescessary? Looks like pretty big overhead to me. I've tested only XML config, but expressions are resolved automatically when parsed. Won't there be a possible conflict? I don't see so far.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    Partially, log4j-core completed successfully including all tests, whole project failed somewhere on Cassandra with some JNI exception. I did not look further into that yet. Does not seem related. 


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mikaelstaldal <gi...@git.apache.org>.

Github user mikaelstaldal commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    @mdvorak Resolving `${}` expressions during serialization is necessary to support dynamic lookups which depend on the current log event, such as [ContextMapLookup](https://logging.apache.org/log4j/2.x/manual/lookups.html#ContextMapLookup).


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    @garydgregory You are right, there is already `AdditionalField`, i'll rename it.
    There is however problem with pulling it up to `AbstractStringLayout`, since its used by many layouts where would be problematic (or impossible) to use it. Thats why I chose to implement it directly to JsonLayout.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    @mikaelstaldal Thanks, I looked for existing issue but somehow missed this one. I'll merge the branches and we'll see whether it will work for all cases. I did not know how to make it work for XML, did the ObjectMapper.withAttributes work in XML? Thanks


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mikaelstaldal <gi...@git.apache.org>.

Github user mikaelstaldal commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    @mdvorak The code looks fine to me. But we will wait a while before merging, since we plan to do a 2.9.1 bugfix release soon. This will go into 2.10.0.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    This is build log, it hangs after that.. But i'm not sure why I'm seeing this exception.
    https://gist.github.com/anonymous/9909ea92d10ac4ebe36f27a0610e7c2d


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by garydgregory <gi...@git.apache.org>.

Github user garydgregory commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    I wonder if the KeyValuePair[] should be pulled up from GelfLayout up to its superclass AbstractStringLayout. This would let other layouts like AbstractJacksonLayout use it since it also subclasses AbstractStringLayout.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by garydgregory <gi...@git.apache.org>.

Github user garydgregory commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    You're right, we do not want to say that all subclasses can or should implement extra fields.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by garydgregory <gi...@git.apache.org>.

Github user garydgregory commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    You should not change the createLayout() method which is already deprecated. Adding new features is one of the main reasons we switched to the builder pattern.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    @mikaelstaldal is there any estimation on 2.10 release date? Obviously I need this :)


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mikaelstaldal <gi...@git.apache.org>.

Github user mikaelstaldal commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    2.9.1 is now released, and the next release will (most likely) be 2.10.0 which will include this. We have no date set for that release, but historically there has been at least three months between minor releases of Log4j 2.x, and 2.9.0 was released one month ago.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mdvorak <gi...@git.apache.org>.

Github user mdvorak commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    Rolled back change of createLayout in eeebf4e, you are indeed correct about that one.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by garydgregory <gi...@git.apache.org>.

Github user garydgregory commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    Note that in the GelfLayout, the field is called "AdditionalField". It seems like a good idea to use the same name. Also, don't forget to update /src/site/xdoc/manual/layouts.xml.vm


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mikaelstaldal <gi...@git.apache.org>.

Github user mikaelstaldal commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    Linking to JIRA issue: https://issues.apache.org/jira/browse/LOG4J2-1694


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mikaelstaldal <gi...@git.apache.org>.

Github user mikaelstaldal commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    @mdvorak I think that we should not care so much about garbage and micro optimizations in the Jackson based layouts, since they are far from garbage-free or optimal anyway.
    
    Keep it as is, and don't bother with ThreadLocals.
    
    If we want to make JsonLayout garbage-free, we would have to stop using Jackson and do something similar to GelfLayout.


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by garydgregory <gi...@git.apache.org>.

Github user garydgregory commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    Hi,
    
    Thank you for the PR. Did you run 'mvn clean install' successfully?


---

[GitHub] logging-log4j2 issue #110: JsonLayout support for custom key-value pairs bei...

Posted by mikaelstaldal <gi...@git.apache.org>.

Github user mikaelstaldal commented on the issue:

    https://github.com/apache/logging-log4j2/pull/110
  
    Have a look at the unfinished branch https://github.com/apache/logging-log4j2/tree/LOG4J2-1694 you can probably pick suitable unit tests from there.


---