Import of LogstashLayout as JsonTemplateLayout #335
Conversation
log4j-layout-jackson-json-template/src/main/resources/JsonLayout.json
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/logging/log4j/jackson/json/template/layout/JsonTemplateLayout.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/util/Throwables.java
Show resolved
Hide resolved
.../src/main/java/org/apache/logging/log4j/jackson/json/template/layout/JsonTemplateLayout.java
Outdated
Show resolved
Hide resolved
No need to put jackson in early like that I think. The package was moved around for modularization in 3.x which makes me think we can still rename things.
You can use other formats if that makes it easier, though we may have to get creative.
You can add the tests jar as a dependency to log4j-perf.
Can likely be done as a follow up. |
👍 I will rename the package to
I am okay with AsciiDoc, my point was, shall I create a dedicated page (e.g.,
Hrm... I didn't know that was possible. I will investigate how I can do that. If you happen to feel like helping, hints are welcome.
👍 |
|
@remkop, please note the revamp in ed1dda4. The results on my machine are as follows: Either I am missing something big or G1 in JDK 11 is creating miracles. I would appreciate some insight here. |
|
I’ll take a look but it will take me some time. |
|
To add a test jar as a dependency, add |
Yep, indeed there are plenty of examples, found it, thanks. |
|
Showing only the average in ns/operation: What do these numbers mean? It is the number of nanoseconds it takes on average to serialize a log event into a StringBuilder using the What can we conclude from these numbers? The The The @vy Does that answer your question? |
|
If you wonder why the My first guess would be lock contention. With 4 threads that could be significant. I don't believe that |
Check.
The definition of what is lock-free is quite a debate on its own. But according to README, I am just baffled by the fact how come
I will experiment with smaller heap sizes, bigger |
|
To be clear, that’s just a hunch. I haven’t run the benchmarks let alone profiled them. |
|
And the following question still stands open: Given TLA results constitute the baseline, what is the pool implied maximum performance degradation we are okay with? With a rough look, MPMC performs 2.5x worse compared to TLAs on 4 cores. I will dig further... Comments are welcome. |
|
Well, what is the trade off? What benefits do we gain in return of this worse performance? |
I don’t think we’re measuring the cost of GC anywhere, are we? |
At this scale, there are so many confounding factors which can explain a difference, and you won't get an explanation without profiling the benchmark. One quite big factor at this scale is the choice of garbage collector, and not because of the time it spends collecting garbage. I just guessed (in what I thought was a private conversation) that the cost might be explained by the little bit of machine code the JIT compiler inserts into reference assignments to track inter-region references (the write barrier). In my experience, this can really show up in microbenchmarks running G1, especially with some concurrency. My advice was simply to run the benchmark with a different garbage collector and see how much it affects the numbers. Since I was tagged here, my advice would be to think twice about optimising this, and think about the future. If someone imports this library, they may well never upgrade it, but stack allocation which might be implemented in the JIT compiler would:
Applications would benefit automatically when they upgrade the JVM, but not if you optimise this now. |
|
Triggered by @richardstartin's observation, why don't we allow all 3 co-exist in the code base? After all, the call site doesn't care about the caching mechanics. We can let the user decide via a configuration knob ala |
|
Not sure I agree... What is the trade-off? What do users gain when they select something other than ThreadLocal? |
If you'd recall, we had this discussion in the mailing list in December and January. There the raised concern was TLAs, in particular, in the domain of web applications where there is a proliferation of threads and As described in this ticket, While this is a perfectly affordable footprint where the thread count matches the available CPU cores, it is not something desirable in a (web) application container. My suggestion is to leave this decision to user (granted we incorporate sane defaults) and support each strategy. Is this a slick Netty/gRPC app? Stick to TLAs. Is this an app running on Tomcat? Stick to pooling. Is this an embedded app with relatively low load? Stick to allocation. |
|
@vy Understood. That makes sense to me. |
...-template/src/main/java/org/apache/logging/log4j/jackson/json/template/layout/util/Uris.java
Outdated
Show resolved
Hide resolved
|
It's possible that there's a corresponding configuration to enable for the test jar to make it available as a dependency like in log4j-core:test-jar. |
|
@jvz, I have the cloned I will really appreciate some more help, please. |
|
I'm not really sure what else there is to configure. I can't get it working at the moment myself. |
|
For the records, I am done. The only missing bit is getting test-jar working for log4j-perf module. |
|
OK. I hope I can look at that this weekend. I am also planning on trying to get a release out this weekend so I would expect to merge the PR right after that.
Ralph
… On Feb 20, 2020, at 2:20 AM, Volkan Yazıcı ***@***.***> wrote:
For the records, I am done. The only missing bit is getting test-jar working for log4j-perf module.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#335?email_source=notifications&email_token=AAA5MX6JUO3KY7HWVRO5UOTRDZDOBA5CNFSM4KMYDMS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMMNB2A#issuecomment-588828904>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA5MXYAHF4LB4IOI7OT2XDRDZDOBANCNFSM4KMYDMSQ>.
|
|
Hi, do you have a documentation for |
|
@ramazanpolat, you can access the latest version of the manual here. Due to The changes are landed both on |
|
Thank you @vy |
|
@vy One quick question: Will e.g: Currently with using JsonLayout, this code: logger.info("This is an info message");produces something like this: {
"timeMillis" : 1598613343782,
"thread" : "main",
"level" : "INFO",
"loggerName" : "com.example.Test",
"message" : "This is an info message",
"endOfBatch" : false,
"loggerFqcn" : "org.apache.logging.log4j.spi.AbstractLogger",
"contextMap" : { },
"threadId" : 1,
"threadPriority" : 5
}But if we pass an object to the logger like this (considering the object constructor is logger.error(new MyDummyEventBean("Something bad happened", "john", "10.1.2.3");JsonLayout gets appends {
"timeMillis" : 1598613343782,
"thread" : "main",
"level" : "INFO",
"loggerName" : "com.example.Test",
"message" : "{\"message\":\"Something bad happened\",\"userId\":\"john\",\"ip\":\"10.1.2.3\"}",
"endOfBatch" : false,
"loggerFqcn" : "org.apache.logging.log4j.spi.AbstractLogger",
"contextMap" : { },
"threadId" : 1,
"threadPriority" : 5
} |
|
@ramazanpolat, yes, {
"$resolver": "message",
"stringified": false
}Further, HTH. |
|
@vy Sorry but {
"line" : 123,
"class" : "tr.gov.sgk.logclient.DAO",
"message" : "MyLogBean2{message='LogBean Message', userId='LogBean userId', ip='LogBean IP'}",
"level" : "ERROR",
"method" : "errorWithExceptionLogstash1",
"logger" : "logstashLogger"
}Maybe I'm doing something wrong. My {
"line": "${json:source:lineNumber}",
"class": "${json:source:className}",
"message": "${json:message:json}",
"level": "${json:level}",
"method": "${json:source:methodName}",
"logger": "${json:logger:name}"
}My <LogstashLayout eventTemplateUri="classpath:LogstashJsonEventLayoutV1.json"
prettyPrintEnabled="true"
locationInfoEnabled="true"
stackTraceEnabled="true" />The class I want to be printed as Json(maybe this class needs to implement an interface?): public class MyLogBean2 {
private String message;
private String userId;
private String ip;
public MyLogBean2() { }
public MyLogBean2(String message, String userId, String ip) {
this.message = message; this.userId = userId; this.ip = ip;
}
public String getMessage() { return message;}
public void setMessage(String message) {this.message = message;}
public String getUserId() {return userId;}
public void setUserId(String userId) {this.userId = userId;}
public String getIp() {return ip;}
public void setIp(String ip) {this.ip = ip;}
@Override
public String toString() {
return "MyLogBean2{" +
"message='" + message + '\'' +
", userId='" + userId + '\'' +
", ip='" + ip + '\'' +
'}';
}
} |
|
|
Hi, I was aware of this new exciting feature only after open this Jira issue: LOG4J2-2936 Please, could you tell me:
Thanks in advance and congrats for your work! |
|
Let me try to answer your questions @diepet:
Next Log4j 2.x release (2.14.0?) will contain the
Yes, it will be rendered as is. If it is |
|
Thanks @vy for the feedbacks! Ok, so let's move the discussions for the point 2 in the Jira issue. Could be possible, in the future, to plan a new I am looking forward to download the new log4j2 release! |
|
[Sorry for the late reply, I am pretty much swamped with my day job nowadays.] @diepet, I have deliberately left out blank value elimination in
If you have a use case which necessitates such a feature, I would like to hear more about it. |
|
Thanks for the feedback @diepet, keep 'em coming! |
This is the very first draft of my work on contributing log4j2-logstash-layout into Log4j core. Please note that this is still a work in progress. I want to have your feedback and support on the following issues which I will share below.
Goals
Below is the list of goals I plan to deliver in order:
UriswithLoaderUtilspattern,timeZone,localeparameters intotimestampkey level.log4j-perflog4j-layout-jackson-json-templatetolog4j-layout-json-templateI will update this list upon progress.
Module and package name
I chose
JsonTemplateLayoutas the new module name, which clearly communicates the intent. I am not totally convinced with the package nameo.a.l.log4j.jackson.json.template.layoutthough. I have my doubts about whether we can remove Jackson dependency in the future or not. This ambition does look neither feasible, nor practical in the short-term. That said, from an API perspective, why shall an internal detail likejacksonget exposed in the package name? I am considering to place it undero.a.l.log4j.layout.json.template. Any ideas?Documentation
LogstashLayouthas quite some configuration knobs which renders it inconvenient to explain it in/src/site/asciidoc/manual/layouts.adoc. I have skimmed through the source code with the hope of finding a similar documentation pattern, but failed to do so. Would somebody mind shedding some light onto the convention I should follow here, please?MapMessage#getFormattedMessage()JSON encoding bugAs reported in LOG4J2-2703,
MapMessage#getFormattedMessage()is incorrectly formattingObjects for JSON. As a workaround inLogstashLayout, I had aLogstashLayout.Builder#mapMessageFormatterIgnoredproperty, which I have dragged intoJsonTemplateLayout.Buildertoo. The property instructs the layout to ignoreMapMessage#getFormattedMessage()and do its own magic to properly encode theMapMessageinto JSON. Shall I keep this ad-hoc fix or rather resolve LOG4J2-2703 first?Performance tests
I have some test utility classes that I want to leverage in performance tests, but test classpath of
log4j-jackson-json-templatemodule is not accessible bylog4j-perfmodule. What is the work around here?What to do with
JsonLayout?I do not intend to implement
JsonLayoutby leveragingJsonTemplateLayout, at least, not now. I would rather just leave it be and get deprecated eventually. Put another way, is this PR the right place to address this issue?Missing features compared to
JsonLayoutYou might have noticed that
JsonTemplateLayoutmisses certain features (e.g.,compact,complete,header,footer, etc.) compared toJsonLayout. This is a deliberate decision of mine. If you have any objections, please chime in.Thread locals
Even though the existing TLA usage of
LogstashLayoutis dragged in, I am going to replace it with an object pool. I have spottedThreadLocalVsPoolBenchmarkand will start from there.