Implements a slf4j logger impl as default logger in azure core #7298

sima-zhu · 2020-01-09T02:43:34Z

No description provided.

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLoggerBase.java

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLoggerBase.java

alzimmermsft · 2020-01-09T05:59:38Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+    void write(StringBuilder buf, Throwable t) {
+        PrintStream targetStream = System.out;
+
+        targetStream.println(buf.toString());


Should make the entire print statement a single operation to prevent race conditions interleaving messages

https://stackoverflow.com/questions/9459657/is-multi-thread-output-from-system-out-println-interleaved

Make the print process atomic so the log message will not intervene by other message, or print throwable to other places.

The individual print statement shouldn't interleave but right now we print the message and stack trace in two separate calls which could result in the following scenario.

Two exceptions are being logged, problemA and problemB, logging could have the following outcome

problemA message

problemB message

problemA stack trace

problemB stack trace

Instead we should build the message + stack trace and log them in one print call.

void write(StringBuilder buf, Throwable t) { if (t != null) { StringWriter sw = new StringWriter(); PrintWriter pw = new PrintWriter(sw); t.printStackTrace(pw); String sStackTrace = sw.toString(); buf.append(sStackTrace); } System.out.println(buf.toString()); }

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

alzimmermsft · 2020-01-09T19:28:03Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLoggerBase.java

+    /**
+     * Log a message at the TRACE level. It is currently not supported.
+     *
+     * @param msg the message string to be logged
+     * @throws UnsupportedOperationException It is currently not supported.
+     */


Let's remove these Javadocs and just use

/** * {@inheritDoc} */

srnagar · 2020-01-09T19:58:09Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLoggerBase.java

+import org.slf4j.Logger;
+import org.slf4j.Marker;
+
+abstract class DefaultLoggerBase implements Logger {


srnagar · 2020-01-09T21:30:08Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+            StringWriter sw = new StringWriter();
+            PrintWriter pw = new PrintWriter(sw);


Both StringWriter and PrintWriter should be closed after use

StringWriter does not need to close as it is a no op.
https://stackoverflow.com/questions/14542535/will-not-closing-a-stringwriter-cause-a-leak

Closed the PrintStream

srnagar · 2020-01-09T21:52:30Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+     * (non-parameterized) log messages.
+     *
+     * @param level
+     *            One of the LOG_LEVEL_XXX constants defining the log level


nit: fix formatting.

srnagar · 2020-01-09T21:55:05Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+     */
+    @Override
+    public boolean isDebugEnabled() {
+        throw new UnsupportedOperationException();


This should be supported since we have an implementation for debug(String format, Object.. argArray) on line 61.

srnagar · 2020-01-09T21:55:58Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+     */
+    @Override
+    public void debug(final String msg) {
+        throw new UnsupportedOperationException();


This should be supported too as we have support for debug overload with formatted strings.

I enabled the ones used by ClientLogger.

srnagar · 2020-01-09T21:56:23Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+     */
+    @Override
+    public String getName() {
+        throw new UnsupportedOperationException();


This should be supported. We should return the name of this logger instance.

Talked offline. Will create an issue and put Up-for-grabs label.

github.com//issues/7341

srnagar · 2020-01-09T21:57:29Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+     */
+    @Override
+    public boolean isInfoEnabled() {
+        throw new UnsupportedOperationException();


All is*Enabled() methods should be supported.

srnagar · 2020-01-09T22:03:40Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+     */
+    @Override
+    public void info(final String msg) {
+        throw new UnsupportedOperationException();


These should be implemented too.

Same as above

srnagar · 2020-01-10T01:55:48Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+            PrintWriter pw = new PrintWriter(sw);
+            t.printStackTrace(pw);
+            buf.append(sw.toString());
+            pw.close();


Use try-with-resources to ensure the resource is always closed.

Since StringWriter has no-op close(), I only move pw to try with resource. Thanks for pointing this out!

alzimmermsft · 2020-01-10T17:53:22Z

sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java

+     * @return The log level.
+     */
+    private LogLevel getConfiguredLogLevel() {
+        if (isFromEnv) {


If we have the default logger implement the isDebugEnabled and other similar APIs there would be no need for isFromEnv nor would there need to be any different handling here.

The only reason the environment logging level was passed around was to handle the fact that there was a two tiered structure of SLF4J logging level and environment logging level, not that this is being merged a lot of the logic in this class and within HttpLoggingPolicy could be cleaned up.

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

…into addlogimpl

srnagar · 2020-01-11T00:08:56Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+        String logLevelStr = Configuration.getGlobalConfiguration().get(AZURE_LOG_LEVEL);
+        LogLevel currentLogLevel = LogLevel.fromString(logLevelStr);
+        return LogLevel.VERBOSE.getLogLevel() >= currentLogLevel.getLogLevel();


This code is common for all is*Enabled() methods. You can extract this out to a method to reduce duplication.

srnagar · 2020-01-11T00:10:05Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+
+    /**
+     * {@inheritDoc}
+     * @param format The formattable message to log.


Combination of inheritdoc and @param docs don't work correctly. Just use @inheritDoc and skip @param docs.

srnagar · 2020-01-11T00:12:53Z

sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java

@@ -36,6 +35,7 @@
 */
 public class ClientLogger {
    private final Logger logger;
+    private final boolean isFromEnv;


I don't think you need this anymore.

sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java

alzimmermsft · 2020-01-13T17:33:25Z

sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java

@@ -36,6 +35,7 @@
 */
 public class ClientLogger {
    private final Logger logger;
+    private final boolean isFromEnv;


Is this being used by anything other than the instantiation in the constructor? If not let's remove it.

alzimmermsft · 2020-01-13T17:35:02Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+        String logLevelStr = Configuration.getGlobalConfiguration().get(AZURE_LOG_LEVEL);
+        LogLevel currentLogLevel = LogLevel.fromString(logLevelStr);


Should this logic be abstracted into LoggingUtils or as a private static method on this class?

Have a private helper method in class. Env log level is only needed for default logger, so it is ok to leave it here for current use.

alzimmermsft · 2020-01-13T17:37:01Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+        String dateTime = getFormattedDate();
+        String threadName = Thread.currentThread().getName();
+        String levelName = level.name();
+        StringBuilder buf = new StringBuilder(32);


Any reason to instantiate the StringBuilder with an internal capacity of 32? Instead could it be instantiated with a capacity large enough to hold the initial string being written to it?

It is inherited from simple logger.
The message we know definitely will be printed out is:
2020-01-09 12:35 [main] [Informational]

This is 39 character long. I can put 64 instead of 32 characters to minimize capacity increase.

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

srnagar · 2020-01-13T18:43:18Z

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

  <suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="ClientLogger.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="DefaultLogger.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="DefaultLoggerBase.java"/>


This line can now be removed as DefaultLoggerBase does not exist.

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

mitchdenny · 2020-01-14T01:06:20Z

/check-enforcer reset

JonathanGiles

Even if the functionality in DefaultLogger is unused, it is trivial to implement and should be done now rather than when we get bug reports from customers.

JonathanGiles · 2020-01-14T19:50:32Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+     * {@inheritDoc}
+     */
+    @Override
+    public void trace(final String msg, final Throwable t) {


Why overload all of these methods to throw an exception when the base class has an actual useful implementation? Look at https://github.com/qos-ch/slf4j/blob/master/slf4j-api/src/main/java/org/slf4j/helpers/MarkerIgnoringBase.java

…r-java into addlogimpl

mitchdenny · 2020-01-14T22:33:24Z

/check-enforcer reset

mitchdenny · 2020-01-14T22:33:34Z

/check-enforcer evaluate

srnagar · 2020-01-15T19:56:48Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java

+     * Construct DefaultLogger for the given class name.
+     *
+     * @param className Class name creating the logger.
+     * @throws RuntimeException it is an error.


Will this throw an exception now that you have a catch block?

Not logger throw an exception. Good catch. Will update the JavaDoc

srnagar · 2020-01-15T19:57:39Z

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

@@ -153,6 +153,7 @@

  <!-- Any code in any package, it should never be a 'throw' keyword in the client library codebase except for in the client logger -->
  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files=".*[/\\]com[/\\]azure[/\\]core[/\\]util[/\\]logging[/\\]*"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files=".*[/\\]com[/\\]azure[/\\]core[/\\]implementation[/\\]logging[/\\]*"/>


Now that all methods are implemented in DefaultLogger this exception is no longer required.

mssfang · 2020-01-16T12:47:56Z

sdk/core/azure-core/src/test/java/com/azure/core/util/logging/ClientLoggerTests.java

@@ -144,7 +133,7 @@ public void onlyLogExceptionMessage() {
     */
    @Test
    public void logExceptionStackTrace() {
-        String logMessage = "This is an exception";
+        String logMessage = "This is an exception fdsafdafdomcklamfd fdsafdafmlkdfmalsf fdsafdcacdalmd";


Is this an error message?

sima-zhu requested review from alzimmermsft, hemanttanwar, jianghaolu, JonathanGiles, mssfang and srnagar as code owners January 9, 2020 02:43

mssfang reviewed Jan 9, 2020

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java Outdated Show resolved Hide resolved

mssfang reviewed Jan 9, 2020

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java Outdated Show resolved Hide resolved

alzimmermsft reviewed Jan 9, 2020

View reviewed changes

sima-zhu requested review from mssfang and alzimmermsft January 9, 2020 19:16

alzimmermsft reviewed Jan 9, 2020

View reviewed changes

srnagar reviewed Jan 9, 2020

View reviewed changes

srnagar reviewed Jan 10, 2020

View reviewed changes

Address comments

ae1e40a

sima-zhu force-pushed the addlogimpl branch from 3931f36 to ae1e40a Compare January 10, 2020 17:30

sima-zhu requested review from alzimmermsft and srnagar January 10, 2020 17:40

alzimmermsft reviewed Jan 10, 2020

View reviewed changes

Unified logger APIs

cd8b2be

sima-zhu requested a review from alzimmermsft January 10, 2020 22:44

sima-zhu added 2 commits January 10, 2020 15:12

Fixed failed tests

e484e10

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

75bde91

…into addlogimpl

srnagar reviewed Jan 13, 2020

View reviewed changes

alzimmermsft reviewed Jan 13, 2020

View reviewed changes

Addressed comments

f3673e2

sima-zhu requested review from alzimmermsft and srnagar January 13, 2020 18:35

Some more changes

5f0f82b

alzimmermsft approved these changes Jan 13, 2020

View reviewed changes

Remove some extra methods

f1a75a7

srnagar approved these changes Jan 13, 2020

View reviewed changes

Remove redundant checkstyle suppression

343ab84

mssfang reviewed Jan 13, 2020

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/implementation/logging/DefaultLogger.java Outdated Show resolved Hide resolved

Update DefaultLogger.java

49be4b3

srnagar mentioned this pull request Jan 14, 2020

Implement the rest APIs of DefaultLogger #7341

Closed

JonathanGiles requested changes Jan 14, 2020

View reviewed changes

JonathanGiles reviewed Jan 14, 2020

View reviewed changes

sima-zhu added 3 commits January 14, 2020 13:02

Implements all APIs

7258f76

Merge branch 'addlogimpl' of https://github.com/sima-zhu/azure-sdk-fo…

7145d10

…r-java into addlogimpl

Fixed linting issue

a0e582c

sima-zhu requested a review from JonathanGiles January 14, 2020 22:27

srnagar approved these changes Jan 15, 2020

View reviewed changes

Address feedback

68085b4

sima-zhu requested a review from alzimmermsft January 15, 2020 20:48

alzimmermsft approved these changes Jan 15, 2020

View reviewed changes

JonathanGiles approved these changes Jan 16, 2020

View reviewed changes

sima-zhu merged commit 6b2c11e into Azure:master Jan 16, 2020

mssfang reviewed Jan 16, 2020

View reviewed changes

sima-zhu deleted the addlogimpl branch July 13, 2020 16:51

		StringWriter sw = new StringWriter();
		PrintWriter pw = new PrintWriter(sw);

		String logLevelStr = Configuration.getGlobalConfiguration().get(AZURE_LOG_LEVEL);
		LogLevel currentLogLevel = LogLevel.fromString(logLevelStr);

Implements a slf4j logger impl as default logger in azure core #7298

Implements a slf4j logger impl as default logger in azure core #7298

Conversation

sima-zhu commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny commented Jan 14, 2020

JonathanGiles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny commented Jan 14, 2020

mitchdenny commented Jan 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment