search

prometheus / client_java

Prometheus instrumentation library for JVM applications
http://prometheus.github.io/client_java/
Apache License 2.0
2.17k stars 791 forks source link

Add log4j2 annotation processor so packages is not required #748

Open rquinio1A opened 2 years ago

rquinio1A commented 2 years ago

Currently it's necessary to pass the packages attributes via <Configuration packages="io.prometheus.client.log4j2"> in log4j configuration to discover the appender. That means relying on step 5 of https://logging.apache.org/log4j/2.x/manual/plugins.html for appender discovery (analyzing packages and annotations at startup).

It would seem less error prone and more efficient to add the log4j annotation processor to the Maven compiler plugin, so it builds the metadata META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat at build time (step1) ?

fstab commented 2 years ago

Would that be something that users of simpleclient_log4j2 need to do, or is that something that we need to do when building simpleclient_log4j2?

rquinio1A commented 2 years ago

It would need to be added in the build of simpleclient_log4j2, so that simpleclient_log4j2.jar in Maven central contains the meatadata file META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat

fstab commented 2 years ago

Thanks for the response. I didn't work with Log4j2Plugins.dat before. I'm wondering

  1. What happens if a user wants to create a uber-JAR with the Maven shade plugin, but multiple dependencies have the Log4j2Plugins.dat file? My guess is that this will fail, because the final JAR will only have one META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat and the Maven shade plugin cannot know which one to choose. That would mean that we break existing builds by adding Log4j2Plugins.dat to simpleclient_log4j2 if the existing build already uses another dependency that has a Log4j2Plugins.dat file.
  2. Even if a user does not create a uber-JAR but puts all dependencies in the classpath: Would log4j be able to load all Log4j2Plugins.dat from all dependencies? I guess if they load Log4j2Plugins.dat as a classpath resource they will just load the one from the JAR file that happens to be first in the classpath. If that's the case, it will be pretty random whether the Log4j2Plugins.dat from simpleclient_log4j2 gets picked up or not.

It would be great if you could elaborate a bit to help me understand how this will work.

rquinio1A commented 2 years ago

Good points, here's what I've found:

  1. For uber-jar we're using this transformer to merge the Log4j2Plugins.dat fields: https://github.com/edwgiz/maven-shaded-log4j-transformer. It's calling log4j-core APIs to do the merge and dump the resulting file.
  2. log4j2 itself is iterating on all the classloader resources and merging plugins in memory: https://github.com/apache/logging-log4j2/blob/f72100df0decc9bda96b4d769822c4e48b2848fc/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginRegistry.java#L162
fstab commented 2 years ago

Thanks for looking into this. So it looks like (2) is not a problem.

However, (1) sounds like this would break existing builds if users create uber-JARs but didn't configure the log4j-transformer plugin. I imagine fixing the build will be non-trivial in some cases. Many frameworks like Spring inherit the Maven shade plugin from a parent POM, and it is not straightforward where to put the log4j-transformer plugin configuration.

lily-es commented 1 year ago

Using the packages attribute is now deprecated in log4j 2. The attribute will be removed in log4j 3. This should be fixed