Skip to content

Set root logger level for CLIs #123742

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Mar 2, 2025
Merged

Set root logger level for CLIs #123742

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c5ab5be
Set root logger level for CLIs
rjernst Feb 28, 2025
90155d1
[CI] Auto commit changes from spotless
Feb 28, 2025
6099237
iter
rjernst Feb 28, 2025
b55a479
[CI] Auto commit changes from spotless
Feb 28, 2025
2a9144a
restore in ESTestCase instead
rjernst Feb 28, 2025
5c813f0
try again
rjernst Mar 1, 2025
4e32d24
iter
rjernst Mar 2, 2025
cf60706
Merge branch 'main' into logging/set_level_cli
rjernst Mar 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions libs/cli/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
2 changes: 2 additions & 0 deletions libs/cli/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
6 changes: 6 additions & 0 deletions libs/cli/src/main/java/org/elasticsearch/cli/Command.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
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;
Expand Down Expand Up @@ -84,12 +86,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);
Expand Down
2 changes: 1 addition & 1 deletion libs/logging/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
5 changes: 5 additions & 0 deletions libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LoggerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

package org.elasticsearch.logging.internal.spi;

import org.elasticsearch.logging.Level;
import org.elasticsearch.logging.Logger;

/**
Expand All @@ -26,6 +27,10 @@ public static LoggerFactory provider() {

public abstract Logger getLogger(Class<?> clazz);

public abstract void setRootLevel(Level level);

public abstract Level getRootLevel();

public static void setInstance(LoggerFactory INSTANCE) {
LoggerFactory.INSTANCE = INSTANCE;
}
Expand Down
39 changes: 35 additions & 4 deletions server/src/main/java/org/elasticsearch/common/logging/internal/LevelUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 + "]");
}
}
13 changes: 13 additions & 0 deletions server/src/main/java/org/elasticsearch/common/logging/internal/LoggerFactoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -30,4 +32,15 @@ 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);
}

@Override
public Level getRootLevel() {
return LevelUtil.elasticsearchLevel(LogManager.getRootLogger().getLevel());
}
}
11 changes: 11 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/cli/CommandTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@

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;
Expand Down Expand Up @@ -43,6 +46,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;

Expand All @@ -56,6 +61,7 @@ protected CommandTestCase(boolean spaceInPath) {

@Before
public void resetTerminal() throws IOException {
capturedLogLevel = LoggerFactory.provider().getRootLevel();
Copy link
Contributor

@mark-vieira mark-vieira Feb 28, 2025

Choose a reason for hiding this comment

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

We've bitten ourselves before by tests doing this and then all subsequent tests create insane amounts of log output causing all kinds of havoc. Is there any reason not to do this in ESTestCase so that we catch all these scenarios?

Copy link
Member Author

@rjernst rjernst Feb 28, 2025

Choose a reason for hiding this comment

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

Seems reasonable. I pushed 2a9144a

Copy link
Member Author

@rjernst rjernst Mar 2, 2025

Choose a reason for hiding this comment

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

I had to relax this a little bit. For some reason, capturing and restoring per-test in ESTestCase causes several tests which test logging to fail, seemingly because they don't get the expected log levels. So I changed this to be at the suite level for now.

terminal.reset();
terminal.setSupportsBinary(false);
terminal.setVerbosity(Terminal.Verbosity.NORMAL);
Expand All @@ -73,6 +79,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();

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/security/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down