-
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
Merged
suneet-s
merged 8 commits into
apache:master
from
TSFenwick:feature-allow-for-peons-to-use-other-loggers-in-addition-to-console-logger
Apr 24, 2023
Merged
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5259294
Allow for Log4J to be configured for peons but still ensure console l…
TSFenwick 1b19c75
fix checkstyle
TSFenwick b4283fb
add warning to logger when it overwrites all loggers to be console
TSFenwick be241d2
optimize calls for altering logging config for ConsoleLoggingEnforcem…
TSFenwick f997c93
update docs, and error message
TSFenwick d2eb8c2
edit docs to be more clear
TSFenwick b394da4
fix checkstyle issues
TSFenwick 77a5196
CI fixes - LoggerTest code coverage and fix spelling issue for loggin…
TSFenwick File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
@@ -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; | ||||||
|
@@ -48,6 +50,9 @@ | |||||
*/ | ||||||
public class ConsoleLoggingEnforcementConfigurationFactory extends ConfigurationFactory | ||||||
{ | ||||||
|
||||||
private static final Logger log = new Logger(ConsoleLoggingEnforcementConfigurationFactory.class); | ||||||
|
||||||
/** | ||||||
* Valid file extensions for XML files. | ||||||
*/ | ||||||
|
@@ -95,6 +100,10 @@ protected void doConfigure() | |||||
loggerConfigList.add(this.getRootLogger()); | ||||||
loggerConfigList.addAll(this.getLoggers().values()); | ||||||
|
||||||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. nit: Extra whitespace. Same comment on line 163 |
||||||
// | ||||||
// For all logger configuration, check if its appender is ConsoleAppender. | ||||||
// If not, replace it's appender to ConsoleAppender. | ||||||
|
@@ -117,16 +126,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; | ||||||
|
||||||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
// add ConsoleAppender to this logger | ||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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