Skip to content

Commit

Permalink
Fixed Inappropriate Logging and String.format Usage (#22450)
Browse files Browse the repository at this point in the history
Fix Improper Usage of ClientLogger and String Format
  • Loading branch information
alzimmermsft authored Jun 22, 2021
1 parent 8197bd0 commit 23260e7
Show file tree
Hide file tree
Showing 45 changed files with 161 additions and 166 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ public void visitToken(DetailAST token) {
// logs error if the @Fluent method has 'throws' at the method declaration.
if (token.findFirstToken(TokenTypes.LITERAL_THROWS) != null) {
log(token, String.format(
"Fluent Method ''%s'' must not be declared to throw any checked exceptions"));
"Fluent Method ''%s'' must not be declared to throw any checked exceptions.",
token.findFirstToken(TokenTypes.IDENT).getText()));
}
break;
default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@ private static ConfigurationSetting read(JsonNode node) {
return readSecretReferenceConfigurationSetting(node, baseSetting);
}
} catch (Exception exception) {
LOGGER.info(String.format(
"The setting is neither a 'FeatureFlagConfigurationSetting' nor 'SecretReferenceConfigurationSetting'"
+ ", return the setting as 'ConfigurationSetting', error is ", exception));
LOGGER.info("The setting is neither a 'FeatureFlagConfigurationSetting' nor "
+ "'SecretReferenceConfigurationSetting', return the setting as 'ConfigurationSetting'. "
+ "Error: ", exception);
}
return baseSetting;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ public void addEvent(String eventName, Map<String, Object> traceEventAttributes,

Span currentSpan = Span.current();
if (currentSpan == null) {
logger.info("Failed to find a starting span to associate the %s with.", eventName);
logger.info("Failed to find a starting span to associate the {} with.", eventName);
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineN
context.getHttpRequest().setUrl(urlBuilder.setScheme(protocol).toUrl());
} catch (MalformedURLException e) {
return Mono.error(new RuntimeException(
String.format("Failed to set the HTTP request protocol to %d.", protocol), e));
String.format("Failed to set the HTTP request protocol to %s.", protocol), e));
}
}
return next.process();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ private boolean usePublicSetter(Object deserializedHeaders, ClientLogger logger)
Method setterMethod = deserializedHeaders.getClass().getDeclaredMethod(potentialSetterName, Map.class);
if (Modifier.isPublic(setterMethod.getModifiers())) {
setterMethod.invoke(deserializedHeaders, values);
logger.verbose("User setter %s on class %s to set header collection.", potentialSetterName,
logger.verbose("User setter {} on class {} to set header collection.", potentialSetterName,
deserializedHeaders.getClass().getSimpleName());
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,7 @@ public static <T> T parse(String itemResponseBodyAsString, Class<T> itemClassTyp
return getSimpleObjectMapper().readValue(itemResponseBodyAsString, itemClassType);
} catch (IOException e) {
throw new IllegalStateException(
String.format("Failed to parse string [%s] to POJO.", itemResponseBodyAsString, e));
String.format("Failed to parse string [%s] to POJO.", itemResponseBodyAsString), e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,15 +140,15 @@ private static boolean isSameCollection(PartitionKeyRange initiallyResolved, Par
*
* @param request Request in progress
* @param targetRange Target partition key range determined by address resolver
* @*/
* */
private void throwIfTargetChanged(RxDocumentServiceRequest request, PartitionKeyRange targetRange) {
// If new range is child of previous range, we don't need to throw any exceptions
// as LSNs are continued on child ranges.
if (request.requestContext.resolvedPartitionKeyRange != null &&
!isSameCollection(request.requestContext.resolvedPartitionKeyRange, targetRange)) {
if (!request.getIsNameBased()) {
String message = String.format(
"Target should not change for non name based requests. Previous target {}, Current {}",
"Target should not change for non name based requests. Previous target %s, Current %s",
request.requestContext.resolvedPartitionKeyRange, targetRange);
assert false : message;
logger.warn(message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ public void queryOrderByMixedTypes(String sortOrder) throws Exception {
// Ensure its a cross partition query
assertThat(partitionKeyRanges.size()).isGreaterThan(1);
// We are inserting documents with int, float, string, array, object and missing propMixed.
String query = String.format("SELECT * FROM r ORDER BY r.propMixed ", sortOrder);
String query = "SELECT * FROM r ORDER BY r.propMixed ";
CosmosQueryRequestOptions options = new CosmosQueryRequestOptions();
List<String> sourceIds = createdDocuments.stream()
.map(Resource::getId)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ Mono<Response<Iterable<DigitalTwinsModelData>>> createModelsWithResponse(Iterabl
modelsPayload.add(mapper.readValue(model, Object.class));
}
catch (JsonProcessingException e) {
logger.error("Could not parse the model payload [%s]: %s", model, e);
logger.error("Could not parse the model payload [{}]: {}", model, e);
return Mono.error(e);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ public void sendBatchPartitionKeyValidate() throws InterruptedException {
logger.info("Event[{}] matched. Countdown: {}", event.getSequenceNumber(), countDownLatch.getCount());
countDownLatch.countDown();
} else {
logger.warning(String.format("Event[%s] matched partition key, but not GUID. Expected: %s. Actual: %s",
event.getSequenceNumber(), messageValue, event.getProperties().get(MESSAGE_ID)));
logger.warning("Event[{}] matched partition key, but not GUID. Expected: {}. Actual: {}",
event.getSequenceNumber(), messageValue, event.getProperties().get(MESSAGE_ID));
}
}, error -> {
Assertions.fail("An error should not have occurred:" + error.toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public void testEventProcessorBuilderMissingProperties() {
+ "sequence number of event = " + eventContext.getEventData().getSequenceNumber());
})
.processError(errorContext -> {
System.out.printf("Error occurred in partition processor for partition {}, {}",
System.out.printf("Error occurred in partition processor for partition %s, %s%n",
errorContext.getPartitionContext().getPartitionId(),
errorContext.getThrowable());
})
Expand All @@ -69,7 +69,7 @@ public void testEventProcessorBuilderWithProcessEvent() {
+ "sequence number of event = " + eventContext.getEventData().getSequenceNumber());
})
.processError(errorContext -> {
System.out.printf("Error occurred in partition processor for partition %s, %s",
System.out.printf("Error occurred in partition processor for partition %s, %s%n",
errorContext.getPartitionContext().getPartitionId(),
errorContext.getThrowable());
})
Expand Down Expand Up @@ -98,7 +98,7 @@ public void testEventProcessorBuilderWithBothSingleAndBatchConsumers() {
});
}, 5, Duration.ofSeconds(1))
.processError(errorContext -> {
System.out.printf("Error occurred in partition processor for partition {}, {}",
System.out.printf("Error occurred in partition processor for partition %s, %s%n",
errorContext.getPartitionContext().getPartitionId(),
errorContext.getThrowable());
})
Expand All @@ -113,7 +113,7 @@ public void testEventProcessorBuilderWithNoProcessEventConsumer() {
.checkpointStore(new SampleCheckpointStore())
.consumerGroup("consumer-group")
.processError(errorContext -> {
System.out.printf("Error occurred in partition processor for partition {}, {}",
System.out.printf("Error occurred in partition processor for partition %s, %s%n",
errorContext.getPartitionContext().getPartitionId(),
errorContext.getThrowable());
})
Expand All @@ -134,7 +134,7 @@ public void testEventProcessorBuilderWithProcessEventBatch() {
});
}, 5, Duration.ofSeconds(1))
.processError(errorContext -> {
System.out.printf("Error occurred in partition processor for partition %s, %s",
System.out.printf("Error occurred in partition processor for partition %s, %s%n",
errorContext.getPartitionContext().getPartitionId(),
errorContext.getThrowable());
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,7 @@ protected void dispose(Closeable... closeables) {
try {
closeable.close();
} catch (IOException error) {
logger.error(String.format("[%s]: %s didn't close properly.", testName,
closeable.getClass().getSimpleName()), error);
logger.error("[{}]: {} didn't close properly.", testName, closeable.getClass().getSimpleName(), error);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ public Mono<AccessToken> getToken(TokenRequestContext request) {
+ "The Target Azure platform could not be determined from environment variables.")));
}
return managedIdentityServiceCredential.authenticate(request)
.doOnSuccess((t -> logger.info(String.format("Azure Identity => Managed Identity environment: %s",
managedIdentityServiceCredential.getEnvironment()))))
.doOnSuccess(t -> logger.info("Azure Identity => Managed Identity environment: {}",
managedIdentityServiceCredential.getEnvironment()))
.doOnNext(token -> LoggingUtil.logTokenSuccess(logger, request))
.doOnError(error -> LoggingUtil.logTokenError(logger, request, error));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,8 @@ Mono<PagedResponse<KeyVaultRoleDefinition>> listRoleDefinitionsFirstPage(String
context.addData(AZ_TRACING_NAMESPACE_KEY, KEYVAULT_TRACING_NAMESPACE_VALUE))
.doOnRequest(ignored -> logger.verbose("Listing role definitions for roleScope - {}", roleScope))
.doOnSuccess(response -> logger.verbose("Listed role definitions for roleScope - {}", roleScope))
.doOnError(error ->
logger.warning(String.format("Failed to list role definitions for roleScope - %s", roleScope),
error))
.doOnError(error -> logger.warning("Failed to list role definitions for roleScope - {}", roleScope,
error))
.onErrorMap(KeyVaultAdministrationUtils::mapThrowableToKeyVaultAdministrationException)
.map(KeyVaultAccessControlAsyncClient::transformRoleDefinitionsPagedResponse);
} catch (RuntimeException e) {
Expand Down Expand Up @@ -633,9 +632,8 @@ Mono<PagedResponse<KeyVaultRoleAssignment>> listRoleAssignmentsFirstPage(String
context.addData(AZ_TRACING_NAMESPACE_KEY, KEYVAULT_TRACING_NAMESPACE_VALUE))
.doOnRequest(ignored -> logger.verbose("Listing role assignments for roleScope - {}", roleScope))
.doOnSuccess(response -> logger.verbose("Listed role assignments for roleScope - {}", roleScope))
.doOnError(error ->
logger.warning(String.format("Failed to list role assignments for roleScope - %s", roleScope),
error))
.doOnError(error -> logger.warning("Failed to list role assignments for roleScope - {}", roleScope,
error))
.onErrorMap(KeyVaultAdministrationUtils::mapThrowableToKeyVaultAdministrationException)
.map(KeyVaultAccessControlAsyncClient::transformRoleAssignmentsPagedResponse);
} catch (RuntimeException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,7 @@ private Mono<PagedResponse<CertificateProperties>> listCertificateVersionsFirstP
context.addData(AZ_TRACING_NAMESPACE_KEY, KEYVAULT_TRACING_NAMESPACE_VALUE))
.doOnRequest(ignored -> logger.verbose("Listing certificate versions - {}", certificateName))
.doOnSuccess(response -> logger.verbose("Listed certificate versions - {}", certificateName))
.doOnError(error -> logger.warning(String.format("Failed to list certificate versions - {}", certificateName), error));
.doOnError(error -> logger.warning("Failed to list certificate versions - {}", certificateName, error));
} catch (RuntimeException ex) {
return monoError(logger, ex);
}
Expand Down Expand Up @@ -1454,7 +1454,7 @@ private Mono<PagedResponse<IssuerProperties>> listPropertiesOfIssuersFirstPage(C
context.addData(AZ_TRACING_NAMESPACE_KEY, KEYVAULT_TRACING_NAMESPACE_VALUE))
.doOnRequest(ignored -> logger.verbose("Listing certificate issuers - {}"))
.doOnSuccess(response -> logger.verbose("Listed certificate issuers - {}"))
.doOnError(error -> logger.warning(String.format("Failed to list certificate issuers - {}"), error));
.doOnError(error -> logger.warning("Failed to list certificate issuers - {}", error));
} catch (RuntimeException ex) {
return monoError(logger, ex);
}
Expand Down Expand Up @@ -1580,7 +1580,7 @@ private Mono<PagedResponse<CertificateContact>> setCertificateContactsWithRespon
context.addData(AZ_TRACING_NAMESPACE_KEY, KEYVAULT_TRACING_NAMESPACE_VALUE))
.doOnRequest(ignored -> logger.verbose("Listing certificate contacts - {}"))
.doOnSuccess(response -> logger.verbose("Listed certificate contacts - {}"))
.doOnError(error -> logger.warning(String.format("Failed to list certificate contacts - {}"), error));
.doOnError(error -> logger.warning("Failed to list certificate contacts - {}", error));
}

/**
Expand Down Expand Up @@ -1615,7 +1615,7 @@ private Mono<PagedResponse<CertificateContact>> listCertificateContactsFirstPage
context.addData(AZ_TRACING_NAMESPACE_KEY, KEYVAULT_TRACING_NAMESPACE_VALUE))
.doOnRequest(ignored -> logger.verbose("Listing certificate contacts - {}"))
.doOnSuccess(response -> logger.verbose("Listed certificate contacts - {}"))
.doOnError(error -> logger.warning(String.format("Failed to list certificate contacts - {}"), error));
.doOnError(error -> logger.warning("Failed to list certificate contacts - {}", error));
} catch (RuntimeException ex) {
return monoError(logger, ex);
}
Expand Down Expand Up @@ -1652,7 +1652,7 @@ private Mono<PagedResponse<CertificateContact>> deleteCertificateContactsWithRes
context.addData(AZ_TRACING_NAMESPACE_KEY, KEYVAULT_TRACING_NAMESPACE_VALUE))
.doOnRequest(ignored -> logger.verbose("Deleting certificate contacts - {}"))
.doOnSuccess(response -> logger.verbose("Deleted certificate contacts - {}"))
.doOnError(error -> logger.warning(String.format("Failed to delete certificate contacts - {}"), error));
.doOnError(error -> logger.warning("Failed to delete certificate contacts - {}", error));
}

/**
Expand Down
Loading

0 comments on commit 23260e7

Please sign in to comment.