Migrate log4j2 to logback

vlsi commented 1 year ago

Use case

Log4j 2.x introduces unclear changes, so we might better stick with a more stable logging solution.

The current log4j2 in JMeter produces an unclear warning:

WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release
WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release
WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release
WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release

Possible solution

No response

Possible workarounds

No response

JMeter Version

5.5

Java Version

No response

OS Version

No response

jeroenhabets commented 1 year ago

FYI Changing bin/log4j2.xml to remove the packages attribute as a workaround but it does not work:

Current: <Configuration status="WARN" packages="org.apache.jmeter.gui.logging">

Tried: <Configuration status="WARN"> But got:

ERROR StatusConsoleListener Error processing element GuiLogEvent ([Appenders: null]): CLASS_NOT_FOUND
ERROR StatusConsoleListener Unable to locate appender "gui-log-event" for logger config "root"

QAInsights commented 1 year ago

Adding more context about deprecated status.

jeroenhabets commented 1 year ago

Adding more context about deprecated status.

@QAInsights "more context" link did not scroll to https://logging.apache.org/log4j/2.x/manual/configuration.html#ConfigurationSyntax for me in Firefox (due to trailing text?)

@vlsi relevant text there is:

packages - Use of the packages attribute is deprecated and will be removed in Log4j 3.0. Plugins should be processed with the Log4j annotation processor. A comma separated list of package names to search for plugins. Plugins are only loaded once per classloader so changing this value may not have any effect upon reconfiguration.

QAInsights commented 1 year ago

Yes @jeroenhabets, but in Chromium-based browsers it works fine. :)

nitinsreevstv commented 5 months ago

While running the ./jmeter, showing this.

WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release java.lang.UnsatisfiedLinkError: Can't load library: /usr/lib/jvm/java-17-openjdk-amd64/lib/libawt_xawt.so at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2398) at java.base/java.lang.Runtime.load0(Runtime.java:755) at java.base/java.lang.System.load(System.java:1957) at java.base/jdk.internal.loader.NativeLibraries.load(Native Method) at java.base/jdk.internal.loader.NativeLibraries$NativeLibraryImpl.open(NativeLibraries.java:388) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:232) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:174) at java.base/jdk.internal.loader.NativeLibraries.findFromPaths(NativeLibraries.java:315) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:285) at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2403) at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:818) at java.base/java.lang.System.loadLibrary(System.java:1993) at java.desktop/java.awt.Toolkit$2.run(Toolkit.java:1388) at java.desktop/java.awt.Toolkit$2.run(Toolkit.java:1386) at java.base/java.security.AccessController.doPrivileged(AccessController.java:318) at java.desktop/java.awt.Toolkit.loadLibraries(Toolkit.java:1385) at java.desktop/java.awt.Toolkit.initStatic(Toolkit.java:1423) at java.desktop/java.awt.Toolkit.<clinit>(Toolkit.java:1397) at java.desktop/java.awt.Component.<clinit>(Component.java:624) at java.desktop/javax.swing.ImageIcon$2.run(ImageIcon.java:145) at java.desktop/javax.swing.ImageIcon$2.run(ImageIcon.java:143) at java.base/java.security.AccessController.doPrivileged(AccessController.java:399) at java.desktop/javax.swing.ImageIcon.createNoPermsComponent(ImageIcon.java:142) at java.desktop/javax.swing.ImageIcon$1.run(ImageIcon.java:114) at java.desktop/javax.swing.ImageIcon$1.run(ImageIcon.java:111) at java.base/java.security.AccessController.doPrivileged(AccessController.java:318) at java.desktop/javax.swing.ImageIcon.<clinit>(ImageIcon.java:111) at org.apache.jmeter.plugin.PluginManager.installPlugin(PluginManager.java:59) at org.apache.jmeter.plugin.PluginManager.install(PluginManager.java:45) at org.apache.jmeter.JMeter.start(JMeter.java:487) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.apache.jmeter.NewDriver.main(NewDriver.java:259) An error occurred: Could not initialize class java.awt.Toolkit

Can someone please help me in this

Java version - 17 OS - Ubuntu 23

ppkarwasz commented 3 months ago

@jeroenhabets

FYI Changing bin/log4j2.xml to remove the packages attribute as a workaround but it does not work:

Current: <Configuration status="WARN" packages="org.apache.jmeter.gui.logging">

Tried: <Configuration status="WARN"> But got:
ERROR StatusConsoleListener Error processing element GuiLogEvent ([Appenders: null]): CLASS_NOT_FOUND
ERROR StatusConsoleListener Unable to locate appender "gui-log-event" for logger config "root"

To register additional Log4j Core plugins a small additional step is necessary: the appender class must be compile with the annotation processor contained in log4j-core (see documentation). This is usually straightforward (the annotation processor path is by default the classpath, which must already contain log4j-core), but in your case log4j-core must be explicitly added to the kapt configuration.

The annotationProcessor configuration does not work since the kotlin Gradle plugin disables annotation processing in the compileJava task.

@vlsi,

Log4j 2.x introduces unclear changes, so we might better stick with a more stable logging solution.

The current log4j2 in JMeter produces an unclear warning:
WARN StatusConsoleListener The use of package scanning to locate plugins is deprecated and will be removed in a future release

It seems to me that the essence of the problem is the clarity of the warning message. Can you submit a PR to the logging-log4j2 repository that replaces the message with a better one?

vlsi commented 3 months ago

It seems to me that the essence of the problem is the clarity of the warning message

The essence is that the end-users can't fix the issue at their end. In other words, JMeter application emits a warning, and the end-users have no power to fix it.

linghengqian commented 3 months ago

It seems to me that the essence of the problem is the clarity of the warning message

The essence is that the end-users can't fix the issue at their end. In other words, JMeter application emits a warning, and the end-users have no power to fix it.

This warning is actually introduced by #5859 . One of my observations is that apache/jmeter does not assert log output in unit tests like apache/hive.
Even if #6326 is not merged, downstream users can simply switch revert #5859 and recompile the apache/jmeter product to turn off this warning.

ppkarwasz commented 3 months ago

It seems to me that the essence of the problem is the clarity of the warning message

The essence is that the end-users can't fix the issue at their end. In other words, JMeter application emits a warning, and the end-users have no power to fix it.

The default level of status logger is ERROR, but JMeter modifies it to a more sound WARN:

https://github.com/apache/jmeter/blob/174ba3178f287d2984b1e53004e2c2cee1ffff16/bin/log4j2.xml#L19

Users can remove the status attribute and fall back to the default behavior.

linghengqian commented 3 months ago

Since https://github.com/apache/jmeter/commit/cbdfd1ba435ad92b32c6538a5a433ac7dff9e024 has entered the master branch, it seems that there is no point in opening the current issue.

vlsi commented 3 months ago

Users can remove the status attribute and fall back to the default behavior.

Let me try again: the warning talks to wrong people. JMeter application users can't fix their JMeter configuration so it adheres to the new best practices. Of course they can silence the warning, however, it does not fix it. The only way to truly fix the warning is to change JMeter sources, so the warning should talk to JMeter devs rather than JMeter users.

downstream users can simply switch revert https://github.com/apache/jmeter/pull/5859

Unfortunately, log4j2 does not back-patch security fixes. In other words, if we stay on log4j 2.17.x (the version before #5859), then we would have to upgrade anyway if a new CVE is discovered. I wish they reconsider and start patching the security issues in old releases.

vlsi commented 3 months ago

Since https://github.com/apache/jmeter/commit/cbdfd1ba435ad92b32c6538a5a433ac7dff9e024 has entered the master branch, it seems that there is no point in opening the current issue.

I would still like to migrate off log4j2

ppkarwasz commented 3 months ago

Users can remove the status attribute and fall back to the default behavior.

Let me try again: the warning talks to wrong people. JMeter application users can't fix their JMeter configuration so it adheres to the new best practices. Of course they can silence the warning, however, it does not fix it. The only way to truly fix the warning is to change JMeter sources, so the warning should talk to JMeter devs rather than JMeter users.

I agree with you, but how do you suggest to do it? It's a chicken-and-egg problem: JMeter didn't run the annotation processor, so the annotation processor could not issue errors or warnings.

Unfortunately, log4j2 does not back-patch security fixes. In other words, if we stay on log4j 2.17.x (the version before #5859), then we would have to upgrade anyway if a new CVE is discovered. I wish they reconsider and start patching the security issues in old releases.

Log4j and Logback have different versioning schemes: Log4j uses semantic versioning, so users can always upgrade to the last version of the supported major versions (2.x and soon 3.x) without breaking changes. In special situations (e.g. Java 6 and Java 7 users) also some minor versions (2.3.x and 2.12.x) receive security updates.

Logback has a custom versioning scheme, where new features and breaking changes can occur in each version.

I would still like to migrate off log4j2

For JPMS reasons you should probably move the GuiLogEventAppender out of ApacheJMeter_core into a separate artifact: a library should not have dependencies on a logging implementation, just the API. See apache/zookeeper#2155 and apache/eventmesh#4719 for example.

vlsi commented 3 months ago

I agree with you, but how do you suggest to do it?

a. Avoid printing the warnings. For instance, if you absolutely want de-support package scanning, could do so and mention it in the migration guide. b. You could raise an error when user configures "unsupported package attribute in log4j 3". In other words, 2.x might be silent, and 3.x might throw an error if you absolutely want de-support the feature. c. If you absolutely want printing a warning, then consider phrasing it in such a way the users could make a reasonable action.

For instance, consider what OpenJDK does with "illegal reflective access"

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.inject.internal.cglib.core.$ReflectUtils$1 (file:/C:/.../.m2/repository/com/google/inject/guice/4.2.2/guice-4.2.2.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.google.inject.internal.cglib.core.$ReflectUtils$1
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

They have "Please consider reporting this to the maintainers of com.google.inject.internal.cglib.core.$ReflectUtils$1". Of course, it is not as good as JMeter build-time warning, but having something like "please consider reporting it to the maintainers of org.apache.jmeter.gui.logging.GuiLogEventAppender" would be better than the current log4j message.

so users can always upgrade to the last version of the supported major versions (2.x and soon 3.x) without breaking changes.

When patching a security issue, everybody wants just the security fix, and they do not want "an extra year worth of features and bugfixes".

There are cases when upgrading minor versions is prohibited. For instance, Spring Boot policy is to keep using the same minor versions for the third-party dependencies: https://github.com/pgjdbc/pgjdbc/issues/2599#issuecomment-1222423561

That is one of the reasons pgjdbc/pgjdbc patches security issues in all minor versions by default (see https://github.com/pgjdbc/pgjdbc/security/advisories/GHSA-24rp-q3w6-vc56), and pgjdbc team can roll a security fix to literally any version on demand (see https://github.com/pgjdbc/pgjdbc/security/policy).

Once again: I'm not against upgrading to a recent version from time to time. However I would like to have a version that has just a necessary changes to fix the CVE when in a hurry of patching a CVE. I do not like the way log4j2 team handles CVEs by forcing everybody upgrading to the latest minor.

For JPMS reasons you should probably move the GuiLogEventAppender out of ApacheJMeter_core into a separate artifact

That is so true

vlsi commented 3 months ago

For JPMS reasons you should probably move the GuiLogEventAppender out of ApacheJMeter_core into a separate artifact

However, ApacheJMeter_core would still have to depend on the new artifact for the backward compatibility reasons so extracting GuiLogEventAppender does not seem to change much.

apache / jmeter