From c5ab5be879a41ffca04acf5f12515fe612404421 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 28 Feb 2025 09:47:04 -0800 Subject: [PATCH 1/7] Set root logger level for CLIs All CLIs in elasticsearch support command line flags for controlling the output level. When --silent is used, the expectation is that normal logging is omitted. Yet the log4j logger is still configured to output error level logs. This commit sets the appropriate log level for log4j depending on the Terminal log level. --- libs/cli/build.gradle | 1 + libs/cli/src/main/java/module-info.java | 2 ++ libs/cli/src/main/java/org/elasticsearch/cli/Command.java | 7 +++++++ libs/logging/src/main/java/module-info.java | 2 +- .../elasticsearch/logging/internal/spi/LoggerFactory.java | 3 +++ .../common/logging/internal/LoggerFactoryImpl.java | 8 ++++++++ x-pack/plugin/security/src/main/java/module-info.java | 1 + 7 files changed, 23 insertions(+), 1 deletion(-) diff --git a/libs/cli/build.gradle b/libs/cli/build.gradle index d5842d4a2c59c..d5b1bd6ac648e 100644 --- a/libs/cli/build.gradle +++ b/libs/cli/build.gradle @@ -12,6 +12,7 @@ apply plugin: 'elasticsearch.publish' dependencies { api 'net.sf.jopt-simple:jopt-simple:5.0.2' api project(':libs:core') + api project(':libs:logging') testImplementation(project(":test:framework")) { exclude group: 'org.elasticsearch', module: 'cli' diff --git a/libs/cli/src/main/java/module-info.java b/libs/cli/src/main/java/module-info.java index e3969a1c74375..eeaf2eae06b96 100644 --- a/libs/cli/src/main/java/module-info.java +++ b/libs/cli/src/main/java/module-info.java @@ -11,6 +11,8 @@ module org.elasticsearch.cli { requires jopt.simple; requires org.elasticsearch.base; + requires java.logging; + requires org.elasticsearch.logging; exports org.elasticsearch.cli; } diff --git a/libs/cli/src/main/java/org/elasticsearch/cli/Command.java b/libs/cli/src/main/java/org/elasticsearch/cli/Command.java index 6d38408ed165a..493fa4c3d462a 100644 --- a/libs/cli/src/main/java/org/elasticsearch/cli/Command.java +++ b/libs/cli/src/main/java/org/elasticsearch/cli/Command.java @@ -15,11 +15,14 @@ import joptsimple.OptionSpec; import org.elasticsearch.core.SuppressForbidden; +import org.elasticsearch.logging.Level; +import org.elasticsearch.logging.internal.spi.LoggerFactory; import java.io.Closeable; import java.io.IOException; import java.io.StringWriter; import java.util.Arrays; +import java.util.logging.LogManager; /** * An action to execute within a cli. @@ -84,12 +87,16 @@ protected void mainWithoutErrorHandling(String[] args, Terminal terminal, Proces return; } + LoggerFactory loggerFactory = LoggerFactory.provider(); if (options.has(silentOption)) { terminal.setVerbosity(Terminal.Verbosity.SILENT); + loggerFactory.setRootLevel(Level.OFF); } else if (options.has(verboseOption)) { terminal.setVerbosity(Terminal.Verbosity.VERBOSE); + loggerFactory.setRootLevel(Level.DEBUG); } else { terminal.setVerbosity(Terminal.Verbosity.NORMAL); + loggerFactory.setRootLevel(Level.INFO); } execute(terminal, options, processInfo); diff --git a/libs/logging/src/main/java/module-info.java b/libs/logging/src/main/java/module-info.java index 3ff21cefd14df..8c9ebbd91e869 100644 --- a/libs/logging/src/main/java/module-info.java +++ b/libs/logging/src/main/java/module-info.java @@ -9,5 +9,5 @@ module org.elasticsearch.logging { exports org.elasticsearch.logging; - exports org.elasticsearch.logging.internal.spi to org.elasticsearch.server; + exports org.elasticsearch.logging.internal.spi to org.elasticsearch.server, org.elasticsearch.cli; } diff --git a/libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java b/libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java index d5db919197307..ec7e7371871ef 100644 --- a/libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java +++ b/libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java @@ -9,6 +9,7 @@ package org.elasticsearch.logging.internal.spi; +import org.elasticsearch.logging.Level; import org.elasticsearch.logging.Logger; /** @@ -26,6 +27,8 @@ public static LoggerFactory provider() { public abstract Logger getLogger(Class clazz); + public abstract void setRootLevel(Level level); + public static void setInstance(LoggerFactory INSTANCE) { LoggerFactory.INSTANCE = INSTANCE; } diff --git a/server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java b/server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java index e8354be5ea225..b0a5cf4b76449 100644 --- a/server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java +++ b/server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java @@ -10,6 +10,8 @@ package org.elasticsearch.common.logging.internal; import org.apache.logging.log4j.LogManager; +import org.elasticsearch.common.logging.Loggers; +import org.elasticsearch.logging.Level; import org.elasticsearch.logging.Logger; import org.elasticsearch.logging.internal.spi.LoggerFactory; @@ -30,4 +32,10 @@ public Logger getLogger(Class clazz) { // classloader a class comes from, we will use the root logging config. return getLogger(clazz.getName()); } + + @Override + public void setRootLevel(Level level) { + var log4jLevel = LevelUtil.log4jLevel(level); + Loggers.setLevel(LogManager.getRootLogger(), log4jLevel); + } } diff --git a/x-pack/plugin/security/src/main/java/module-info.java b/x-pack/plugin/security/src/main/java/module-info.java index a26e93633d30f..4820fd4ad6096 100644 --- a/x-pack/plugin/security/src/main/java/module-info.java +++ b/x-pack/plugin/security/src/main/java/module-info.java @@ -49,6 +49,7 @@ requires oauth2.oidc.sdk; requires org.slf4j; requires unboundid.ldapsdk; + requires org.elasticsearch.logging; exports org.elasticsearch.xpack.security.action to org.elasticsearch.server; exports org.elasticsearch.xpack.security.action.apikey to org.elasticsearch.server; From 90155d11a01b3000178467e3def1af54dc24df8c Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Fri, 28 Feb 2025 18:02:27 +0000 Subject: [PATCH 2/7] [CI] Auto commit changes from spotless --- libs/cli/src/main/java/org/elasticsearch/cli/Command.java | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/cli/src/main/java/org/elasticsearch/cli/Command.java b/libs/cli/src/main/java/org/elasticsearch/cli/Command.java index 493fa4c3d462a..1690515532e7b 100644 --- a/libs/cli/src/main/java/org/elasticsearch/cli/Command.java +++ b/libs/cli/src/main/java/org/elasticsearch/cli/Command.java @@ -22,7 +22,6 @@ import java.io.IOException; import java.io.StringWriter; import java.util.Arrays; -import java.util.logging.LogManager; /** * An action to execute within a cli. From 6099237f7648d568c2e4558a753964905617274c Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 28 Feb 2025 10:53:38 -0800 Subject: [PATCH 3/7] iter --- .../logging/internal/spi/LoggerFactory.java | 2 + .../common/logging/internal/LevelUtil.java | 39 +++++++++++++++++-- .../logging/internal/LoggerFactoryImpl.java | 5 +++ .../elasticsearch/cli/CommandTestCase.java | 13 +++++++ 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java b/libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java index ec7e7371871ef..a485a78a6e63f 100644 --- a/libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java +++ b/libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java @@ -29,6 +29,8 @@ public static LoggerFactory provider() { public abstract void setRootLevel(Level level); + public abstract Level getRootLevel(); + public static void setInstance(LoggerFactory INSTANCE) { LoggerFactory.INSTANCE = INSTANCE; } diff --git a/server/src/main/java/org/elasticsearch/common/logging/internal/LevelUtil.java b/server/src/main/java/org/elasticsearch/common/logging/internal/LevelUtil.java index df939dd0f2d21..c1d6a80ee4618 100644 --- a/server/src/main/java/org/elasticsearch/common/logging/internal/LevelUtil.java +++ b/server/src/main/java/org/elasticsearch/common/logging/internal/LevelUtil.java @@ -9,20 +9,51 @@ package org.elasticsearch.common.logging.internal; +import static org.apache.logging.log4j.Level.ALL; +import static org.apache.logging.log4j.Level.DEBUG; +import static org.apache.logging.log4j.Level.ERROR; +import static org.apache.logging.log4j.Level.FATAL; +import static org.apache.logging.log4j.Level.INFO; +import static org.apache.logging.log4j.Level.OFF; +import static org.apache.logging.log4j.Level.TRACE; +import static org.apache.logging.log4j.Level.WARN; + public final class LevelUtil { private LevelUtil() {} public static org.apache.logging.log4j.Level log4jLevel(final org.elasticsearch.logging.Level level) { return switch (level) { - case OFF -> org.apache.logging.log4j.Level.OFF; - case FATAL -> org.apache.logging.log4j.Level.FATAL; + case OFF -> OFF; + case FATAL -> FATAL; case ERROR -> org.apache.logging.log4j.Level.ERROR; - case WARN -> org.apache.logging.log4j.Level.WARN; + case WARN -> WARN; case INFO -> org.apache.logging.log4j.Level.INFO; case DEBUG -> org.apache.logging.log4j.Level.DEBUG; - case TRACE -> org.apache.logging.log4j.Level.TRACE; + case TRACE -> TRACE; case ALL -> org.apache.logging.log4j.Level.ALL; }; } + + public static org.elasticsearch.logging.Level elasticsearchLevel(final org.apache.logging.log4j.Level log4jLevel) { + // we can't use a switch because log4j levels are not an enum + if (log4jLevel == OFF) { + return org.elasticsearch.logging.Level.OFF; + } else if (log4jLevel == FATAL) { + return org.elasticsearch.logging.Level.FATAL; + } else if (log4jLevel == ERROR) { + return org.elasticsearch.logging.Level.ERROR; + } else if (log4jLevel == WARN) { + return org.elasticsearch.logging.Level.WARN; + } else if (log4jLevel == INFO) { + return org.elasticsearch.logging.Level.INFO; + } else if (log4jLevel == DEBUG) { + return org.elasticsearch.logging.Level.DEBUG; + } else if (log4jLevel == TRACE) { + return org.elasticsearch.logging.Level.TRACE; + } else if (log4jLevel == ALL) { + return org.elasticsearch.logging.Level.ALL; + } + throw new AssertionError("unknown log4j level [" + log4jLevel + "]"); + } } diff --git a/server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java b/server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java index b0a5cf4b76449..393a94125da60 100644 --- a/server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java +++ b/server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java @@ -38,4 +38,9 @@ public void setRootLevel(Level level) { var log4jLevel = LevelUtil.log4jLevel(level); Loggers.setLevel(LogManager.getRootLogger(), log4jLevel); } + + @Override + public Level getRootLevel() { + return LevelUtil.elasticsearchLevel(LogManager.getRootLogger().getLevel()); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java b/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java index 406454bf72dd8..744584acaef39 100644 --- a/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java @@ -9,8 +9,13 @@ package org.elasticsearch.cli; +import org.elasticsearch.common.logging.internal.LevelUtil; +import org.elasticsearch.logging.Level; +import org.elasticsearch.logging.LogManager; +import org.elasticsearch.logging.internal.spi.LoggerFactory; import org.elasticsearch.test.ESTestCase; import org.hamcrest.Matcher; +import org.junit.After; import org.junit.Before; import java.io.IOException; @@ -43,6 +48,8 @@ public abstract class CommandTestCase extends ESTestCase { /** The ES config dir */ protected Path configDir; + private Level capturedLogLevel; + /** Whether to include a whitespace in the file-system path. */ private final boolean spaceInPath; @@ -56,6 +63,7 @@ protected CommandTestCase(boolean spaceInPath) { @Before public void resetTerminal() throws IOException { + capturedLogLevel = LoggerFactory.provider().getRootLevel(); terminal.reset(); terminal.setSupportsBinary(false); terminal.setVerbosity(Terminal.Verbosity.NORMAL); @@ -73,6 +81,11 @@ public void resetTerminal() throws IOException { envVars.clear(); } + @After + public void restoreRootLogLevel() { + LoggerFactory.provider().setRootLevel(capturedLogLevel); + } + /** Creates a Command to test execution. */ protected abstract Command newCommand(); From b55a479500726edaac882854f883240ac1ce139b Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Fri, 28 Feb 2025 19:02:15 +0000 Subject: [PATCH 4/7] [CI] Auto commit changes from spotless --- .../src/main/java/org/elasticsearch/cli/CommandTestCase.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java b/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java index 744584acaef39..2a5be0cb7dce2 100644 --- a/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java @@ -9,9 +9,7 @@ package org.elasticsearch.cli; -import org.elasticsearch.common.logging.internal.LevelUtil; import org.elasticsearch.logging.Level; -import org.elasticsearch.logging.LogManager; import org.elasticsearch.logging.internal.spi.LoggerFactory; import org.elasticsearch.test.ESTestCase; import org.hamcrest.Matcher; From 2a9144a1017b9e19f296cc21571ee1d6359a0643 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 28 Feb 2025 14:54:40 -0800 Subject: [PATCH 5/7] restore in ESTestCase instead --- .../java/org/elasticsearch/cli/CommandTestCase.java | 11 ----------- .../main/java/org/elasticsearch/test/ESTestCase.java | 4 ++++ 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java b/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java index 2a5be0cb7dce2..406454bf72dd8 100644 --- a/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java @@ -9,11 +9,8 @@ package org.elasticsearch.cli; -import org.elasticsearch.logging.Level; -import org.elasticsearch.logging.internal.spi.LoggerFactory; import org.elasticsearch.test.ESTestCase; import org.hamcrest.Matcher; -import org.junit.After; import org.junit.Before; import java.io.IOException; @@ -46,8 +43,6 @@ public abstract class CommandTestCase extends ESTestCase { /** The ES config dir */ protected Path configDir; - private Level capturedLogLevel; - /** Whether to include a whitespace in the file-system path. */ private final boolean spaceInPath; @@ -61,7 +56,6 @@ protected CommandTestCase(boolean spaceInPath) { @Before public void resetTerminal() throws IOException { - capturedLogLevel = LoggerFactory.provider().getRootLevel(); terminal.reset(); terminal.setSupportsBinary(false); terminal.setVerbosity(Terminal.Verbosity.NORMAL); @@ -79,11 +73,6 @@ public void resetTerminal() throws IOException { envVars.clear(); } - @After - public void restoreRootLogLevel() { - LoggerFactory.provider().setRootLevel(capturedLogLevel); - } - /** Creates a Command to test execution. */ protected abstract Command newCommand(); diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index de787f909cd8e..181ad2ba39fa4 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -121,6 +121,7 @@ import org.elasticsearch.indices.IndicesModule; import org.elasticsearch.indices.analysis.AnalysisModule; import org.elasticsearch.jdk.RuntimeVersionFeature; +import org.elasticsearch.logging.internal.spi.LoggerFactory; import org.elasticsearch.plugins.AnalysisPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.plugins.scanners.StablePluginsRegistry; @@ -263,6 +264,7 @@ public abstract class ESTestCase extends LuceneTestCase { private static final Collection loggedLeaks = new ArrayList<>(); private HeaderWarningAppender headerWarningAppender; + private org.elasticsearch.logging.Level capturedLogLevel; @AfterClass public static void resetPortCounter() { @@ -584,6 +586,7 @@ public final void before() { this.threadContext = new ThreadContext(Settings.EMPTY); HeaderWarning.setThreadContext(threadContext); } + capturedLogLevel = LoggerFactory.provider().getRootLevel(); } private static final List breakers = Collections.synchronizedList(new ArrayList<>()); @@ -618,6 +621,7 @@ protected boolean enableBigArraysReleasedCheck() { @After public final void after() throws Exception { + LoggerFactory.provider().setRootLevel(capturedLogLevel); if (enableBigArraysReleasedCheck()) { MockBigArrays.ensureAllArraysAreReleased(); } From 5c813f063c1fadf268623044f536c8b5a4886023 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 28 Feb 2025 16:46:37 -0800 Subject: [PATCH 6/7] try again --- .../org/elasticsearch/test/ESTestCase.java | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index 181ad2ba39fa4..cba2ba620df25 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -264,7 +264,6 @@ public abstract class ESTestCase extends LuceneTestCase { private static final Collection loggedLeaks = new ArrayList<>(); private HeaderWarningAppender headerWarningAppender; - private org.elasticsearch.logging.Level capturedLogLevel; @AfterClass public static void resetPortCounter() { @@ -577,6 +576,22 @@ public void removeHeaderWarningAppender() { } } + private org.elasticsearch.logging.Level capturedLogLevel = null; + + @Before + public void captureLoggingLevel() { + capturedLogLevel = LoggerFactory.provider().getRootLevel(); + } + + @After + public void restoreLoggingLevel() { + if (capturedLogLevel != null) { + // log level might not have been captured if test was skipped + LoggerFactory.provider().setRootLevel(capturedLogLevel); + capturedLogLevel = null; + } + } + @Before public final void before() { LeakTracker.setContextHint(getTestName()); @@ -586,7 +601,6 @@ public final void before() { this.threadContext = new ThreadContext(Settings.EMPTY); HeaderWarning.setThreadContext(threadContext); } - capturedLogLevel = LoggerFactory.provider().getRootLevel(); } private static final List breakers = Collections.synchronizedList(new ArrayList<>()); @@ -621,7 +635,6 @@ protected boolean enableBigArraysReleasedCheck() { @After public final void after() throws Exception { - LoggerFactory.provider().setRootLevel(capturedLogLevel); if (enableBigArraysReleasedCheck()) { MockBigArrays.ensureAllArraysAreReleased(); } From 4e32d246f1ef63ff9d6d041ce73ead1ed62bfde6 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Sun, 2 Mar 2025 08:45:58 -0800 Subject: [PATCH 7/7] iter --- .../java/org/elasticsearch/test/ESTestCase.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index cba2ba620df25..2ed86cf701378 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -576,17 +576,18 @@ public void removeHeaderWarningAppender() { } } - private org.elasticsearch.logging.Level capturedLogLevel = null; + private static org.elasticsearch.logging.Level capturedLogLevel = null; - @Before - public void captureLoggingLevel() { + // just capture the expected level once before the suite starts + @BeforeClass + public static void captureLoggingLevel() { capturedLogLevel = LoggerFactory.provider().getRootLevel(); } - @After - public void restoreLoggingLevel() { + @AfterClass + public static void restoreLoggingLevel() { if (capturedLogLevel != null) { - // log level might not have been captured if test was skipped + // log level might not have been captured if suite was skipped LoggerFactory.provider().setRootLevel(capturedLogLevel); capturedLogLevel = null; }