vy / log4j2-logstash-layout

Log4j 2.x plugin for customizable and Logstash-friendly JSON layout.
Apache License 2.0
89 stars 26 forks source link

Backporting to an earlier version of Log4j2 #26

Closed justinsaliba closed 5 years ago

justinsaliba commented 5 years ago

Hi,

Thanks for taking the time to create this project. I'm currently looking at this library to add more verbose logging throughout my applications and this seems to be exactly what I'm looking for.

I've successfully demo'd this to my peers and it was very well received. The only snag which we encountered whilst testing this with our framework is that we are stuck on v0.14 of this library. This is because the library started using ByteBufferDestinationHelper in v0.15 and in later versions. This class was only introduced to Log4j2 in v2.9.

Would you be willing to consider downgrading this library to support Log4j2 2.8.2? I'm willing to do the work myself and submit a PR in the near future to support this, but before doing so I would like to understand whether you would be willing to provide sporadic feedback, or whether you would even consider this at all.

Thanks in advance,

Justin

vy commented 5 years ago

Hey @justinsaliba! I am glad the library served (or is about to serve) a purpose for you. I prefer to stick to the latest Log4j2 release as much as possible, given Log4j2 is always backwards compatible. But I also do get your problem. I think we can find a workaround. Here is my proposal:

  1. I am gonna make sure log4j2-logstash-layout works with Log4j2 2.8.X releases, though I will use the latest Log4j2 release in pom.xml.
  2. In your project's pom.xml where you inject log4j2-logstash-layout, you can exclude the log4j2 transitive dependency dragged by log4j2-logstash-layout and let your log4j2 2.8.X kick in.

Does this sound like a viable approach to you? What do you think?

justinsaliba commented 5 years ago

Thanks for the quick reply :).

Just a quick clarification, just to make sure I'm understanding correctly.

Given that this library does not package log4j2 (due to <scope>provided</scope> in its pom), and that log4j2 is always forward compatible does it still make sense to depend on the latest version of Log4j2 if you are still looking to support v2.8.X? I've checked one my applications and its dependency tree does not list any transitive dependencies coming from this library, so there isn't anything to exclude:

...
[INFO] |  +- asm:asm-commons:jar:3.1:provided
[INFO] |  |  \- asm:asm-tree:jar:3.1:provided
[INFO] |  +- org.jgrapht:jgrapht-jdk1.5:jar:0.7.3:provided
[INFO] |  \- org.reflections:reflections:jar:0.9.9:provided
[INFO] +- com.vlkan.log4j2:log4j2-logstash-layout:jar:0.17-SNAPSHOT:compile             <---
[INFO] +- com.fasterxml.jackson.dataformat:jackson-dataformat-xml:jar:2.9.4:provided
[INFO] |  +- com.fasterxml.jackson.core:jackson-core:jar:2.9.4:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-annotations:jar:2.9.0:compile
...

I think your approach makes sense if you still want to depend on Log4j2 2.11.X (or above). I will try and gather more feedback from my team early next week to see what their thoughts are.

Have a great weekend!

Justin

vy commented 5 years ago

Oopsie! You are right. Due to the provided scope, it does not appear as a transitive dependency -- sorry, totally forgot about it. Hrm...

I gave it a quick shot in the IDE. Things that need to be fixed are as follows:

  1. Back port ByteBufferDestinationHelper -- trivial
  2. Replace StringMapMessage with MapMessage -- though I need to check the implications of this.
  3. LogstashLayoutBenchmarkState doesn't compile due to missing JsonLayout.Builder#setAdditionalFields() method. This might be the kind of issue where I need to stick to the latest Log4j2 version in the tests and benchmarks, though let the library be compatible with older Log4j2 versions.

Am I missing anything? What do you think?

Would you mind also elaborating a bit on why you can't upgrade Log4j2?

justinsaliba commented 5 years ago

Regarding 3. - yes I had thought of that when I tried to downgrade the Log4j2 version myself. Unfortunately the additionalFields property was only added in Log4j2 2.9, and is actually one of the reasons why my googling led me to this repository 😄. I didn't pay much attention to this because I was considering this library as being a replacement of JSONLayout (which comes bundled with Log4j2).

Am I missing anything? What do you think?

You might encounter some other compilation errors related to BlackHoleByteBufferDestination, probably relating to the change in interface definition between 2.8.x and 2.11.x.

Would you mind also elaborating a bit on why you can't upgrade Log4j2?

We are using an integration / API management technology (MuleSoft) which bundles its own Log4j2 version, and MuleSoft specifically support 2.8.2. Moreover, MuleSoft has an IPaaS offering where we simply upload application ZIPs, and they take care of the deployment process onto an official, preconfigured binary image of Mule. As a result, we cannot really modify the cloud binaries, nor override some of their libraries, so we have to comply with Log4j2 2.8.X.

vy commented 5 years ago

@justinsaliba, could you cherry-pick b06f9470, package it locally and see if it works for you? I was able to successfully use log4j2-logstash-layout-0.17-SNAPSHOT compiled from b06f9470, in an application employing log4j-core-2.8.2.

justinsaliba commented 5 years ago

Seems to work just fine a normal Mule app on the latest version, I'll test it out tomorrow on something that's a bit more complicated than a "hello world" API :).

Thanks for this!

justinsaliba commented 5 years ago

Hey @vy. The fix seems to be okay :). It's going to take me a couple of days to fully test this out because I would need to change some parts of my application around, but rest assured that your bit works without issues. Unfortunately I do not really have the time to do that at this point in time.

With that said, I've tried running the app using 0.16 which caused the NoClassDefFoundError. With 0.17-SNAPSHOT (using your latest commit) things were working fine and I could see the logs being generated using my own template.

Justin

vy commented 5 years ago

Put the ribbon of v0.17, should appear in Maven Central in a couple of hours.