From b1801549590a91a39b4195dd790af7803c9fdf98 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Mon, 3 Jul 2023 18:23:45 -0700 Subject: [PATCH] Allow multiple consoleAppender to be used in peon logging --- ...oggingEnforcementConfigurationFactory.java | 40 +++++---- .../ConsoleLoggingEnforcementTest.java | 82 ++++++++++++++++++- 2 files changed, 102 insertions(+), 20 deletions(-) diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java b/indexing-service/src/main/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java index a8f774177b7c..70a6a24325ef 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java @@ -34,7 +34,7 @@ import org.apache.logging.log4j.core.config.xml.XmlConfiguration; import org.apache.logging.log4j.core.layout.PatternLayout; -import javax.annotation.Nullable; +import javax.annotation.Nonnull; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -91,15 +91,18 @@ protected void doConfigure() { super.doConfigure(); - Appender consoleAppender = findConsoleAppender(); - if (consoleAppender == null) { + List consoleAppenders = findConsoleAppenders(); + if (consoleAppenders.isEmpty()) { // create a ConsoleAppender with default pattern if no console appender is configured in the configuration file - consoleAppender = ConsoleAppender.newBuilder() - .setName("_Injected_Console_Appender_") - .setLayout(PatternLayout.newBuilder() - .withPattern("%d{ISO8601} %p [%t] %c - %m%n") - .build()) - .build(); + consoleAppenders.add( + ConsoleAppender.newBuilder() + .setName("_Injected_Console_Appender_") + .setLayout(PatternLayout.newBuilder() + .withPattern("%d{ISO8601} %p [%t] %c - %m%n") + .build() + ) + .build() + ); } List loggerConfigList = new ArrayList<>(); @@ -112,36 +115,38 @@ protected void doConfigure() // If not, replace it's appender to ConsoleAppender. // for (LoggerConfig logger : loggerConfigList) { - applyConsoleAppender(logger, consoleAppender); + applyConsoleAppender(logger, consoleAppenders); } } - @Nullable - private Appender findConsoleAppender() + @Nonnull + private List findConsoleAppenders() { + List consoleAppenders = new ArrayList<>(); for (Map.Entry entry : this.getAppenders().entrySet()) { Appender appender = entry.getValue(); if (appender instanceof ConsoleAppender) { - return appender; + consoleAppenders.add(appender); } } - return null; + return consoleAppenders; } /** * Ensure there is a console logger defined. Without a console logger peon logs wont be able to be stored in deep storage */ - private void applyConsoleAppender(LoggerConfig logger, Appender consoleAppender) + private void applyConsoleAppender(LoggerConfig logger, List consoleAppenders) { + List consoleAppenderNames = consoleAppenders.stream().map(Appender::getName).collect(Collectors.toList()); for (AppenderRef appenderRef : logger.getAppenderRefs()) { - if (consoleAppender.getName().equals(appenderRef.getRef())) { + if (consoleAppenderNames.contains(appenderRef.getRef())) { // we need a console logger no matter what, but we want to be able to define a different appender if necessary return; } } Level level = Level.INFO; Filter filter = null; - + Appender consoleAppender = consoleAppenders.get(0); if (!logger.getAppenderRefs().isEmpty()) { AppenderRef appenderRef = logger.getAppenderRefs().get(0); @@ -160,7 +165,6 @@ private void applyConsoleAppender(LoggerConfig logger, Appender consoleAppender) consoleAppender.getName()); } - // add ConsoleAppender to this logger logger.addAppender(consoleAppender, level, filter); } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementTest.java index e74f87748880..cf434ac89ff3 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementTest.java @@ -228,6 +228,73 @@ public void testEmptyAppender() throws IOException Assert.assertEquals("%m", layout.getConversionPattern()); } + @Test + public void testMultipleConsoleAppender() throws IOException + { + // this logger configuration contains multiple appenders and appender refers + String log4jConfiguration = "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""; + + LoggerContext context = enforceConsoleLogger(log4jConfiguration); + + // this logger is not defined in configuration, it derivates ROOT logger configuration + assertHasConsoleAppenderAndHttpAppender(getLogger(context, "name_not_in_config"), Level.INFO); + + assertHasConsoleAppenderAndHttpAppender(getLogger(context, "org.apache.druid"), Level.DEBUG); + assertHasConsoleAppenderAndHttpAppender(getLogger(context, ROOT), Level.INFO); + + assertHasConsoleAppenderAndHttpAppenderWithNameValidation(getLogger(context, "org.apache.mmon1"), Level.DEBUG, "Console1", "Http"); + assertHasConsoleAppenderAndHttpAppenderWithNameValidation(getLogger(context, "org.apache.mmon2"), Level.DEBUG, "Console2", "Http"); + assertHasOnlyOneConsoleAppender(getLogger(context, "org.apache.mmon3"), Level.DEBUG); + assertHasOnlyOneConsoleAppender(getLogger(context, "org.apache.mmon4"), Level.DEBUG); + + // the ConsoleAppender should be exactly the same as it's in the configuration + PatternLayout layout = (PatternLayout) getLogger(context, "anything").getAppenders() + .values() + .stream() + .findFirst() + .get() + .getLayout(); + Assert.assertEquals("%m", layout.getConversionPattern()); + } + private LoggerContext enforceConsoleLogger(String configuration) throws IOException { LoggerContext context = new LoggerContext("test"); @@ -239,19 +306,30 @@ private LoggerContext enforceConsoleLogger(String configuration) throws IOExcept } private void assertHasConsoleAppenderAndHttpAppender(Logger logger, Level level) + { + assertHasConsoleAppenderAndHttpAppenderWithNameValidation(logger, level, "Console", "Http"); + } + + private void assertHasConsoleAppenderAndHttpAppenderWithNameValidation( + Logger logger, + Level level, + String consoleName, + String httpName + ) { // there's two appenders Assert.assertEquals(2, logger.getAppenders().size()); // and the appenders must be ConsoleAppender and HttpAppender - Assert.assertEquals(ConsoleAppender.class, logger.getAppenders().get("Console").getClass()); - Assert.assertEquals(HttpAppender.class, logger.getAppenders().get("Http").getClass()); + Assert.assertEquals(ConsoleAppender.class, logger.getAppenders().get(consoleName).getClass()); + Assert.assertEquals(HttpAppender.class, logger.getAppenders().get(httpName).getClass()); if (level != null) { Assert.assertEquals(level, logger.getLevel()); } } + private void assertHasOnlyOneConsoleAppender(Logger logger, Level level) { // there's only one appender