diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/GoodLoggingCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/GoodLoggingCheck.java
new file mode 100644
index 000000000000..682c60f9e83f
--- /dev/null
+++ b/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:
+ *
+ * - A non-static instance logger.
+ * - 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.
+ * - 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.
+ *
+ */
+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 classNameDeque = new ArrayDeque<>();
+ // Collection of Invalid logging packages
+ private static final Set 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);
+ }
+ }
+}
diff --git a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml
index c5603ef049c5..bff7bc036418 100755
--- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml
+++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml
@@ -98,7 +98,7 @@
-
@@ -106,7 +106,7 @@
files=".*[/\\]implementation[/\\].*\.java"/>
-
@@ -139,4 +139,11 @@
+
+
+
+
+
+
+
diff --git a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml
index bac305e17308..a487d1a71916 100755
--- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml
+++ b/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 -->
+
+
+
+
diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HostPolicy.java b/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HostPolicy.java
index 71576d93cbb5..d3431a66e3f5 100644
--- a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HostPolicy.java
+++ b/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 process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
- LOGGER.info("Setting host to {}", host);
+ logger.info("Setting host to {}", host);
Mono result;
final UrlBuilder urlBuilder = UrlBuilder.parse(context.httpRequest().url());
diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/PortPolicy.java b/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/PortPolicy.java
index f585187067c7..f182b04a7b90 100644
--- a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/PortPolicy.java
+++ b/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 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());
diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/ProtocolPolicy.java b/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/ProtocolPolicy.java
index 393e51221c6a..343b0b520d7a 100644
--- a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/ProtocolPolicy.java
+++ b/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 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());
diff --git a/sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/MSIToken.java b/sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/MSIToken.java
index 56673f63a28c..44b9798b2a9c 100644
--- a/sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/MSIToken.java
+++ b/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));