Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate log4j2 to logback #5937

Open
vlsi opened this issue May 19, 2023 · 15 comments
Open

Migrate log4j2 to logback #5937

vlsi opened this issue May 19, 2023 · 15 comments

Comments

@vlsi
Copy link
Collaborator

vlsi commented May 19, 2023

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

@vlsi vlsi added this to the 6.0 milestone May 19, 2023
@jeroenhabets
Copy link

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
Copy link
Contributor

Adding more context about deprecated status.

@jeroenhabets
Copy link

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
Copy link
Contributor

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

@nitinsreevstv
Copy link

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 added a commit to ppkarwasz/jmeter that referenced this issue Aug 4, 2024
The Log4j Core `packages` configuration attribute is deprecated because:

* It triggers the scanning of the mentioned packages and slows down the
  startup process.
* It was replaced in version 2.0 by a faster mechanism that relies on a
  `Log4jPlugins.dat` metadata file. See [Log4j Plugins](https://logging.apache.org/log4j/2.x/manual/plugins.html) for more details.

This PR removes the `packages` attribute from the standard configuration
file and configures the build script of `jmeter-core` to use the
`PluginProcessor` contained in `log4j-core`.

Fixes apache#5937.
@ppkarwasz
Copy link
Contributor

@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
Copy link
Collaborator Author

vlsi commented Aug 6, 2024

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
Copy link
Member

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.

@ppkarwasz
Copy link
Contributor

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:

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

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

vlsi pushed a commit to ppkarwasz/jmeter that referenced this issue Aug 6, 2024
…er The use of package scanning to locate plugins is deprecated ...

The Log4j Core `packages` configuration attribute is deprecated because:

* It triggers the scanning of the mentioned packages and slows down the
  startup process.
* It was replaced in version 2.0 by a faster mechanism that relies on a
  `Log4jPlugins.dat` metadata file. See [Log4j Plugins](https://logging.apache.org/log4j/2.x/manual/plugins.html) for more details.

This PR removes the `packages` attribute from the standard configuration
file and configures the build script of `jmeter-core` to use the
`PluginProcessor` contained in `log4j-core`.

See apache#5937.
vlsi pushed a commit to ppkarwasz/jmeter that referenced this issue Aug 6, 2024
…er The use of package scanning to locate plugins is deprecated ...

The Log4j Core `packages` configuration attribute is deprecated because:

* It triggers the scanning of the mentioned packages and slows down the
  startup process.
* It was replaced in version 2.0 by a faster mechanism that relies on a
  `Log4jPlugins.dat` metadata file. See [Log4j Plugins](https://logging.apache.org/log4j/2.x/manual/plugins.html) for more details.

This PR removes the `packages` attribute from the standard configuration
file and configures the build script of `jmeter-core` to use the
`PluginProcessor` contained in `log4j-core`.

See apache#5937.
@linghengqian
Copy link
Member

Since cbdfd1b has entered the master branch, it seems that there is no point in opening the current issue.

@vlsi
Copy link
Collaborator Author

vlsi commented Aug 6, 2024

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 #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
Copy link
Collaborator Author

vlsi commented Aug 6, 2024

Since cbdfd1b 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
Copy link
Contributor

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
Copy link
Collaborator Author

vlsi commented Aug 6, 2024

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: pgjdbc/pgjdbc#2599 (comment)

That is one of the reasons pgjdbc/pgjdbc patches security issues in all minor versions by default (see 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
Copy link
Collaborator Author

vlsi commented Aug 6, 2024

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.

ppkarwasz added a commit to apache/logging-log4j2 that referenced this issue Aug 13, 2024
This improves status logger warnings about:

* missing `Log4j2Plugin.dat` plugin descriptors,
* usage of package scanning.

The warnings are reworded in a way that they are more useful to end-users.
If package scanning is enabled, linkage errors (almost exclusively due to optional dependencies) are logged at a `DEBUG` level instead of `WARN`.

This addresses @vlsi suggestions from
apache/jmeter#5937 (comment) and fixes #2835.
ppkarwasz added a commit to apache/logging-log4j2 that referenced this issue Aug 13, 2024
This improves status logger warnings about:

* missing `Log4j2Plugin.dat` plugin descriptors,
* usage of package scanning.

The warnings are reworded in a way that they are more useful to end-users.
If package scanning is enabled, linkage errors (almost exclusively due to optional dependencies) are logged at a `DEBUG` level instead of `WARN`.

The `PluginManager` tests are revamped to:

- Look for a status logger warning if a deprecated plugin scanning feature is used.
- Look for a status logger warning if no plugin descriptor is available.

This addresses @vlsi suggestions from
apache/jmeter#5937 (comment) and fixes #2835.

Co-authored-by: Volkan Yazıcı <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants