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

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

Closed
wants to merge 7 commits into from

Conversation

@mdvorak
Copy link
Contributor

mdvorak commented Sep 12, 2017

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 LogEventWithAdditionalFields 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:

<JsonLayout>
  <KeyValuePair key="hostName" value="${hostName}"/>
</JsonLayout>

Inserts into each generated JSON hostName key:

{
"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:

public static class LogEventWithAdditionalFields {
    // ....
    @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> getAdditionalFields() { return additionalFields; }
}
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
@garydgregory

This comment has been minimized.

Copy link
Contributor

garydgregory commented Sep 12, 2017

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.

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 12, 2017

@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.

@garydgregory

This comment has been minimized.

Copy link
Contributor

garydgregory commented Sep 12, 2017

You're right, we do not want to say that all subclasses can or should implement extra fields.

@garydgregory

This comment has been minimized.

Copy link
Contributor

garydgregory commented Sep 12, 2017

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

@garydgregory

This comment has been minimized.

Copy link
Contributor

garydgregory commented Sep 12, 2017

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.

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 12, 2017

Rolled back change of createLayout in eeebf4e, you are indeed correct about that one.

@mikaelstaldal

This comment has been minimized.

Copy link
Contributor

mikaelstaldal commented Sep 12, 2017

@mikaelstaldal

This comment has been minimized.

Copy link
Contributor

mikaelstaldal commented Sep 12, 2017

It would be nice to add this to XmlLayout and YamlLayout also.

@mikaelstaldal

This comment has been minimized.

Copy link
Contributor

mikaelstaldal commented Sep 12, 2017

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.

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 13, 2017

@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

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 13, 2017

@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.

@mdvorak mdvorak force-pushed the mdvorak:jsonlayout-extras29 branch from 9236224 to d16ae65 Sep 13, 2017
Added test from LOF4J2-1694
@mdvorak mdvorak force-pushed the mdvorak:jsonlayout-extras29 branch from 732f2c2 to 31affd9 Sep 13, 2017
(cherry picked from commit fc50939)
@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 13, 2017

@mikaelstaldal I've merged your tests from fc50939, thanks for that! That saved some time.

Anyway, I've pulled up my changes in JsonLayout to AbstractJacksonLayout - annotations, despite being named @Json* works both for XML and YAML as well.

Please take a look, it looks promising, hopefully it won't break anything (I don't fully trust the @JsonUnwrapped annotation, but seems to be working atm).

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 13, 2017

@garydgregory

This comment has been minimized.

Copy link
Contributor

garydgregory commented Sep 13, 2017

Hi,

Thank you for the PR. Did you run 'mvn clean install' successfully?

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 14, 2017

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.

@mikaelstaldal

This comment has been minimized.

Copy link
Contributor

mikaelstaldal commented Sep 14, 2017

@mdvorak Resolving ${} expressions during serialization is necessary to support dynamic lookups which depend on the current log event, such as ContextMapLookup.

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 14, 2017

@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 ThreadLocals, 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?

@mikaelstaldal

This comment has been minimized.

Copy link
Contributor

mikaelstaldal commented Sep 14, 2017

@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.

@mikaelstaldal

This comment has been minimized.

Copy link
Contributor

mikaelstaldal commented Sep 14, 2017

@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.

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 15, 2017

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

@mdvorak

This comment has been minimized.

Copy link
Contributor Author

mdvorak commented Sep 20, 2017

@mikaelstaldal is there any estimation on 2.10 release date? Obviously I need this :) Thanks

@garydgregory

This comment has been minimized.

Copy link
Contributor

garydgregory commented Sep 20, 2017

We are in the process of releasing 2.9.1, anything beyond that does not have a hard set date yet.

@mikaelstaldal

This comment has been minimized.

Copy link
Contributor

mikaelstaldal commented Sep 26, 2017

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.

@asfgit asfgit closed this in 474bef4 Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.