CustomCheckStyleRules: Good Logging Practice

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

@@ -0,0 +1,181 @@
+// 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.ArrayDeque;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Deque;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Good Logging Practice:
+ * <ol>
+ * <li>A non-static instance logger.</li>
+ * <li>ClientLogger in public API should all named 'logger', public API classes are those classes that are declared
+ *     as public and that do not exist in an implementation package or subpackage.</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 = "logger";
+
+    private static final String LOGGER_NAME_ERROR = "ClientLogger instance naming: use ''%s'' instead of ''%s'' for consistency.";
+    private static final String STATIC_LOGGER_ERROR = "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''.";
+
+    // Boolean indicator that indicates if the java class imports ClientLogger
+    private boolean hasClientLoggerImported;
+    // A container stores the class names, pop top element if exist the class name AST node
+    private Deque<String> classNameDeque = new ArrayDeque<>();
+    // Collection of Invalid logging packages
+    private static final Set<String> INVALID_LOGS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
+        "org.slf4j", "org.apache.logging.log4j", "java.util.logging"
+    )));
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] {
+            TokenTypes.IMPORT,
+            TokenTypes.CLASS_DEF,
+            TokenTypes.LITERAL_NEW,
+            TokenTypes.VARIABLE_DEF,
+            TokenTypes.METHOD_CALL
+        };
+    }
+
+    @Override
+    public void finishTree(DetailAST ast) {
+        hasClientLoggerImported = false;
+    }
+
+    @Override
+    public void leaveToken(DetailAST ast) {
+        if (ast.getType() == TokenTypes.CLASS_DEF) {
+            classNameDeque.pollFirst();
+        }
+    }
+
+    @Override
+    public void visitToken(DetailAST ast) {
+        switch (ast.getType()) {
+            case TokenTypes.IMPORT:
+                final String importClassPath = FullIdent.createFullIdentBelow(ast).getText();
+                hasClientLoggerImported = hasClientLoggerImported || importClassPath.equals(CLIENT_LOGGER_PATH);
+
+                INVALID_LOGS.forEach(item -> {
+                    if (importClassPath.startsWith(item)) {
+                        log(ast, String.format(NOT_CLIENT_LOGGER_ERROR, "external logger", CLIENT_LOGGER_PATH, item));
+                    }
+                });
+                break;
+            case TokenTypes.CLASS_DEF:
+                classNameDeque.addFirst(ast.findFirstToken(TokenTypes.IDENT).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) {
+        final 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 ClientLogger instance
+        if (identToken == null || !identToken.getText().equals(CLIENT_LOGGER)) {
+            return;
+        }
+        // LITERAL_NEW node always has ELIST node below
+        TokenUtil.findFirstTokenByPredicate(literalNewToken.findFirstToken(TokenTypes.ELIST), exprToken -> {
+            // Skip check if not EXPR node or if has no DOT node below. EXPR always has children below
+            if (exprToken.getType() != TokenTypes.EXPR || exprToken.getFirstChild().getType() != TokenTypes.DOT) {
+                return false;
+            }
+            // Check instantiation of ClientLogger
+            final String containerClassName = FullIdent.createFullIdent(exprToken.getFirstChild()).getText();
+            // Add suffix of '.class' at the end of class name
+            final String className = classNameDeque.peekFirst();
+            if (!containerClassName.equals(className + ".class")) {
+                log(exprToken, String.format("Not newing a ClientLogger with matching class name. Use ''%s.class'' instead of ''%s''", className, containerClassName));
+            }
+            return true;
+        });
+    }
+
+    /**
+     * 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,15 +98,15 @@
   <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 -->
   <suppress checks="com\.azure\.tools\.checkstyle\.checks\.(ExternalDependencyExposedCheck|NoImplInPublicAPI|ServiceClientInstantiationCheck|ServiceClientBuilderCheck|ServiceInterfaceCheck|JavaDocFormatting|JavadocThrowsChecks|EnforceFinalFieldsCheck|ThrowFromClientLoggerCheck)"
             files=".*[/\\]implementation[/\\].*\.java"/>
 
   <!-- Don't apply custom Checkstyle rules to files under samples package -->
-  <suppress checks="com\.azure\.tools\.checkstyle\.checks\.(ExternalDependencyExposedCheck|HttpPipelinePolicyCheck|EnforceFinalFieldsCheck|ThrowFromClientLoggerCheck)"
+  <suppress checks="com\.azure\.tools\.checkstyle\.checks\.(ExternalDependencyExposedCheck|HttpPipelinePolicyCheck|EnforceFinalFieldsCheck|ThrowFromClientLoggerCheck|GoodLoggingCheck)"
             files=".*[/\\]samples[/\\].*\.java"/>
 
   <!-- Don't check for JavaDocPackage in samples or tests -->
@@ -139,4 +139,11 @@
   <!-- CodeSnippet Suppression for now, which need code owner's attention -->
   <suppress checks="com.azure.tools.checkstyle.checks.JavadocCodeSnippetCheck" files="com.azure.data.appconfiguration.ConfigurationAsyncClient.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.JavadocCodeSnippetCheck" files="com.azure.messaging.eventhubs.EventData.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

@@ -351,5 +351,15 @@ page at http://checkstyle.sourceforge.net/config.html -->
     2) Methods arguments should be concatenated by dash '-'. Ex. string-string  for methodName(String s, String s2)
     3) Use '#' to concatenate 1) and 2), ex packageName.className.methodName#string-string -->
     <module name="com.azure.tools.checkstyle.checks.JavadocCodeSnippetCheck"/>
+
+    <!--CUSTOM CHECKS-->
+    <!-- Good Logging Practice
+    1) ClientLogger in public API should all named 'logger', public API classes are those classes that are declared
+       as public and that do not exist in an implementation package or subpackage.
+    2) ClientLogger should be non-static instance logger
+    3) Should not use any external logger class, only use ClientLogger. No slf4j, log4j, or other logging imports are allowed.
+      'System.out' and 'System.err' is not allowed as well.
+    4) All classes should use ClientLogger as logger only but except ClientLogger itself -->
+    <module name="com.azure.tools.checkstyle.checks.GoodLoggingCheck"/>
   </module>
 </module>

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));