CustomCheckStyleRules: Good Logging Practice

eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/GoodLoggingCheck.java

@@ -0,0 +1,167 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.azure.tools.checkstyle.checks;
+
+import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
+import com.puppycrawl.tools.checkstyle.api.DetailAST;
+import com.puppycrawl.tools.checkstyle.api.FullIdent;
+import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+import com.puppycrawl.tools.checkstyle.utils.TokenUtil;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Good Logging Practice:
+ * <ol>
+ * <li>Non-static Logger instance.</li>
+ * <li>Logger variable name should be 'logger' in public class and not exist in an implementation package.</li>
+ * <li>Should not use any external logger class, only use ClientLogger. No slf4j, log4j, or other logging imports are allowed.</li>
+ * <li>'System.out' and 'System.err' is not allowed as well.</li>
+ * </ol>
+ */
+public class GoodLoggingCheck extends AbstractCheck {
+    private static final String CLIENT_LOGGER_PATH = "com.azure.core.util.logging.ClientLogger";
+    private static final String CLIENT_LOGGER = "ClientLogger";
+    private static final String LOGGER_NAME_ERROR = "ClientLogger instance naming: use ''%s'' instead of ''%s'' for consistency.";
+    private static final String STATIC_LOGGER_ERROR = "Reference to ClientLogger should not be static: remove static modifier.";
+    private static final String NOT_CLIENT_LOGGER_ERROR = "Do not use %s class. Use ''%s'' as a logging mechanism instead of ''%s''.";
+    private static final String LOGGER = "logger";
+
+    private boolean hasClientLoggerImported;
+    private String className;
+
+    private static final Set<String> INVALID_LOG_SET = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
+        "org.slf4j", "org.apache.logging.log4j"
+    )));
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] {
+            TokenTypes.IMPORT,
+            TokenTypes.LITERAL_CLASS,
+            TokenTypes.LITERAL_NEW,
+            TokenTypes.VARIABLE_DEF,
+            TokenTypes.METHOD_CALL
+        };
+    }
+
+    @Override
+    public void finishTree(DetailAST ast) {
+        hasClientLoggerImported = false;
+    }
+
+    @Override
+    public void visitToken(DetailAST ast) {
+        switch (ast.getType()) {
+            case TokenTypes.IMPORT:
+                String importClassPath = FullIdent.createFullIdentBelow(ast).getText();
+                hasClientLoggerImported = hasClientLoggerImported || importClassPath.equals(CLIENT_LOGGER_PATH);
+                for (final String logger : INVALID_LOG_SET) {
+                    if (importClassPath.startsWith(logger)) {
+                        // Checks no use any external logger class.
+                        log(ast, String.format(NOT_CLIENT_LOGGER_ERROR, "external logger", CLIENT_LOGGER_PATH, logger));
+                    }
+                }
+                break;
+            case TokenTypes.LITERAL_CLASS:
+                if (ast.getNextSibling() == null) {
+                    break;
+                }
+                className = ast.getNextSibling().getText();
+                break;
+            case TokenTypes.LITERAL_NEW:
+                checkLoggerInstantiation(ast);
+                break;
+            case TokenTypes.VARIABLE_DEF:
+                checkLoggerNameMatch(ast);
+                break;
+            case TokenTypes.METHOD_CALL:
+                final DetailAST dotToken = ast.findFirstToken(TokenTypes.DOT);
+                if (dotToken == null) {
+                    return;
+                }
+                final String methodCallName = FullIdent.createFullIdentBelow(dotToken).getText();
+                if (methodCallName.startsWith("System.out") || methodCallName.startsWith("System.err")) {
+                    log(ast, String.format(NOT_CLIENT_LOGGER_ERROR, "Java System", CLIENT_LOGGER_PATH, methodCallName));
+                }
+                break;
+            default:
+                // Checkstyle complains if there's no default block in switch
+                break;
+        }
+    }
+
+    /**
+     * check if the VARIABLE_DEF AST node type is 'ClientLogger'.
+     * @param varDefAST VARIABLE_DEF AST node
+     * @return true if the variable type is 'ClientLogger'.
+     */
+    private boolean isTypeClientLogger(DetailAST varDefAST) {
+        DetailAST typeAST = varDefAST.findFirstToken(TokenTypes.TYPE);
+        if (typeAST == null) {
+            return false;
+        }
+        return TokenUtil.findFirstTokenByPredicate(typeAST, node ->
+           node.getType() == TokenTypes.IDENT && node.getText().equals(CLIENT_LOGGER)
+        ).isPresent();
+    }
+
+    /**
+     * Check if instantiating a matched class name for the same class.
+     *
+     * @param literalNewToken LITERAL_NEW node
+     */
+    private void checkLoggerInstantiation(DetailAST literalNewToken) {
+        final DetailAST identToken = literalNewToken.findFirstToken(TokenTypes.IDENT);
+        // Not named 'logger'
+        if (identToken == null || !identToken.getText().equals(CLIENT_LOGGER)) {
+            return;
+        }
+        // Edge cases
+        final DetailAST elistToken = literalNewToken.findFirstToken(TokenTypes.ELIST);
+        final DetailAST exprToken = elistToken.findFirstToken(TokenTypes.EXPR);
+        if (exprToken.getFirstChild().getType() != TokenTypes.DOT) {
+            return;
+        }
+        // Check instantiation of ClientLogger
+        final String containerClassName = FullIdent.createFullIdentBelow(exprToken).getText();
+        // Add suffix of '.class' at the end ot class name
+        if (!containerClassName.equals(className + ".class")) {
+            log(literalNewToken, String.format("Not newing a ClientLogger with matching class name. Use ''%s.class'' instead of ''%s.class''", className, containerClassName));
+        }
+    }
+
+    /**
+     * Check if the given ClientLogger named 'logger'
+     *
+     * @param varToken VARIABLE_DEF node
+     */
+    private void checkLoggerNameMatch(DetailAST varToken) {
+        if (!hasClientLoggerImported || !isTypeClientLogger(varToken)) {
+            return;
+        }
+        // Check if the Logger instance named as 'logger'.
+        final DetailAST identAST = varToken.findFirstToken(TokenTypes.IDENT);
+        if (identAST != null && !identAST.getText().equals(LOGGER)) {
+            log(varToken, String.format(LOGGER_NAME_ERROR, LOGGER, identAST.getText()));
+        }
+        // Check if the Logger is static instance, log as error if it is static instance logger.
+        if (TokenUtil.findFirstTokenByPredicate(varToken, node -> node.getType() == TokenTypes.MODIFIERS && node.branchContains(TokenTypes.LITERAL_STATIC)).isPresent()) {
+            log(varToken, STATIC_LOGGER_ERROR);
+        }
+    }
+}

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

@@ -98,7 +98,7 @@
   <suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files=".*\BlobOutputStream.java"/>
 
   <!-- Don't apply custom Checkstyle rules to files under test package -->
-  <suppress checks="com\.azure\.tools\.checkstyle\.checks\.(ExternalDependencyExposedCheck|NoImplInPublicAPI|ServiceClientInstantiationCheck|ServiceClientBuilderCheck|ServiceInterfaceCheck|HttpPipelinePolicyCheck|JavaDocFormatting|JavadocThrowsChecks|EnforceFinalFieldsCheck|ThrowFromClientLoggerCheck)"
+  <suppress checks="com\.azure\.tools\.checkstyle\.checks\.(ExternalDependencyExposedCheck|NoImplInPublicAPI|ServiceClientInstantiationCheck|ServiceClientBuilderCheck|ServiceInterfaceCheck|HttpPipelinePolicyCheck|JavaDocFormatting|JavadocThrowsChecks|EnforceFinalFieldsCheck|ThrowFromClientLoggerCheck|GoodLoggingCheck)"
             files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
 
   <!-- Don't apply custom Checkstyle rules to files under implementation package -->
@@ -130,4 +130,11 @@
   <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.BlobOutputStream.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.BlockBlobClient.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.PageBlobAsyncClient.java"/>
+
+  <!-- ClientLogger class suppression -->
+  <suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="ClientLogger.java"/>
+  <!-- Azure Core Test -->
+  <suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files=".*[/\\]core[/\\]test[/\\].*\.java"/>
+  <!-- Public API already released -->
+  <suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="CertificateUtil.java"/>
 </suppressions>

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

@@ -322,7 +322,7 @@ page at http://checkstyle.sourceforge.net/config.html -->
     <!-- CUSTOM CHECKS -->
     <!-- Verify the whenever a field is assigned just once in constructor to be final -->
     <module name="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck"/>
-    
+
     <!-- Javadoc format: 'param / return / throws' descriptions text should only have one space character after the
      parameter name or return -->
     <module name="com.azure.tools.checkstyle.checks.JavaDocFormatting"/>
@@ -336,5 +336,12 @@ page at http://checkstyle.sourceforge.net/config.html -->
     1) Must be a public class.
     2) Not in an implementation package or sub-package. -->
     <module name="com.azure.tools.checkstyle.checks.HttpPipelinePolicyCheck"/>
+
+    <!--CUSTOM CHECKS-->
+    <!-- Good Logging Practice
+    1) ServiceLogger exists in public API should named 'logger'
+    2) Non-static ServiceLogger instance logger
+    3) All classes should use ServiceLogger as logger only but except ServiceLogger itself -->
+    <module name="com.azure.tools.checkstyle.checks.GoodLoggingCheck"/>
   </module>
 </module>

sdk/core/azure-core-test/src/main/java/com/azure/core/test/http/PlaybackClient.java

@@ -9,8 +9,7 @@
 import com.azure.core.http.ProxyOptions;
 import com.azure.core.test.models.NetworkCallRecord;
 import com.azure.core.test.models.RecordedData;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import com.azure.core.util.logging.ClientLogger;
 import reactor.core.publisher.Mono;
 
 import java.net.URI;
@@ -25,7 +24,7 @@
  * HTTP client that plays back {@link NetworkCallRecord NetworkCallRecords}.
  */
 public final class PlaybackClient implements HttpClient {
-    private final Logger logger = LoggerFactory.getLogger(PlaybackClient.class);
+    private final ClientLogger logger = new ClientLogger(PlaybackClient.class);
     private final AtomicInteger count = new AtomicInteger(0);
     private final Map<String, String> textReplacementRules;
     private final RecordedData recordedData;
@@ -88,10 +87,8 @@ private Mono<HttpResponse> playbackHttpResponse(final HttpRequest request) {
         count.incrementAndGet();
 
         if (networkCallRecord == null) {
-            if (logger.isWarnEnabled()) {
-                logger.warn("NOT FOUND - Method: {} URL: {}", incomingMethod, incomingUrl);
-                logger.warn("Records requested: {}.", count);
-            }
+            logger.warning("NOT FOUND - Method: {} URL: {}", incomingMethod, incomingUrl);
+            logger.warning("Records requested: {}.", count);
 
             return Mono.error(new IllegalStateException("==> Unexpected request: " + incomingMethod + " " + incomingUrl));
         }

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HostPolicy.java

@@ -7,8 +7,8 @@
 import com.azure.core.http.HttpPipelineNextPolicy;
 import com.azure.core.http.HttpResponse;
 import com.azure.core.implementation.http.UrlBuilder;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+
+import com.azure.core.util.logging.ClientLogger;
 import reactor.core.publisher.Mono;
 
 import java.net.MalformedURLException;
@@ -18,7 +18,7 @@
  */
 public class HostPolicy implements HttpPipelinePolicy {
     private final String host;
-    private static final Logger LOGGER = LoggerFactory.getLogger(HostPolicy.class);
+    private final ClientLogger logger = new ClientLogger(HostPolicy.class);
 
     /**
      * Create HostPolicy.
@@ -31,7 +31,7 @@ public HostPolicy(String host) {
 
     @Override
     public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
-        LOGGER.info("Setting host to {}", host);
+        logger.info("Setting host to {}", host);
 
         Mono<HttpResponse> result;
         final UrlBuilder urlBuilder = UrlBuilder.parse(context.httpRequest().url());

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/PortPolicy.java

@@ -7,8 +7,7 @@
 import com.azure.core.http.HttpPipelineNextPolicy;
 import com.azure.core.http.HttpResponse;
 import com.azure.core.implementation.http.UrlBuilder;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import com.azure.core.util.logging.ClientLogger;
 import reactor.core.publisher.Mono;
 
 import java.net.MalformedURLException;
@@ -19,7 +18,7 @@
 public class PortPolicy implements HttpPipelinePolicy {
     private final int port;
     private final boolean overwrite;
-    private static final Logger LOGGER = LoggerFactory.getLogger(PortPolicy.class);
+    private final ClientLogger logger = new ClientLogger(PortPolicy.class);
 
     /**
      * Create a new PortPolicy object.
@@ -36,7 +35,7 @@ public PortPolicy(int port, boolean overwrite) {
     public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
         final UrlBuilder urlBuilder = UrlBuilder.parse(context.httpRequest().url());
         if (overwrite || urlBuilder.port() == null) {
-            LOGGER.info("Changing port to {}", port);
+            logger.info("Changing port to {}", port);
 
             try {
                 context.httpRequest().url(urlBuilder.port(port).toURL());

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/ProtocolPolicy.java

@@ -7,8 +7,7 @@
 import com.azure.core.http.HttpPipelineNextPolicy;
 import com.azure.core.http.HttpResponse;
 import com.azure.core.implementation.http.UrlBuilder;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import com.azure.core.util.logging.ClientLogger;
 import reactor.core.publisher.Mono;
 
 import java.net.MalformedURLException;
@@ -19,7 +18,7 @@
 public class ProtocolPolicy implements HttpPipelinePolicy {
     private final String protocol;
     private final boolean overwrite;
-    private static final Logger LOGGER = LoggerFactory.getLogger(ProtocolPolicy.class);
+    private final ClientLogger logger = new ClientLogger(ProtocolPolicy.class);
 
     /**
      * Create a new ProtocolPolicy.
@@ -36,7 +35,7 @@ public ProtocolPolicy(String protocol, boolean overwrite) {
     public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
         final UrlBuilder urlBuilder = UrlBuilder.parse(context.httpRequest().url());
         if (overwrite || urlBuilder.scheme() == null) {
-            LOGGER.info("Setting protocol to {}", protocol);
+            logger.info("Setting protocol to {}", protocol);
 
             try {
                 context.httpRequest().url(urlBuilder.scheme(protocol).toURL());

sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/MSIToken.java

@@ -69,13 +69,13 @@ private static Long parseDateToEpochSeconds(String dateTime) {
         try {
             return Long.parseLong(dateTime);
         } catch (NumberFormatException e) {
-            System.err.println(e.getMessage());
+            logger.error(e.getMessage());
         }
 
         try {
             return Instant.from(dtf.parse(dateTime)).toEpochMilli() / 1000L;
         } catch (DateTimeParseException e) {
-            System.err.println(e.getMessage());
+            logger.error(e.getMessage());
         }
 
         throw logger.logExceptionAsError(new IllegalArgumentException("Unable to parse date time " + dateTime));

sdk/storage/azure-storage-queue/src/main/java/com/azure/storage/queue/QueueServiceClientBuilder.java

@@ -119,7 +119,7 @@ public QueueServiceAsyncClient buildAsyncClient() {
         Objects.requireNonNull(endpoint);
 
         if (sasTokenCredential == null && sharedKeyCredential == null && bearerTokenCredential == null) {
-            logger.logExceptionAsError(new IllegalArgumentException("Credentials are required for authorization"));
+            throw logger.logExceptionAsError(new IllegalArgumentException("Credentials are required for authorization"));
         }
 
         if (pipeline != null) {
@@ -198,7 +198,7 @@ public QueueServiceClientBuilder endpoint(String endpoint) {
                 this.bearerTokenCredential = null;
             }
         } catch (MalformedURLException ex) {
-            logger.logExceptionAsError(new IllegalArgumentException("The Azure Storage Queue endpoint url is malformed."));
+            throw logger.logExceptionAsError(new IllegalArgumentException("The Azure Storage Queue endpoint url is malformed."));
         }
 
         return this;