Azure / azure-sdk-for-java

This repository is for active development of the Azure SDK for Java. For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/java/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-java.
MIT License
2.25k stars 1.93k forks source link

Lower levels of two log statements #40833

Open Blackbaud-JasonBodnar opened 6 days ago

Blackbaud-JasonBodnar commented 6 days ago

Description

Lower the levels of two logging statements. According to the Log4J docs WARN should be used for "an event that might possible lead to an error". Starting up a client is not an event that could lead to an error.

All SDK Contribution checklist:

General Guidelines and Best Practices

Testing Guidelines

github-actions[bot] commented 6 days ago

Thank you for your contribution @Blackbaud-JasonBodnar! We will review the pull request and get back to you soon.

kushagraThapar commented 6 days ago

Thanks @Blackbaud-JasonBodnar for this PR, while it makes sense to treat this log as info, however, I think the reason this was put as warn is because of the importance of client creation event. We have seen customers switching off the info logs in their application and ends up creating multiple cosmos clients without noticing it. That's why this was changed from info to warn here - https://github.com/Azure/azure-sdk-for-java/pull/39169

@FabianMeiswinkel please provide more information on this change if the above doesn't make sense!

Blackbaud-JasonBodnar commented 6 days ago

Thanks @Blackbaud-JasonBodnar for this PR, while it makes sense to treat this log as info, however, I think the reason this was put as warn is because of the importance of client creation event. We have seen customers switching off the info logs in their application and ends up creating multiple cosmos clients without noticing it. That's why this was changed from info to warn here - #39169

@FabianMeiswinkel please provide more information on this change if the above doesn't make sense!

But you are breaking standards. WARN should only be used for potential problems. If there is an issue with multiple clients being created then that should be logged as WARN.

Organizations take WARN and ERROR messages very seriously often alerting employees when too many are seen during a time window. My organization reviews our WARN and ERROR messages every day and these messages just obscure real messages that need to looked into.

Looking at the commit you refer to I see in one file:

  private def logInfoOrWarning(msg: => String): Unit = {
    if (this.verboseLoggingAfterReEnqueueingRetriesEnabled.get()) {
      log.logWarning(msg)
    } else {
      log.logInfo(msg)
    }
  }

Why wasn't something similar used for the CosmosClientBuilder? Or better yet, log it at the appropriate INFO or DEBUG level and when there is an issue tell customers to set their logging appropriately so they can see these messages. That's why we have different log levels and it is so easy to change the level of what is logged for each instance.