Skip to content

Conversation

Rassyan
Copy link
Contributor

@Rassyan Rassyan commented Jul 30, 2025

Upgrade to log4j 2.25.1
closes #132035

@Rassyan Rassyan requested a review from a team as a code owner July 30, 2025 10:17
@elasticsearchmachine elasticsearchmachine added v9.2.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jul 30, 2025
@PeteGillinElastic PeteGillinElastic added :Core/Infra/Logging Log management and logging utilities and removed needs:triage Requires assignment of a team area label labels Jul 30, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 30, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst
Copy link
Member

rjernst commented Aug 1, 2025

@elasticsearchmachine test this please

@rjernst
Copy link
Member

rjernst commented Aug 1, 2025

@elasticmachine test this please

@breskeby breskeby self-requested a review September 1, 2025 11:24
@mosche
Copy link
Contributor

mosche commented Sep 1, 2025

Looks like this is suffering from apache/logging-log4j2#3437

@mosche
Copy link
Contributor

mosche commented Sep 1, 2025

Updating this having merged #132238

@mosche
Copy link
Contributor

mosche commented Sep 2, 2025

@breskeby is looking into the transitive dependency issue mentioned above, this needs further fixes / improvements in our build to be properly handled.

@Rassyan
Copy link
Contributor Author

Rassyan commented Sep 2, 2025

Thanks for flagging this potential dependency concern.
Should any related adjustments become needed here, I'd be happy to help coordinate. Just let me know specific steps you'd like taken.

@breskeby
Copy link
Contributor

@Rassyan Can you rebase against latest master? I think some thirdparty check tasks will start failing with the update that probably need to be adjusted too. You should be able to reproduce those by running ./gradlew precommit. Let me know if you want help from our side to tackle those issues

@Rassyan
Copy link
Contributor Author

Rassyan commented Sep 15, 2025

Hi @breskeby,

Thanks for the pointer. I've rebased onto the latest main (which includes your #134169) and now encounter a dependency resolution failure when running ./gradlew precommit, even before applying my Log4j upgrade changes.

The error occurs in the :benchmarks:compileJava configuration:

* What went wrong:
Could not determine the dependencies of task ':benchmarks:compileJava'.
> Could not resolve all dependencies for configuration ':benchmarks:compileClasspath'.
   > Unexpected state for resolution: Unknown

This suggests the new component metadata rules might be conflicting with the benchmarks project's dependency graph. Could this be the kind of third-party check failure you anticipated?

I'm not sure what adjustments are needed here, as the issue seems related to the new dependency resolution setup. Could you please take a look when you have a moment?

Thanks!

@breskeby
Copy link
Contributor

breskeby commented Sep 15, 2025

can you run the build with --stacktrace? and share the output (maybe a bulid scan or share directly here). you likely need add the transitive provided dependencies now to the verification file in gradle/*.

you can do that automatically by running the precommit with ----write-verification-metadata sha256

@Rassyan

This comment was marked as outdated.

@mosche
Copy link
Contributor

mosche commented Sep 17, 2025

Unfortunately there's more issues, our use of package scanning for log4j2 plugins causes deprecation warnings that fail tests.

The use of package scanning to locate Log4j plugins is deprecated.
Please remove the deprecated `PluginManager.addPackage()` method call from `org.elasticsearch.common.logging.LogConfigurator.loadLog4jPlugins(LogConfigurator.java:157)`. 
See https://logging.apache.org/log4j/2.x/faq.html#package-scanning for details., Some custom `Core` Log4j plugins are not properly registered: 
org.elasticsearch.common.logging.RateLimitingFilter 
org.elasticsearch.common.logging.ECSJsonLayout 
org.elasticsearch.common.logging.ESJsonLayout 
org.elasticsearch.common.logging.HeaderWarningAppender 
Please consider reporting this to the maintainers of these plugins.

https://logging.apache.org/log4j/2.x/faq.html#package-scanning

I gave this a quick look trying to migrate to the annotation processor, but didn't get it to work without having to dig deeper.

diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/ElasticsearchJavaBasePlugin.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/ElasticsearchJavaBasePlugin.java
index 4cedd69d9b7..470eb283e80 100644
--- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/ElasticsearchJavaBasePlugin.java
+++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/ElasticsearchJavaBasePlugin.java
@@ -98,7 +98,7 @@ public class ElasticsearchJavaBasePlugin implements Plugin<Project> {
             // TODO Discuss moving compileOptions.getCompilerArgs() to use provider api with Gradle team.
             List<String> compilerArgs = compileOptions.getCompilerArgs();
             compilerArgs.add("-Werror");
-            compilerArgs.add("-Xlint:all,-path,-serial,-options,-deprecation,-try,-removal");
+            compilerArgs.add("-Xlint:all,-path,-serial,-options,-deprecation,-try,-removal,-processing");
             compilerArgs.add("-Xdoclint:all");
             compilerArgs.add("-Xdoclint:-missing");
             compileOptions.setEncoding("UTF-8");
diff --git a/server/build.gradle b/server/build.gradle
index 2bcd6e5322d..69b42635727 100644
--- a/server/build.gradle
+++ b/server/build.gradle
@@ -67,6 +67,10 @@ dependencies {
   api "org.apache.logging.log4j:log4j-api:${versions.log4j}"
   api "org.apache.logging.log4j:log4j-core:${versions.log4j}"

+  annotationProcessor("org.apache.logging.log4j:log4j-api:${versions.log4j}")
+  annotationProcessor("org.apache.logging.log4j:log4j-core:${versions.log4j}")
+
+
   // access to native functions
   implementation project(':libs:native')

@@ -85,6 +89,12 @@ dependencies {
   internalClusterTestImplementation(project(':modules:data-streams'))
 }

+compileJava {
+  // annotation processor to generate the Log4j2 plugin descriptors
+  options.compilerArgs << '-Alog4j.graalvm.groupId=org.elasticsearch'
+  options.compilerArgs << '-Alog4j.graalvm.artifactId=elasticsearch'
+}
+
 spotless {
   java {
     targetExclude "src/main/java/org/elasticsearch/index/IndexVersion.java"
diff --git a/server/src/main/java/org/elasticsearch/common/logging/LogConfigurator.java b/server/src/main/java/org/elasticsearch/common/logging/LogConfigurator.java
index 134f7746ba6..ebd2e4fa4c0 100644
--- a/server/src/main/java/org/elasticsearch/common/logging/LogConfigurator.java
+++ b/server/src/main/java/org/elasticsearch/common/logging/LogConfigurator.java
@@ -154,7 +154,7 @@ public class LogConfigurator {
      * Load logging plugins so we can have {@code node_name} in the pattern.
      */
     public static void loadLog4jPlugins() {
-        PluginManager.addPackage(LogConfigurator.class.getPackage().getName());
+        //PluginManager.addPackage(LogConfigurator.class.getPackage().getName());
     }

     /**

@Rassyan
Copy link
Contributor Author

Rassyan commented Sep 18, 2025

Hi @mosche,

Thanks for the detailed explanation about the Log4j2 package scanning deprecation and its impact on the tests.

I understand that the recommended long-term solution is to migrate to the annotation processor for plugin registration. However, as you noted, that requires deeper changes to the build setup.

As a near-term workaround to address the failing tests caused by the deprecation warning, I propose suppressing this specific warning message in our test framework's allowed list. This aligns with how we handle other similar Log4j warnings (like the JNDI and JMX lookup warnings).

I've tested the following change locally, and it successfully prevents the tests from failing due to this warning:

diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java
--- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java	(revision c332dca5e16cced6f0bc1918a834066059bb0545)
+++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java	(date 1758167624811)
@@ -823,7 +823,8 @@
         "JNDI lookup class is not available because this JRE does not support JNDI. "
             + "JNDI string lookups will not be available, continuing configuration.",
         "JMX runtime input lookup class is not available because this JRE does not support JMX. "
-            + "JMX lookups will not be available, continuing configuration. "
+            + "JMX lookups will not be available, continuing configuration. ",
+        "The use of package scanning to locate Log4j plugins is deprecated."
     );
 
     // separate method so that this can be checked again after suite scoped cluster is shut down
@@ -839,7 +840,8 @@
                     anyOf(
                         emptyCollectionOf(String.class),
                         contains(startsWith(LOG_4J_MSG_PREFIXES.get(0)), startsWith(LOG_4J_MSG_PREFIXES.get(1))),
-                        contains(startsWith(LOG_4J_MSG_PREFIXES.get(1)))
+                        contains(startsWith(LOG_4J_MSG_PREFIXES.get(1))),
+                        contains(startsWith(LOG_4J_MSG_PREFIXES.get(2)))
                     )
                 );
             } finally {

Given that the deprecated addPackage() method is only scheduled for removal in Log4j 3.0 (as per LOG4J2-3644), suppressing this warning in tests seems like a safe and pragmatic step for now. It unblocks development and CI while we plan the migration to the annotation processor.

Would you be open to accepting this change as a temporary fix? We can then track the migration to the annotation processor (Log4j2Plugins.dat) as a separate issue.

Let me know your thoughts.

@mosche
Copy link
Contributor

mosche commented Sep 18, 2025

Thanks @Rassyan, that's probably the best way forward without significantly increasing the scope of this!
Please commit and I'll trigger a full run of the test suite and we can check how far we can get with that.

@Rassyan
Copy link
Contributor Author

Rassyan commented Sep 18, 2025

Thanks @mosche! 🙌 Just pushed the change. Really appreciate your guidance through this - it's been a great learning experience collaborating with you. Excited to see the test results! 🚀

@mosche
Copy link
Contributor

mosche commented Sep 18, 2025

@elasticmachine test this please

@mosche
Copy link
Contributor

mosche commented Sep 18, 2025

@Rassyan could you run the test suites below, there's some more warnings.

./gradlew :qa:evil-tests:test
./gradlew :qa:logging-config:test

Specifically No Root logger was configured, creating default ERROR-level Root logger with Console appender sounds concerning. There seems to be a difference in behavior.

@Rassyan
Copy link
Contributor Author

Rassyan commented Sep 18, 2025

Hi @mosche,

I dug into EvilLoggerConfigurationTests#testLoggingLevelsFromSettings to compare behavior between Log4j 2.19.0 and 2.25.1.

In both versions, the warning “No Root logger was configured, creating default ERROR-level Root logger with Console appender” is present. The key difference is in how they handle the status listener:

This level shift in 2.19.0 breaks ESTestCase#checkStaticState, which expects WARN-level events.

I’m unsure whether we should suppress these warnings or address the listener behavior directly. Would you have any advice on how you’d like to proceed? I’m happy to help implement the right fix.

Attached:

  • Screenshot comparing WARN output in 2.19.0 vs 2.25.1
  • Screenshot showing listener level being overridden in 2.19.0
Clipboard_Screenshot_1758211239 Clipboard_Screenshot_1758211245 Clipboard_Screenshot_1758211499

@mosche
Copy link
Contributor

mosche commented Sep 18, 2025

That's a great find, thanks for digging into that!
I have a changeset pending here that ignores those additional warnings, just didn't get to investigate the one you looked into 🎉 I'll push asap.
And there's another stupid issue where log levels were set to deprecation, the old version of log4j2 was apparently more tolerant / didn't complain about it 🤷

@mosche
Copy link
Contributor

mosche commented Sep 18, 2025

@elasticmachine test this please

@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Sep 19, 2025
@mosche
Copy link
Contributor

mosche commented Sep 19, 2025

@elasticmachine test this please

@mosche
Copy link
Contributor

mosche commented Sep 19, 2025

@elasticmachine test this please

@mosche
Copy link
Contributor

mosche commented Sep 22, 2025

@elasticmachine test this please

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mosche
Copy link
Contributor

mosche commented Sep 24, 2025

@elasticmachine test this please

@mosche
Copy link
Contributor

mosche commented Sep 25, 2025

CI surfaced a log4j2 concurrency bug
apache/logging-log4j2#3940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Logging Log management and logging utilities external-contributor Pull request authored by a developer outside the Elasticsearch team serverless-linked Added by automation, don't add manually Team:Core/Infra Meta label for core/infra team >upgrade v9.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade log4j2 to newer version
8 participants