Skip to content

Commit

Permalink
Allow for Log4J to be configured for peons but still ensure console l…
Browse files Browse the repository at this point in the history 
…ogging is enforced (#14094)

* Allow for Log4J to be configured for peons but still ensure console logging is enforced

This change will allow for log4j to be configured for peons but require console logging is still
configured for them to ensure peon logs are saved to deep storage.

Also fixed the test ConsoleLoggingEnforcementTest to use a valid appender for the non console
Config as the previous config was incorrect and would never return a logger.

* fix checkstyle

* add warning to logger when it overwrites all loggers to be console

* optimize calls for altering logging config for ConsoleLoggingEnforcementConfigurationFactory

add getName to the druid logger class

* update docs, and error message

* edit docs to be more clear

* fix checkstyle issues

* CI fixes - LoggerTest code coverage and fix spelling issue for logging docs
  • Loading branch information
@TSFenwick
TSFenwick authored Apr 24, 2023
1 parent 887f8db commit accd553
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 32 deletions.
14 changes: 8 additions & 6 deletions docs/configuration/logging.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,15 @@ The following example log4j2.xml is based upon the micro quickstart:
</Configuration>
```

Peons always output logs to standard output. Middle Managers redirect task logs from standard output to
[long-term storage](index.md#log-long-term-storage).

> NOTE:
> Although Druid shares the log4j configuration file task peon processes,
> the appenders in this file DO NOT take effect for peon processes. Peons always output logs to standard output.
> Middle Managers redirect task logs from standard output to [long-term storage](index.md#log-long-term-storage).
>
> However, log level settings do take effect for these task peon processes.
> This means you can configure loggers at different logging level for task logs using `log4j2.xml`.
> Druid shares the log4j configuration file among all services, including task peon processes.
> However, you must define a console appender in the logger for your peon processes.
> If you don't define a console appender, Druid creates and configures a new console appender
> that retains the log level, such as `info` or `warn`, but does not retain any other appender
> configuration, including non-console ones.
## Log directory
The included log4j2.xml configuration for Druid and ZooKeeper writes logs to the `log` directory at the root of the distribution.
Expand Down
28 changes: 22 additions & 6 deletions .../apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.apache.druid.indexing.common.tasklogs;

import org.apache.druid.java.util.common.logger.Logger;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.core.Appender;
import org.apache.logging.log4j.core.Filter;
Expand All @@ -28,6 +29,7 @@
import org.apache.logging.log4j.core.config.Configuration;
import org.apache.logging.log4j.core.config.ConfigurationFactory;
import org.apache.logging.log4j.core.config.ConfigurationSource;
import org.apache.logging.log4j.core.config.Configurator;
import org.apache.logging.log4j.core.config.LoggerConfig;
import org.apache.logging.log4j.core.config.xml.XmlConfiguration;
import org.apache.logging.log4j.core.layout.PatternLayout;
Expand All @@ -48,11 +50,20 @@
*/
public class ConsoleLoggingEnforcementConfigurationFactory extends ConfigurationFactory
{

private static final Logger log = new Logger(ConsoleLoggingEnforcementConfigurationFactory.class);

/**
* Valid file extensions for XML files.
*/
public static final String[] SUFFIXES = new String[]{".xml", "*"};

// Alter log level for this class to be warning. This needs to happen because the logger is using the default
// config, which is always level error and appends to console, since the logger is being configured by this class.
static {
Configurator.setLevel(log.getName(), Level.WARN);
}

@Override
public String[] getSupportedTypes()
{
Expand Down Expand Up @@ -95,6 +106,7 @@ protected void doConfigure()
loggerConfigList.add(this.getRootLogger());
loggerConfigList.addAll(this.getLoggers().values());


//
// For all logger configuration, check if its appender is ConsoleAppender.
// If not, replace it's appender to ConsoleAppender.
Expand All @@ -117,16 +129,16 @@ private Appender findConsoleAppender()
}

/**
* remove all appenders from a logger and append a console appender to it
* 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)
{
if (logger.getAppenderRefs().size() == 1
&& logger.getAppenderRefs().get(0).getRef().equals(consoleAppender.getName())) {
// this logger has only one appender and its the console appender
return;
for (AppenderRef appenderRef : logger.getAppenderRefs()) {
if (consoleAppender.getName().equals(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;

Expand All @@ -143,8 +155,12 @@ private void applyConsoleAppender(LoggerConfig logger, Appender consoleAppender)
// use the first appender's definition
level = appenderRef.getLevel();
filter = appenderRef.getFilter();
log.warn("Clearing all configured appenders for logger %s. Using %s instead.",
logger.toString(),
consoleAppender.getName());
}


// add ConsoleAppender to this logger
logger.addAppender(consoleAppender, level, filter);
}
Expand Down
55 changes: 35 additions & 20 deletions ...rc/test/java/org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.apache.logging.log4j.core.Logger;
import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.appender.ConsoleAppender;
import org.apache.logging.log4j.core.appender.HttpAppender;
import org.apache.logging.log4j.core.config.ConfigurationSource;
import org.apache.logging.log4j.core.layout.PatternLayout;
import org.junit.Assert;
Expand Down Expand Up @@ -79,9 +80,9 @@ public void testNoConsoleAppender() throws IOException
// this logger configuration has no console logger appender, a default one will be created
String log4jConfiguration = "<Configuration status=\"WARN\">\n"
+ " <Appenders>\n"
+ " <RollingRandomAccessFile name=\"FileAppender\" fileName=\"a.log\">\n"
+ " <PatternLayout pattern=\"%d{ISO8601} %p [%t] %c - %m%n\"/>\n"
+ " </RollingRandomAccessFile>\n"
+ " <Http name=\"Http\" url=\"http://localhost:9200/\">\n"
+ " <JsonLayout properties=\"true\"/>\n"
+ " </Http>\n"
+ " </Appenders>\n"
+ " <Loggers>\n"
+ " <Root level=\"info\">\n"
Expand Down Expand Up @@ -115,13 +116,13 @@ public void testHasConsoleAppenderButNotUsed() throws IOException
+ " <Console name=\"Console\" target=\"SYSTEM_OUT\">\n"
+ " <PatternLayout pattern=\"%m\"/>\n"
+ " </Console>\n"
+ " <RollingRandomAccessFile name=\"FileAppender\" fileName=\"a.log\">\n"
+ " <PatternLayout pattern=\"%d{ISO8601} %p [%t] %c - %m%n\"/>\n"
+ " </RollingRandomAccessFile>\n"
+ " <Http name=\"Http\" url=\"http://localhost:9200/\">\n"
+ " <JsonLayout properties=\"true\"/>\n"
+ " </Http>\n"
+ " </Appenders>\n"
+ " <Loggers>\n"
+ " <Root level=\"info\">\n"
+ " <AppenderRef ref=\"FileAppender\"/>\n"
+ " <AppenderRef ref=\"Http\"/>\n"
+ " </Root>\n"
+ " </Loggers>\n"
+ "</Configuration>";
Expand Down Expand Up @@ -152,17 +153,17 @@ public void testMultipleAppender() throws IOException
+ " <Console name=\"Console\" target=\"SYSTEM_OUT\">\n"
+ " <PatternLayout pattern=\"%m\"/>\n"
+ " </Console>\n"
+ " <RollingRandomAccessFile name=\"FileAppender\" fileName=\"a.log\">\n"
+ " <PatternLayout pattern=\"%d{ISO8601} %p [%t] %c - %m%n\"/>\n"
+ " </RollingRandomAccessFile>\n"
+ " <Http name=\"Http\" url=\"http://localhost:9200/\">\n"
+ " <JsonLayout properties=\"true\"/>\n"
+ " </Http>\n"
+ " </Appenders>\n"
+ " <Loggers>\n"
+ " <Root level=\"info\">\n"
+ " <AppenderRef ref=\"FileAppender\"/>\n"
+ " <AppenderRef ref=\"Http\"/>\n"
+ " <AppenderRef ref=\"Console\"/>\n"
+ " </Root>\n"
+ " <Logger level=\"debug\" name=\"org.apache.druid\" additivity=\"false\">\n"
+ " <AppenderRef ref=\"FileAppender\"/>\n"
+ " <AppenderRef ref=\"Http\"/>\n"
+ " <AppenderRef ref=\"Console\"/>\n"
+ " </Logger>\n"
+ " </Loggers>\n"
Expand All @@ -171,10 +172,10 @@ public void testMultipleAppender() throws IOException
LoggerContext context = enforceConsoleLogger(log4jConfiguration);

// this logger is not defined in configuration, it derivates ROOT logger configuration
assertHasOnlyOneConsoleAppender(getLogger(context, "name_not_in_config"), Level.INFO);
assertHasConsoleAppenderAndHttpAppender(getLogger(context, "name_not_in_config"), Level.INFO);

assertHasOnlyOneConsoleAppender(getLogger(context, "org.apache.druid"), Level.DEBUG);
assertHasOnlyOneConsoleAppender(getLogger(context, ROOT), Level.INFO);
assertHasConsoleAppenderAndHttpAppender(getLogger(context, "org.apache.druid"), Level.DEBUG);
assertHasConsoleAppenderAndHttpAppender(getLogger(context, ROOT), Level.INFO);

// the ConsoleAppender should be exactly the same as it's in the configuration
PatternLayout layout = (PatternLayout) getLogger(context, "anything").getAppenders()
Expand All @@ -195,15 +196,15 @@ public void testEmptyAppender() throws IOException
+ " <Console name=\"Console\" target=\"SYSTEM_OUT\">\n"
+ " <PatternLayout pattern=\"%m\"/>\n"
+ " </Console>\n"
+ " <RollingRandomAccessFile name=\"FileAppender\" fileName=\"a.log\">\n"
+ " <PatternLayout pattern=\"%d{ISO8601} %p [%t] %c - %m%n\"/>\n"
+ " </RollingRandomAccessFile>\n"
+ " <Http name=\"Http\" url=\"http://localhost:9200/\">\n"
+ " <JsonLayout properties=\"true\"/>\n"
+ " </Http>\n"
+ " </Appenders>\n"
+ " <Loggers>\n"
+ " <Root level=\"info\">\n"
+ " </Root>\n"
+ " <Logger level=\"debug\" name=\"org.apache.druid\" additivity=\"false\">\n"
+ " <AppenderRef ref=\"FileAppender\"/>\n"
+ " <AppenderRef ref=\"Http\"/>\n"
+ " <AppenderRef ref=\"Console\"/>\n"
+ " </Logger>\n"
+ " </Loggers>\n"
Expand All @@ -214,7 +215,7 @@ public void testEmptyAppender() throws IOException
// this logger is not defined in configuration, it derivates ROOT logger configuration
assertHasOnlyOneConsoleAppender(getLogger(context, "name_not_in_config"), Level.INFO);

assertHasOnlyOneConsoleAppender(getLogger(context, "org.apache.druid"), Level.DEBUG);
assertHasConsoleAppenderAndHttpAppender(getLogger(context, "org.apache.druid"), Level.DEBUG);
assertHasOnlyOneConsoleAppender(getLogger(context, ROOT), Level.INFO);

// the ConsoleAppender should be exactly the same as it's in the configuration
Expand All @@ -237,6 +238,20 @@ private LoggerContext enforceConsoleLogger(String configuration) throws IOExcept
return context;
}

private void assertHasConsoleAppenderAndHttpAppender(Logger logger, Level level)
{
// 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());

if (level != null) {
Assert.assertEquals(level, logger.getLevel());
}
}

private void assertHasOnlyOneConsoleAppender(Logger logger, Level level)
{
// there's only one appender
Expand Down
5 changes: 5 additions & 0 deletions processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,11 @@ private void logException(BiConsumer<String, Throwable> fn, Throwable t, String
}
}

public String getName()
{
return this.log.getName();
}

/**
* Logs all the segment ids you could ever want, {@link #SEGMENTS_PER_LOG_MESSAGE} at a time, as a comma separated
* list.
Expand Down
7 changes: 7 additions & 0 deletions processing/src/test/java/org/apache/druid/java/util/common/logger/LoggerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ public void testLogSegmentsMany()
Assert.assertEquals(expected, msgCount.getValue());
}

@Test
public void testGetName()
{
String expected = "org.apache.druid.java.util.common.logger.LoggerTest";
Assert.assertEquals(expected, log.getName());
}

private Logger.LogFunction getLogToListFunction(List<String> messages)
{
return (msg, format) -> messages.add(StringUtils.format(msg, format));
Expand Down
1 change: 1 addition & 0 deletions website/.spelling
View file
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,7 @@ APPROX_QUANTILE_FIXED_BUCKETS
100x
- ../docs/configuration/logging.md
_common
appender
appenders
- ../docs/dependencies/deep-storage.md
druid-hdfs-storage
Expand Down

0 comments on commit accd553

Please sign in to comment.