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

TSFenwick · 2023-04-15T02:34:10Z

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.

Description

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

If a ConsoleAppender is not configured, the peon will ignore the configured log4j appenders and use only a ConsoleAppender.

Release note

Changed: Peon logs can now be configured with multiple log4j2 Appenders. Previously, peon logs could only be configured with a ConsoleAppender. On upgrade, there is a chance peon logs will start logging to different appenders if a Logger used by the peon has multiple appenders configured and one of them is a console appender.

Key changed/added classes in this PR

ConsoleLoggingEnforcementConfigurationFactory
ConsoleLoggingEnforcementTest

This PR has:

…ogging 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.

TSFenwick · 2023-04-15T02:34:38Z

Still need to edit the docs for this change

FrankChen021 · 2023-04-15T06:03:47Z

Because peon's log is saved to deep storage by middlemanagers, and the web-console provides a UI to show logs of tasks from deep storage, what's the motivation that users configure peon to write log to somewhere else?

suneet-s · 2023-04-15T23:08:37Z

Because peon's log is saved to deep storage by middlemanagers, and the web-console provides a UI to show logs of tasks from deep storage, what's the motivation that users configure peon to write log to somewhere else?

Users may not want to write the entire peon logs to another location since the logs are uploaded to deep storage eventually.
However, I can see a case where a Druid operator might want to expose a subset of the peon logs for consumption by non Druid experts. Here's an example I can think of

Some users may want to see ingestion progress logs, but the current peon logs are too detailed. Something like this would allow a Druid operator to configure ingestion progress logs to be written to a separate location with another appender while the entire set of logs is still uploaded to deep storage.

...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java

suneet-s

This change seems reasonable to me.

Since we do not have integration tests for peon logs being uploaded to deep storage afaik, please also describe the manual testing done.

I'd also like to know how this behavior differs from the behavior in peons spawned by the KubernetesTaskRunner. If there are any differences in behavior, we'll have to think about how to call it out in the docs + release notes

...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java

…entConfigurationFactory add getName to the druid logger class

suneet-s

Both nits from me. LGTM after adding docs. I will run CI once the docs changes are made

suneet-s · 2023-04-17T22:32:48Z

...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java

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


Is there a reason not to do this in the static initialization block? near line 55. IMO that would be easier to follow

suneet-s · 2023-04-17T22:33:33Z

...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java

@@ -143,6 +152,7 @@ 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 ConsoleAppender instead.", logger.getName());


Suggested change

log.warn("Clearing all configured appenders for logger %s. Using ConsoleAppender instead.", logger.getName());

log.warn("Clearing all configured appenders for logger %s. Using %s instead.", logger.getName(), consoleAppender.getName());

suneet-s · 2023-04-18T16:51:14Z

docs/configuration/logging.md

@@ -107,8 +107,9 @@ The following example log4j2.xml is based upon the micro quickstart:

 > 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).
+> the appenders in this file WILL NOT take effect for peon processes if there is no console appender defined.


Suggested change

> the appenders in this file WILL NOT take effect for peon processes if there is no console appender defined.

> the appenders in this file WILL NOT take effect for loggers in the peon processes unless there is a console appender defined.

suneet-s

+1 after CI with a small nit on wording in the docs

cc @vtlim for docs change

docs/configuration/logging.md

vtlim · 2023-04-18T17:35:10Z

docs/configuration/logging.md

@@ -107,8 +107,9 @@ The following example log4j2.xml is based upon the micro quickstart:

 > NOTE:
 > Although Druid shares the log4j configuration file task peon processes,


Shares from whom to whom? Should it say "shares the log4j configuration file between task peon processes"?

How is this for the write up?

Although Druid shares the log4j configuration file amongst all services, including task peon processes, the appenders in this file WILL NOT take effect for loggers in the peon processes unless there is a console appender defined. Instead all the appenders for peons processes will be overwritten to be console appenders and use the log levels as defined in the configuration file. This means you can configure loggers at different logging level for task logs using log4j2.xml.

I think keeping the note is useful since it is something that isn't intuitive, but you know docs better than me so i'm happy to remove it it

docs/configuration/logging.md

…g docs

suneet-s

LGTM with 1 nit about spacing. No need to address this comment if there are no other changes needed.

@vtlim / @FrankChen021 Do you have any other comments?

suneet-s · 2023-04-20T01:51:47Z

...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java

@@ -95,6 +106,7 @@ protected void doConfigure()
      loggerConfigList.add(this.getRootLogger());
      loggerConfigList.addAll(this.getLoggers().values());

+


nit: Extra whitespace. Same comment on line 163

vtlim

LGTM for docs

fix checkstyle

1b19c75

suneet-s reviewed Apr 15, 2023

View reviewed changes

...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java Outdated Show resolved Hide resolved

suneet-s added the Area - Ingestion label Apr 15, 2023

suneet-s reviewed Apr 15, 2023

View reviewed changes

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

b4283fb

suneet-s reviewed Apr 17, 2023

View reviewed changes

...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java Outdated Show resolved Hide resolved

suneet-s reviewed Apr 17, 2023

View reviewed changes

...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java Outdated Show resolved Hide resolved

optimize calls for altering logging config for ConsoleLoggingEnforcem…

be241d2

…entConfigurationFactory add getName to the druid logger class

suneet-s reviewed Apr 17, 2023

View reviewed changes

update docs, and error message

f997c93

github-actions bot added the Area - Documentation label Apr 18, 2023

suneet-s reviewed Apr 18, 2023

View reviewed changes

suneet-s approved these changes Apr 18, 2023

View reviewed changes

vtlim reviewed Apr 18, 2023

View reviewed changes

TSFenwick added 3 commits April 18, 2023 22:36

edit docs to be more clear

d2eb8c2

fix checkstyle issues

b394da4

CI fixes - LoggerTest code coverage and fix spelling issue for loggin…

77a5196

…g docs

suneet-s approved these changes Apr 20, 2023

View reviewed changes

vtlim approved these changes Apr 20, 2023

View reviewed changes

suneet-s merged commit accd553 into apache:master Apr 24, 2023

maytasm mentioned this pull request Jul 4, 2023

Allow multiple consoleAppender to be used in peon logging #14521

Merged

10 tasks

abhishekagarwal87 added this to the 27.0 milestone Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TSFenwick commented Apr 15, 2023 •

edited by suneet-s

Loading

TSFenwick commented Apr 15, 2023

FrankChen021 commented Apr 15, 2023

suneet-s commented Apr 15, 2023

suneet-s left a comment

suneet-s left a comment

suneet-s Apr 17, 2023

suneet-s Apr 17, 2023

suneet-s Apr 18, 2023

suneet-s left a comment

vtlim Apr 18, 2023

TSFenwick Apr 18, 2023

suneet-s left a comment

suneet-s Apr 20, 2023

vtlim left a comment

	log.warn("Clearing all configured appenders for logger %s. Using ConsoleAppender instead.", logger.getName());
	log.warn("Clearing all configured appenders for logger %s. Using %s instead.", logger.getName(), consoleAppender.getName());

	> the appenders in this file WILL NOT take effect for peon processes if there is no console appender defined.
	> the appenders in this file WILL NOT take effect for loggers in the peon processes unless there is a console appender defined.

		@@ -107,8 +107,9 @@ The following example log4j2.xml is based upon the micro quickstart:

		> NOTE:
		> Although Druid shares the log4j configuration file task peon processes,

		@@ -95,6 +106,7 @@ protected void doConfigure()
		loggerConfigList.add(this.getRootLogger());
		loggerConfigList.addAll(this.getLoggers().values());

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

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

Conversation

TSFenwick commented Apr 15, 2023 • edited by suneet-s Loading

Description

Release note

Key changed/added classes in this PR

TSFenwick commented Apr 15, 2023

FrankChen021 commented Apr 15, 2023

suneet-s commented Apr 15, 2023

suneet-s left a comment

Choose a reason for hiding this comment

suneet-s left a comment

Choose a reason for hiding this comment

suneet-s Apr 17, 2023

Choose a reason for hiding this comment

suneet-s Apr 17, 2023

Choose a reason for hiding this comment

suneet-s Apr 18, 2023

Choose a reason for hiding this comment

suneet-s left a comment

Choose a reason for hiding this comment

vtlim Apr 18, 2023

Choose a reason for hiding this comment

TSFenwick Apr 18, 2023

Choose a reason for hiding this comment

suneet-s left a comment

Choose a reason for hiding this comment

suneet-s Apr 20, 2023

Choose a reason for hiding this comment

vtlim left a comment

Choose a reason for hiding this comment

TSFenwick commented Apr 15, 2023 •

edited by suneet-s

Loading