-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
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
…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.
Still need to edit the docs for this change |
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.
|
...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
...org/apache/druid/indexing/common/tasklogs/ConsoleLoggingEnforcementConfigurationFactory.java
Outdated
Show resolved
Hide resolved
…entConfigurationFactory add getName to the druid logger class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both nits from me. LGTM after adding docs. I will run CI once the docs changes are made
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to do this in the static initialization block? near line 55. IMO that would be easier to follow
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()); |
docs/configuration/logging.md
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 after CI with a small nit on wording in the docs
cc @vtlim for docs change
docs/configuration/logging.md
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shares from whom to whom? Should it say "shares the log4j configuration file between task peon processes"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -95,6 +106,7 @@ protected void doConfigure() | |||
loggerConfigList.add(this.getRootLogger()); | |||
loggerConfigList.addAll(this.getLoggers().values()); | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Extra whitespace. Same comment on line 163
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for docs
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: