Consolidating logging initialization in cli launcher (#85920)

Serveral mechanisms exist for intializing logging in cli tools. Some
base Command classes exist which initialize logging. But they do this
late, when they are constructed, which may be after static init has
occured for classes grabbing a Logger. Other CLIs like node tool
explicitly initialize logging to avoid that problem.

This commit removes all the of the LoggingAware classes, and
unifies logging configuration to occur at the very beginning of the cli
launcher.

relates #85758

Author: rjernst

distribution/tools/cli-launcher/src/main/java/org/elasticsearch/launcher/CliToolLauncher.java

@@ -8,9 +8,12 @@
 
 package org.elasticsearch.launcher;
 
+import org.apache.logging.log4j.Level;
 import org.elasticsearch.cli.CliToolProvider;
 import org.elasticsearch.cli.Command;
 import org.elasticsearch.cli.Terminal;
+import org.elasticsearch.common.logging.LogConfigurator;
+import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.core.SuppressForbidden;
 
 import java.util.Map;
@@ -43,6 +46,10 @@ class CliToolLauncher {
      */
     public static void main(String[] args) throws Exception {
         Map<String, String> sysprops = getSystemProperties();
+
+        // configure logging as early as possible
+        configureLoggingWithoutConfig(sysprops);
+
         String toolname = getToolName(sysprops);
         String libs = sysprops.getOrDefault("cli.libs", "");
 
@@ -76,4 +83,15 @@ private static Map<String, String> getSystemProperties() {
     private static void exit(int status) {
         System.exit(status);
     }
+
+    /**
+     * Configures logging without Elasticsearch configuration files based on the system property "es.logger.level" only. As such, any
+     * logging will be written to the console.
+     */
+    private static void configureLoggingWithoutConfig(Map<String, String> sysprops) {
+        // initialize default for es.logger.level because we will not read the log4j2.properties
+        final String loggerLevel = sysprops.getOrDefault("es.logger.level", Level.INFO.name());
+        final Settings settings = Settings.builder().put("logger.level", loggerLevel).build();
+        LogConfigurator.configureWithoutConfig(settings);
+    }
 }

distribution/tools/geoip-cli/src/main/java/org/elasticsearch/geoip/GeoIpCli.java

@@ -47,7 +47,7 @@ public class GeoIpCli extends Command {
     private final OptionSpec<String> targetDirectory;
 
     public GeoIpCli() {
-        super("A CLI tool to prepare local GeoIp database service", () -> {});
+        super("A CLI tool to prepare local GeoIp database service");
         sourceDirectory = parser.acceptsAll(Arrays.asList("s", "source"), "Source directory").withRequiredArg().required();
         targetDirectory = parser.acceptsAll(Arrays.asList("t", "target"), "Target directory").withRequiredArg();
 

distribution/tools/keystore-cli/src/main/java/org/elasticsearch/cli/keystore/KeyStoreCli.java

@@ -8,12 +8,12 @@
 
 package org.elasticsearch.cli.keystore;
 
-import org.elasticsearch.common.cli.LoggingAwareMultiCommand;
+import org.elasticsearch.cli.MultiCommand;
 
 /**
  * A cli tool for managing secrets in the elasticsearch keystore.
  */
-class KeyStoreCli extends LoggingAwareMultiCommand {
+class KeyStoreCli extends MultiCommand {
 
     KeyStoreCli() {
         super("A tool for managing settings stored in the elasticsearch keystore");

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/cli/PluginCli.java

@@ -9,7 +9,7 @@
 package org.elasticsearch.plugins.cli;
 
 import org.elasticsearch.cli.Command;
-import org.elasticsearch.common.cli.LoggingAwareMultiCommand;
+import org.elasticsearch.cli.MultiCommand;
 import org.elasticsearch.core.internal.io.IOUtils;
 
 import java.io.IOException;
@@ -19,7 +19,7 @@
 /**
  * A cli tool for adding, removing and listing plugins for elasticsearch.
  */
-class PluginCli extends LoggingAwareMultiCommand {
+class PluginCli extends MultiCommand {
 
     private final Collection<Command> commands;
 

libs/cli/src/main/java/org/elasticsearch/cli/Command.java

@@ -33,8 +33,6 @@ public abstract class Command implements Closeable {
     /** A description of the command, used in the help output. */
     protected final String description;
 
-    private final Runnable beforeMain;
-
     // these are the system properties and env vars from the environment,
     // but they can be overriden by tests. Really though Command should be stateless,
     // so the signature of main should take them in, which can happen once the entrypoint
@@ -52,13 +50,11 @@ public abstract class Command implements Closeable {
 
     /**
      * Construct the command with the specified command description and runnable to execute before main is invoked.
+     *  @param description the command description
      *
-     * @param description the command description
-     * @param beforeMain the before-main runnable
      */
-    public Command(final String description, final Runnable beforeMain) {
+    public Command(final String description) {
         this.description = description;
-        this.beforeMain = beforeMain;
         this.sysprops = captureSystemProperties();
         this.envVars = captureEnvironmentVariables();
     }
@@ -86,8 +82,6 @@ public final int main(String[] args, Terminal terminal) throws Exception {
             Runtime.getRuntime().addShutdownHook(shutdownHookThread);
         }
 
-        beforeMain.run();
-
         try {
             mainWithoutErrorHandling(args, terminal);
         } catch (OptionException e) {

libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java

@@ -34,12 +34,11 @@ public class MultiCommand extends Command {
 
     /**
      * Construct the multi-command with the specified command description and runnable to execute before main is invoked.
+     *  @param description the multi-command description
      *
-     * @param description the multi-command description
-     * @param beforeMain the before-main runnable
      */
-    public MultiCommand(final String description, final Runnable beforeMain) {
-        super(description, beforeMain);
+    public MultiCommand(final String description) {
+        super(description);
         this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class);
         parser.posixlyCorrect(true);
     }

qa/evil-tests/src/test/java/org/elasticsearch/cli/EvilCommandTests.java

@@ -24,7 +24,7 @@ public class EvilCommandTests extends ESTestCase {
     public void testCommandShutdownHook() throws Exception {
         final AtomicBoolean closed = new AtomicBoolean();
         final boolean shouldThrow = randomBoolean();
-        final Command command = new Command("test-command-shutdown-hook", () -> {}) {
+        final Command command = new Command("test-command-shutdown-hook") {
             @Override
             protected void execute(Terminal terminal, OptionSet options) throws Exception {
 

server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java

@@ -42,7 +42,7 @@ class Elasticsearch extends EnvironmentAwareCommand {
 
     // visible for testing
     Elasticsearch() {
-        super("Starts Elasticsearch", () -> {}); // we configure logging later so we override the base class from configuring logging
+        super("Starts Elasticsearch"); // we configure logging later so we override the base class from configuring logging
         versionOption = parser.acceptsAll(Arrays.asList("V", "version"), "Prints Elasticsearch version information and exits");
         daemonizeOption = parser.acceptsAll(Arrays.asList("d", "daemonize"), "Starts Elasticsearch in the background")
             .availableUnless(versionOption);

server/src/main/java/org/elasticsearch/cluster/coordination/NodeToolCli.java

@@ -8,22 +8,13 @@
 package org.elasticsearch.cluster.coordination;
 
 import org.elasticsearch.cli.MultiCommand;
-import org.elasticsearch.common.cli.CommandLoggingConfigurator;
 import org.elasticsearch.env.NodeRepurposeCommand;
 import org.elasticsearch.env.OverrideNodeVersionCommand;
 
-// NodeToolCli does not extend LoggingAwareCommand, because LoggingAwareCommand performs logging initialization
-// after LoggingAwareCommand instance is constructed.
-// It's too late for us, because before UnsafeBootstrapMasterCommand is added to the list of subcommands
-// log4j2 initialization will happen, because it has static reference to Logger class.
-// Even if we avoid making a static reference to Logger class, there is no nice way to avoid declaring
-// UNSAFE_BOOTSTRAP, which depends on ClusterService, which in turn has static Logger.
-// TODO execute CommandLoggingConfigurator.configureLoggingWithoutConfig() in the constructor of commands, not in beforeMain
 class NodeToolCli extends MultiCommand {
 
     NodeToolCli() {
-        super("A CLI tool to do unsafe cluster and index manipulations on current node", () -> {});
-        CommandLoggingConfigurator.configureLoggingWithoutConfig();
+        super("A CLI tool to do unsafe cluster and index manipulations on current node");
         subcommands.put("repurpose", new NodeRepurposeCommand());
         subcommands.put("unsafe-bootstrap", new UnsafeBootstrapMasterCommand());
         subcommands.put("detach-cluster", new DetachClusterCommand());

server/src/main/java/org/elasticsearch/common/cli/CommandLoggingConfigurator.java

@@ -39,18 +39,7 @@ public abstract class EnvironmentAwareCommand extends Command {
      * @param description the command description
      */
     public EnvironmentAwareCommand(final String description) {
-        this(description, CommandLoggingConfigurator::configureLoggingWithoutConfig);
-    }
-
-    /**
-     * Construct the command with the specified command description and runnable to execute before main is invoked. Commands constructed
-     * with this constructor must take ownership of configuring logging.
-     *
-     * @param description the command description
-     * @param beforeMain the before-main runnable
-     */
-    public EnvironmentAwareCommand(final String description, final Runnable beforeMain) {
-        super(description, beforeMain);
+        super(description);
         this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class);
     }
 

server/src/main/java/org/elasticsearch/common/cli/EnvironmentAwareCommand.java

@@ -7,12 +7,12 @@
  */
 package org.elasticsearch.index.shard;
 
-import org.elasticsearch.common.cli.LoggingAwareMultiCommand;
+import org.elasticsearch.cli.MultiCommand;
 
 /**
  * Class encapsulating and dispatching commands from the {@code elasticsearch-shard} command line tool
  */
-class ShardToolCli extends LoggingAwareMultiCommand {
+class ShardToolCli extends MultiCommand {
 
     ShardToolCli() {
         super("A CLI tool to remove corrupted parts of unrecoverable shards");

server/src/main/java/org/elasticsearch/common/cli/LoggingAwareCommand.java

@@ -34,7 +34,7 @@ static class DummyCommand extends Command {
         Exception exception = null;
 
         DummyCommand() {
-            super("Does nothing", () -> {});
+            super("Does nothing");
         }
 
         @Override

server/src/main/java/org/elasticsearch/common/cli/LoggingAwareMultiCommand.java

@@ -30,7 +30,7 @@ static class DummyMultiCommand extends MultiCommand {
         final AtomicBoolean closed = new AtomicBoolean();
 
         DummyMultiCommand() {
-            super("A dummy multi command", () -> {});
+            super("A dummy multi command");
         }
 
         @Override
@@ -56,7 +56,7 @@ static class DummySubCommand extends Command {
         }
 
         DummySubCommand(final boolean throwsExceptionOnClose) {
-            super("A dummy subcommand", () -> {});
+            super("A dummy subcommand");
             this.throwsExceptionOnClose = throwsExceptionOnClose;
         }
 
@@ -203,7 +203,7 @@ public void testCloseWhenSubCommandCloseThrowsException() throws Exception {
 
     static class ErrorThrowingSubCommand extends Command {
         ErrorThrowingSubCommand() {
-            super("error throwing", () -> {});
+            super("error throwing");
         }
 
         @Override

server/src/main/java/org/elasticsearch/index/shard/ShardToolCli.java

@@ -9,10 +9,10 @@
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
 
+import org.elasticsearch.cli.Command;
 import org.elasticsearch.cli.ExitCodes;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.cli.UserException;
-import org.elasticsearch.common.cli.LoggingAwareCommand;
 import org.elasticsearch.core.PathUtils;
 import org.elasticsearch.core.SuppressForbidden;
 
@@ -24,7 +24,7 @@
 
 import static org.elasticsearch.license.CryptUtils.writeEncryptedPrivateKey;
 
-public class KeyPairGeneratorTool extends LoggingAwareCommand {
+public class KeyPairGeneratorTool extends Command {
 
     private final OptionSpec<String> publicKeyPathOption;
     private final OptionSpec<String> privateKeyPathOption;

server/src/test/java/org/elasticsearch/cli/CommandTests.java

@@ -9,12 +9,12 @@
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
 
+import org.elasticsearch.cli.Command;
 import org.elasticsearch.cli.ExitCodes;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.cli.UserException;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesArray;
-import org.elasticsearch.common.cli.LoggingAwareCommand;
 import org.elasticsearch.core.PathUtils;
 import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.license.License;
@@ -28,7 +28,7 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 
-public class LicenseGeneratorTool extends LoggingAwareCommand {
+public class LicenseGeneratorTool extends Command {
 
     private final OptionSpec<String> publicKeyPathOption;
     private final OptionSpec<String> privateKeyPathOption;