Added logic of checking both slf4j impl and env for logging level #7068
Conversation
sima-zhu
requested review from
alzimmermsft,
hemanttanwar,
jianghaolu and
srnagar
as code owners
sdk/core/azure-core/src/main/java/com/azure/core/util/tracing/Tracer.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/tracing/Tracer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LogLevel is in an implementation package and will need to be moved to public package.
There was a problem hiding this comment.
The PR is for reviewing the code flow, not finished yet.
Will merge the changes in PR: #6886.
Also, will fix the tests after merging.
|
Need to update some tests or change their setup based on this change. If you want to add more tests around handling when SLF4J has a binder and which levels can be logged at you can look at the ClientLoggerTests which setup the logger with a given level of support and here is the conversion of environment log level name to level https://github.com/qos-ch/slf4j/blob/master/slf4j-simple/src/main/java/org/slf4j/simple/SimpleLoggerConfiguration.java#L145. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Based on my testing, if there is some other logging binding to SLF4J, logger.isDebugEnabled() will reflect the log level set by user. So if log4j set log level to info, then the clientLogger logger.canLogAtLevel(LogLevel.INFO) returns true.
Also, Binder check API is inside of slf4j-simple pkg which asks to introduce slf4j impl library to src code. This is not ideal as you comment above.
sima-zhu
requested review from
gapra-msft,
jaschrep-msft and
rickle-msft
as code owners
| 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); |
There was a problem hiding this comment.
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.
We can initial it into log method. Try to figure out if we can set the logLevel during runtime.
...analytics/azure-ai-textanalytics/src/samples/java/com/azure/ai/textanalytics/HelloWorld.java
Show resolved
Hide resolved
|
Closed the PR as we agreed to use Sys.out for now. |
5 participants
The PR is to have more checking on the log level.
We agreed to check
I have implemented src code only for team to review.
TODO: Will add more unit tests for code coverage.