Skip to content

Commit

Permalink
Use More Static Loggers in Core Libraries (Azure#27339)
Browse files Browse the repository at this point in the history 
Use Static Loggers in Core Libraries
  • Loading branch information
@alzimmermsft
alzimmermsft authored Mar 1, 2022
1 parent d766ce2 commit 457609b
Show file tree
Hide file tree
Showing 74 changed files with 341 additions and 337 deletions.
35 changes: 1 addition & 34 deletions ...ode-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/GoodLoggingCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ public class GoodLoggingCheck extends AbstractCheck {
TokenTypes.CLASS_DEF,
TokenTypes.LITERAL_NEW,
TokenTypes.VARIABLE_DEF,
TokenTypes.METHOD_CALL,
TokenTypes.METHOD_DEF
TokenTypes.METHOD_CALL
};

static final String STATIC_LOGGER_ERROR = "Use a static ClientLogger instance in a static method.";
Expand Down Expand Up @@ -117,9 +116,6 @@ public void visitToken(DetailAST ast) {
log(ast, String.format(NOT_CLIENT_LOGGER_ERROR, "Java System", CLIENT_LOGGER_PATH, methodCallName));
}
break;
case TokenTypes.METHOD_DEF:
checkForInvalidStaticLoggerUsage(ast);
break;
default:
// Checkstyle complains if there's no default block in switch
break;
Expand Down Expand Up @@ -185,33 +181,4 @@ private void checkLoggerNameMatch(DetailAST varToken) {
log(varToken, String.format(LOGGER_NAME_ERROR, LOGGER, identAST.getText()));
}
}

/**
* Report error if a static ClientLogger instance used in a non-static method.
*
* @param methodDefToken METHOD_DEF AST node
*/
private void checkForInvalidStaticLoggerUsage(DetailAST methodDefToken) {

// if not a static method
if (!(TokenUtil.findFirstTokenByPredicate(methodDefToken,
node -> node.branchContains(TokenTypes.LITERAL_STATIC)).isPresent())) {

// error if static `LOGGER` present, LOGGER.*
if (methodDefToken.findFirstToken(TokenTypes.SLIST) != null) {
TokenUtil.forEachChild(methodDefToken.findFirstToken(TokenTypes.SLIST), TokenTypes.EXPR, exprToken -> {
if (exprToken != null) {
DetailAST methodCallToken = exprToken.findFirstToken(TokenTypes.METHOD_CALL);
if (methodCallToken != null && methodCallToken.findFirstToken(TokenTypes.DOT) != null) {
if (methodCallToken.findFirstToken(TokenTypes.DOT)
.findFirstToken(TokenTypes.IDENT).getText().equals(LOGGER.toUpperCase())) {
log(methodDefToken, STATIC_LOGGER_ERROR);
}
}
}
});
}
}
}

}
8 changes: 0 additions & 8 deletions ...-quality-reports/src/test/java/com/azure/tools/checkstyle/checks/GoodLoggerCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,6 @@ public void invalidLoggerNameTestData() throws Exception {
verify(checker, getPath("InvalidLoggerNameTestData.java"), expected);
}

@Test
public void nonStaticLoggerTestData() throws Exception {
String[] expected = {
expectedErrorMessage(9, 5, STATIC_LOGGER_ERROR)
};
verify(checker, getPath("NonStaticLoggerTestData.java"), expected);
}

@Test
public void wrongClassInLoggerConstructorTestData() throws Exception {
String[] expected = {
Expand Down
10 changes: 5 additions & 5 deletions sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/AmqpRetryOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*/
@Fluent
public class AmqpRetryOptions {
private final ClientLogger logger = new ClientLogger(AmqpRetryOptions.class);
private static final ClientLogger LOGGER = new ClientLogger(AmqpRetryOptions.class);

private int maxRetries;
private Duration delay;
Expand Down Expand Up @@ -69,7 +69,7 @@ public AmqpRetryOptions setMode(AmqpRetryMode retryMode) {
*/
public AmqpRetryOptions setMaxRetries(int numberOfRetries) {
if (numberOfRetries < 0) {
throw logger.logExceptionAsError(new IllegalArgumentException("'numberOfRetries' cannot be negative."));
throw LOGGER.logExceptionAsError(new IllegalArgumentException("'numberOfRetries' cannot be negative."));
}

this.maxRetries = numberOfRetries;
Expand All @@ -88,7 +88,7 @@ public AmqpRetryOptions setMaxRetries(int numberOfRetries) {
public AmqpRetryOptions setDelay(Duration delay) {
Objects.requireNonNull(delay, "'delay' cannot be null.");
if (delay.isNegative() || delay.isZero()) {
throw logger.logExceptionAsError(new IllegalArgumentException("'delay' must be positive."));
throw LOGGER.logExceptionAsError(new IllegalArgumentException("'delay' must be positive."));
}

this.delay = delay;
Expand All @@ -107,7 +107,7 @@ public AmqpRetryOptions setDelay(Duration delay) {
public AmqpRetryOptions setMaxDelay(Duration maximumDelay) {
Objects.requireNonNull(maximumDelay, "'maximumDelay' cannot be null.");
if (maximumDelay.isNegative() || maximumDelay.isZero()) {
throw logger.logExceptionAsError(new IllegalArgumentException("'maximumDelay' must be positive."));
throw LOGGER.logExceptionAsError(new IllegalArgumentException("'maximumDelay' must be positive."));
}

this.maxDelay = maximumDelay;
Expand All @@ -126,7 +126,7 @@ public AmqpRetryOptions setMaxDelay(Duration maximumDelay) {
public AmqpRetryOptions setTryTimeout(Duration tryTimeout) {
Objects.requireNonNull(tryTimeout, "'tryTimeout' cannot be null");
if (tryTimeout.isNegative() || tryTimeout.isZero()) {
throw logger.logExceptionAsError(new IllegalArgumentException("'tryTimeout' must be positive."));
throw LOGGER.logExceptionAsError(new IllegalArgumentException("'tryTimeout' must be positive."));
}

this.tryTimeout = tryTimeout;
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/ProxyOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class ProxyOptions implements AutoCloseable {
*/
public static final String PROXY_PASSWORD = "PROXY_PASSWORD";

private final ClientLogger logger = new ClientLogger(ProxyOptions.class);
private static final ClientLogger LOGGER = new ClientLogger(ProxyOptions.class);
private final PasswordAuthentication credentials;
private final Proxy proxyAddress;
private final ProxyAuthenticationType authentication;
Expand Down Expand Up @@ -62,7 +62,7 @@ public ProxyOptions(ProxyAuthenticationType authentication, Proxy proxyAddress,
if (username != null && password != null) {
this.credentials = new PasswordAuthentication(username, password.toCharArray());
} else {
logger.info("Username or password is null. Using system-wide authentication.");
LOGGER.info("Username or password is null. Using system-wide authentication.");
this.credentials = null;
}
}
Expand Down
30 changes: 15 additions & 15 deletions ...-core-amqp/src/main/java/com/azure/core/amqp/implementation/ActiveClientTokenManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
* Manages the re-authorization of the client to the token audience against the CBS node.
*/
public class ActiveClientTokenManager implements TokenManager {
private final ClientLogger logger = new ClientLogger(ActiveClientTokenManager.class);
private static final ClientLogger LOGGER = new ClientLogger(ActiveClientTokenManager.class);
private final AtomicBoolean hasScheduled = new AtomicBoolean();
private final AtomicBoolean hasDisposed = new AtomicBoolean();
private final Mono<ClaimsBasedSecurityNode> cbsNode;
Expand Down Expand Up @@ -79,14 +79,14 @@ public Mono<Long> authorize() {

// If this is the first time authorize is called, the task will not have been scheduled yet.
if (!hasScheduled.getAndSet(true)) {
logger.atInfo()
LOGGER.atInfo()
.addKeyValue("scopes", scopes)
.log("Scheduling refresh token task.");

final Duration firstInterval = Duration.ofMillis(refreshIntervalMS);
lastRefreshInterval.set(firstInterval);
authorizationResults.emitNext(AmqpResponseCode.ACCEPTED, (signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult).log("Could not emit ACCEPTED.");
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult).log("Could not emit ACCEPTED.");
return false;
});

Expand All @@ -104,12 +104,12 @@ public void close() {
}

authorizationResults.emitComplete((signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult).log("Could not close authorizationResults.");
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult).log("Could not close authorizationResults.");

return false;
});
durationSource.emitComplete((signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult).log("Could not close durationSource.");
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult).log("Could not close durationSource.");

return false;
});
Expand All @@ -122,7 +122,7 @@ public void close() {
private Disposable scheduleRefreshTokenTask(Duration initialRefresh) {
// EmitterProcessor can queue up an initial refresh interval before any subscribers are received.
durationSource.emitNext(initialRefresh, (signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult).log("Could not emit initial refresh interval.");
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult).log("Could not emit initial refresh interval.");

return false;
});
Expand All @@ -131,7 +131,7 @@ private Disposable scheduleRefreshTokenTask(Duration initialRefresh) {
.takeUntil(duration -> hasDisposed.get())
.flatMap(delay -> {

logger.atInfo()
LOGGER.atInfo()
.addKeyValue("scopes", scopes)
.log("Refreshing token.");

Expand All @@ -142,27 +142,27 @@ private Disposable scheduleRefreshTokenTask(Duration initialRefresh) {
(amqpException, interval) -> {
final Duration lastRefresh = lastRefreshInterval.get();

logger.atError()
LOGGER.atError()
.addKeyValue("scopes", scopes)
.addKeyValue(INTERVAL_KEY, interval)
.log("Error is transient. Rescheduling authorization task.", amqpException);

durationSource.emitNext(lastRefresh, (signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult)
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult)
.addKeyValue("lastRefresh", lastRefresh)
.log("Could not emit lastRefresh.");

return false;
});
})
.subscribe(interval -> {
logger.atVerbose()
LOGGER.atVerbose()
.addKeyValue("scopes", scopes)
.addKeyValue(INTERVAL_KEY, interval)
.log("Authorization successful. Refreshing token.");

authorizationResults.emitNext(AmqpResponseCode.ACCEPTED, (signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult)
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult)
.log("Could not emit ACCEPTED after refresh.");
return false;
});
Expand All @@ -171,14 +171,14 @@ private Disposable scheduleRefreshTokenTask(Duration initialRefresh) {
lastRefreshInterval.set(nextRefresh);

durationSource.emitNext(nextRefresh, (signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult)
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult)
.addKeyValue("nextRefresh", nextRefresh)
.log("Could not emit nextRefresh.");

return false;
});
}, error -> {
logger.atError()
LOGGER.atError()
.addKeyValue("scopes", scopes)
.addKeyValue("audience", tokenAudience)
.log("Error occurred while refreshing token that is not retriable. Not scheduling"
Expand All @@ -188,14 +188,14 @@ private Disposable scheduleRefreshTokenTask(Duration initialRefresh) {
if (!hasDisposed.getAndSet(true)) {
hasScheduled.set(false);
durationSource.emitComplete((signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult)
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult)
.log("Could not close durationSource.");

return false;
});

authorizationResults.emitError(error, (signalType, emitResult) -> {
addSignalTypeAndResult(logger.atVerbose(), signalType, emitResult)
addSignalTypeAndResult(LOGGER.atVerbose(), signalType, emitResult)
.log("Could not emit authorization error.", error);

return false;
Expand Down
6 changes: 3 additions & 3 deletions ...zure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpExceptionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
* Handles exceptions generated by AMQP connections, sessions, and/or links.
*/
abstract class AmqpExceptionHandler {
private final ClientLogger logger = new ClientLogger(AmqpExceptionHandler.class);
private static final ClientLogger LOGGER = new ClientLogger(AmqpExceptionHandler.class);

/**
* Creates a new instance of the exception handler.
Expand All @@ -26,7 +26,7 @@ abstract class AmqpExceptionHandler {
* @param exception The exception that caused the connection error.
*/
void onConnectionError(Throwable exception) {
logger.warning("Connection exception encountered.", exception);
LOGGER.warning("Connection exception encountered.", exception);
}

/**
Expand All @@ -35,6 +35,6 @@ void onConnectionError(Throwable exception) {
* @param shutdownSignal The shutdown signal that was received.
*/
void onConnectionShutdown(AmqpShutdownSignal shutdownSignal) {
addShutdownSignal(logger.atInfo(), shutdownSignal).log("Shutdown received");
addShutdownSignal(LOGGER.atInfo(), shutdownSignal).log("Shutdown received");
}
}
6 changes: 3 additions & 3 deletions ...core-amqp/src/main/java/com/azure/core/amqp/implementation/AzureTokenManagerProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
public class AzureTokenManagerProvider implements TokenManagerProvider {
static final String TOKEN_AUDIENCE_FORMAT = "amqp://%s/%s";

private final ClientLogger logger = new ClientLogger(AzureTokenManagerProvider.class);
private static final ClientLogger LOGGER = new ClientLogger(AzureTokenManagerProvider.class);
private final CbsAuthorizationType authorizationType;
private final String fullyQualifiedNamespace;
private final String activeDirectoryScope;
Expand Down Expand Up @@ -48,7 +48,7 @@ public TokenManager getTokenManager(Mono<ClaimsBasedSecurityNode> cbsNodeMono, S
final String scopes = getScopesFromResource(resource);
final String tokenAudience = String.format(Locale.US, TOKEN_AUDIENCE_FORMAT, fullyQualifiedNamespace, resource);

logger.atVerbose()
LOGGER.atVerbose()
.addKeyValue("audience", tokenAudience)
.addKeyValue("resource", resource)
.log("Creating new token manager.");
Expand All @@ -66,7 +66,7 @@ public String getScopesFromResource(String resource) {
} else if (CbsAuthorizationType.SHARED_ACCESS_SIGNATURE.equals(authorizationType)) {
return String.format(Locale.US, TOKEN_AUDIENCE_FORMAT, fullyQualifiedNamespace, resource);
} else {
throw logger.logExceptionAsError(new IllegalArgumentException(String.format(Locale.US,
throw LOGGER.logExceptionAsError(new IllegalArgumentException(String.format(Locale.US,
"'%s' is not supported authorization type for token audience.", authorizationType)));
}
}
Expand Down
4 changes: 3 additions & 1 deletion ...e/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ConnectionOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
*/
@Immutable
public class ConnectionOptions {
private static final ClientLogger LOGGER = new ClientLogger(ConnectionOptions.class);

private final TokenCredential tokenCredential;
private final AmqpTransportType transport;
private final AmqpRetryOptions retryOptions;
Expand Down Expand Up @@ -243,7 +245,7 @@ private static int getPort(AmqpTransportType transport) {
case AMQP_WEB_SOCKETS:
return WebSocketsConnectionHandler.HTTPS_PORT;
default:
throw new ClientLogger(ConnectionOptions.class).logThrowableAsError(
throw LOGGER.logThrowableAsError(
new IllegalArgumentException("Transport Type is not supported: " + transport));
}
}
Expand Down
6 changes: 3 additions & 3 deletions ...ore-amqp/src/main/java/com/azure/core/amqp/implementation/ConnectionStringProperties.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* The set of properties that comprise a connection string from the Azure portal.
*/
public class ConnectionStringProperties {
private final ClientLogger logger = new ClientLogger(ConnectionStringProperties.class);
private static final ClientLogger LOGGER = new ClientLogger(ConnectionStringProperties.class);

private static final String TOKEN_VALUE_SEPARATOR = "=";
private static final String ENDPOINT_SCHEME_SB_PREFIX = "sb://";
Expand Down Expand Up @@ -105,7 +105,7 @@ public ConnectionStringProperties(String connectionString) {
if (endpoint == null
|| (includesSharedKey && includesSharedAccessSignature) // includes both SAS and key or value
|| (!hasSharedKeyAndValue && !includesSharedAccessSignature)) { // invalid key, value and SAS
throw logger.logExceptionAsError(new IllegalArgumentException(ERROR_MESSAGE_FORMAT));
throw LOGGER.logExceptionAsError(new IllegalArgumentException(ERROR_MESSAGE_FORMAT));
}

this.endpoint = endpoint;
Expand Down Expand Up @@ -163,7 +163,7 @@ public String getSharedAccessSignature() {
private String validateAndUpdateDefaultScheme(final String endpoint) {

if (CoreUtils.isNullOrEmpty(endpoint)) {
throw logger.logExceptionAsError(new IllegalArgumentException(
throw LOGGER.logExceptionAsError(new IllegalArgumentException(
"'Endpoint' must be provided in 'connectionString'."));
}

Expand Down
Loading

0 comments on commit 457609b

Please sign in to comment.