vy opened a new pull request #335: Import of LogstashLayout as JsonTemplateLayout URL: https://github.com/apache/logging-log4j2/pull/335 This is the very first draft of my work on contributing [log4j2-logstash-layout](/vy/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. ## Module and package name I chose `JsonTemplateLayout` as the new module name, which clearly communicates the intent. I am not totally convinced with the package name `o.a.l.log4j.jackson.json.template.layout` though. 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 like `jackson` get exposed in the package name? I am considering to place it under `o.a.l.log4j.layout.json.template`. Any ideas? ## Documentation `LogstashLayout` has 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 bug As reported in [LOG4J2-2703](https://issues.apache.org/jira/browse/LOG4J2-2703), `MapMessage#getFormattedMessage()` is incorrectly formatting `Object`s for JSON. As a workaround in `LogstashLayout`, I had a `LogstashLayout.Builder#mapMessageFormatterIgnored` property, which I have dragged into `JsonTemplateLayout.Builder` too. The property instructs the layout to ignore `MapMessage#getFormattedMessage()` and do its own magic to properly encode the `MapMessage` into 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-template` module is not accessible by `log4j-perf` module. What is the work around here? ## What to do with `JsonLayout`? I do not intend to implement `JsonLayout` by leveraging `JsonTemplateLayout`, 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 `JsonLayout` You might have noticed that `JsonTemplateLayout` misses certain features (e.g., `compact`, `complete`, `header`, `footer`, etc.) compared to `JsonLayout`. This is a deliberate decision of mine. If you have any objections, please chime in. ## Thread locals Even though the existing TLA usage of `LogstashLayout` is dragged in, I am going to replace it with an object pool. I have spotted `ThreadLocalVsPoolBenchmark` and will start from there.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
