-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Added logic of checking both slf4j impl and env for logging level #7068
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,11 +5,11 @@ | |
|
|
||
| import com.azure.core.util.Configuration; | ||
| import com.azure.core.util.CoreUtils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.Objects; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.slf4j.helpers.NOPLogger; | ||
|
|
||
| import static com.azure.core.implementation.LoggingUtil.getEnvironmentLoggingLevel; | ||
|
|
||
|
|
@@ -36,6 +36,7 @@ | |
| */ | ||
| public class ClientLogger { | ||
| private final Logger logger; | ||
| private final java.util.logging.Logger defaultLogger; | ||
|
|
||
| /** | ||
| * Retrieves a logger for the passed class using the {@link LoggerFactory}. | ||
|
|
@@ -50,9 +51,13 @@ public ClientLogger(Class<?> clazz) { | |
| * Retrieves a logger for the passed class name using the {@link LoggerFactory}. | ||
| * | ||
| * @param className Class name creating the logger. | ||
| * @throws RuntimeException it is an error. | ||
| */ | ||
| public ClientLogger(String className) { | ||
| logger = LoggerFactory.getLogger(className); | ||
| System.setProperty("java.util.logging.config.file", | ||
| ClientLogger.class.getClassLoader().getResource("logging.properties").getPath()); | ||
| defaultLogger = java.util.logging.Logger.getLogger(className); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -131,10 +136,9 @@ public void error(String format, Object... args) { | |
| * @param args Arguments for the message, if an exception is being logged last argument is the throwable. | ||
| */ | ||
| private void log(LogLevel logLevel, String format, Object... args) { | ||
| LogLevel environmentLoggingLevel = getEnvironmentLoggingLevel(); | ||
|
|
||
| if (canLogAtLevel(logLevel, environmentLoggingLevel)) { | ||
| performLogging(logLevel, environmentLoggingLevel, false, format, args); | ||
| LogLevel rootLogLevel = loadSystemLogLevel(); | ||
| if (canLogAtLevel(logLevel, rootLogLevel)) { | ||
| performLogging(logLevel, rootLogLevel, false, format, args); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -162,34 +166,23 @@ public RuntimeException logExceptionAsError(RuntimeException runtimeException) { | |
|
|
||
| private RuntimeException logException(RuntimeException runtimeException, LogLevel logLevel) { | ||
| Objects.requireNonNull(runtimeException, "'runtimeException' cannot be null."); | ||
| LogLevel rootLogLevel = loadSystemLogLevel(); | ||
|
|
||
| LogLevel environmentLoggingLevel = getEnvironmentLoggingLevel(); | ||
|
|
||
| if (canLogAtLevel(logLevel, environmentLoggingLevel)) { | ||
| performLogging(logLevel, environmentLoggingLevel, true, runtimeException.getMessage(), runtimeException); | ||
| if (canLogAtLevel(logLevel, rootLogLevel)) { | ||
| performLogging(logLevel, rootLogLevel, true, runtimeException.getMessage(), runtimeException); | ||
| } | ||
|
|
||
| return runtimeException; | ||
| } | ||
|
|
||
| /** | ||
| * Determines if the environment and logger support logging at the given log level. | ||
| * | ||
| * @param logLevel The {@link LogLevel} being validated as supported. | ||
| * @return Flag indicating if the environment and logger support logging at the given log level. | ||
| */ | ||
| public boolean canLogAtLevel(LogLevel logLevel) { | ||
| return canLogAtLevel(logLevel, getEnvironmentLoggingLevel()); | ||
| } | ||
|
|
||
| /* | ||
| * Performs the logging. | ||
| * | ||
| * @param format formattable message. | ||
| * @param args Arguments for the message, if an exception is being logged last argument is the throwable. | ||
| */ | ||
| private void performLogging(LogLevel logLevel, LogLevel environmentLogLevel, boolean isExceptionLogging, | ||
| String format, Object... args) { | ||
| private void performLogging(LogLevel logLevel, LogLevel rootLogLevel, | ||
| boolean isExceptionLogging, String format, Object... args) { | ||
| // If the logging level is less granular than verbose remove the potential throwable from the args. | ||
| String throwableMessage = ""; | ||
| if (doesArgsHaveThrowable(args)) { | ||
|
|
@@ -207,11 +200,55 @@ private void performLogging(LogLevel logLevel, LogLevel environmentLogLevel, boo | |
| * Environment is logging at a level higher than verbose, strip out the throwable as it would log its | ||
| * stack trace which is only expected when logging at a verbose level. | ||
| */ | ||
| if (environmentLogLevel.getLogLevel() > LogLevel.VERBOSE.getLogLevel()) { | ||
| if (rootLogLevel.getLogLevel() > LogLevel.VERBOSE.getLogLevel()) { | ||
| args = removeThrowable(args); | ||
| } | ||
| } | ||
| boolean isFromEnv = isLogFromEnv(); | ||
| printInPlace(isFromEnv, throwableMessage, logLevel, format, args); | ||
|
|
||
| } | ||
|
|
||
| private void printInPlace(boolean isFromEnv, String throwableMessage, LogLevel logLevel, String format, | ||
| Object... args) { | ||
| if (isFromEnv) { | ||
| printFromDefaultLogger(throwableMessage, logLevel, format, args); | ||
| } else { | ||
| printFromLogger(throwableMessage, logLevel, format, args); | ||
| } | ||
| } | ||
|
|
||
| private void printFromDefaultLogger(String throwableMessage, LogLevel logLevel, String format, Object... args) { | ||
| switch (logLevel) { | ||
| case VERBOSE: | ||
| defaultLogger.log(java.util.logging.Level.FINE, format, args); | ||
| break; | ||
| case INFORMATIONAL: | ||
| defaultLogger.log(java.util.logging.Level.INFO, format, args); | ||
| break; | ||
| case WARNING: | ||
| if (!CoreUtils.isNullOrEmpty(throwableMessage)) { | ||
| format += System.lineSeparator() + throwableMessage; | ||
| } | ||
| defaultLogger.log(java.util.logging.Level.WARNING, format, args); | ||
| break; | ||
| case ERROR: | ||
| if (!CoreUtils.isNullOrEmpty(throwableMessage)) { | ||
| format += System.lineSeparator() + throwableMessage; | ||
| } | ||
| defaultLogger.log(java.util.logging.Level.SEVERE, format, args); | ||
| break; | ||
| default: | ||
| // Don't do anything, this state shouldn't be possible. | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| private boolean isLogFromEnv() { | ||
| return logger instanceof NOPLogger; | ||
| } | ||
|
|
||
| private void printFromLogger(String throwableMessage, LogLevel logLevel, String format, Object... args) { | ||
| switch (logLevel) { | ||
| case VERBOSE: | ||
| logger.debug(format, args); | ||
|
|
@@ -237,37 +274,44 @@ private void performLogging(LogLevel logLevel, LogLevel environmentLogLevel, boo | |
| } | ||
| } | ||
|
|
||
| /* | ||
| * Determines if the environment and logger support logging at the given log level. | ||
| /** | ||
| * Determines if the app or environment logger support logging at the given log level. | ||
| * | ||
| * @param logLevel Logging level for the log message. | ||
| * @param environmentLoggingLevel Logging level the environment is set to support. | ||
| * @return Flag indicating if the environment and logger are configured to support logging at the given log level. | ||
| */ | ||
| private boolean canLogAtLevel(LogLevel logLevel, LogLevel environmentLoggingLevel) { | ||
| // Do not log if logLevel is null is not set. | ||
| if (logLevel == null) { | ||
| return false; | ||
| } | ||
| public boolean canLogAtLevel(LogLevel logLevel) { | ||
|
Member
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.
Contributor
Author
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. The PR is for reviewing the code flow, not finished yet. |
||
| LogLevel rootLogLevel = loadSystemLogLevel(); | ||
| return canLogAtLevel(logLevel, rootLogLevel); | ||
| } | ||
|
|
||
| // Attempting to log at a level not supported by the environment. | ||
| if (logLevel.getLogLevel() < environmentLoggingLevel.getLogLevel()) { | ||
| return false; | ||
| } | ||
| private boolean canLogAtLevel(LogLevel logLevel, LogLevel allowedLogLevel) { | ||
| // Attempting to log at a level not supported by the SLF4J configuration or env variable. | ||
| return logLevel.getLogLevel() >= allowedLogLevel.getLogLevel(); | ||
| } | ||
|
|
||
| // Determine if the logger configuration supports logging at the level. | ||
| switch (logLevel) { | ||
| case VERBOSE: | ||
| return logger.isDebugEnabled(); | ||
| case INFORMATIONAL: | ||
| return logger.isInfoEnabled(); | ||
| case WARNING: | ||
| return logger.isWarnEnabled(); | ||
| case ERROR: | ||
| return logger.isErrorEnabled(); | ||
| default: | ||
| return false; | ||
| /** | ||
| * Checking the system log with the preference order of slf4j and environment variable. | ||
| * | ||
| * @return The log level. | ||
| */ | ||
| private LogLevel loadSystemLogLevel() { | ||
| if (isLogFromEnv()) { | ||
| return getEnvironmentLoggingLevel(); | ||
| } | ||
| if (logger.isDebugEnabled()) { | ||
|
Member
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. Should the binder still be checked? That way if an SLF4J binding exists but all logging levels are disabled we won't default into the environment configuration and instead wouldn't log anything as expected.
Contributor
Author
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. Based on my testing, if there is some other logging binding to SLF4J, Also, Binder check API is inside of |
||
| return LogLevel.VERBOSE; | ||
| } | ||
| if (logger.isInfoEnabled()) { | ||
| return LogLevel.INFORMATIONAL; | ||
| } | ||
| if (logger.isWarnEnabled()) { | ||
| return LogLevel.WARNING; | ||
| } | ||
| if (logger.isErrorEnabled()) { | ||
| return LogLevel.ERROR; | ||
| } | ||
| return LogLevel.NOT_SET; | ||
| } | ||
|
|
||
| /* | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| handlers= java.util.logging.FileHandler, java.util.logging.ConsoleHandler | ||
| .level = ALL | ||
| java.util.logging.ConsoleHandler.level = FINER |
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.
Don't overwrite a system property. Use another means to configure the logger. Also, don't create a default logger unless necessary - you're paying the price here to create two loggers.
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.
We can initial it into log method. Try to figure out if we can set the logLevel during runtime.