reduce log level on requests that will not be retried #1031
Conversation
This log entry generates errors on multiple common patterns that should not result in the SDK generating error logs. As an example, the recommended method for checking if a blob exists is to read it's properties. If a blob does not exist, it generates a 404 response, which causes an error here. Given the SDK returns an error, this should be left up to the SDK consumer if this warrants logging an error.
| @@ -101,7 +101,7 @@ where | |||
| ); | |||
| error | |||
| } else { | |||
| log::error!("non-io error occurred which will not be retried: {}", error); | |||
| log::debug!("non-io error occurred which will not be retried: {}", error); | |||
There was a problem hiding this comment.
The change above makes a lot of sense. But the errors here are all non-HTTP errors. Wouldn't logging at an error level still make sense?
|
I am actually wondering if the SDK should log at all in the case where it is returning a As an example, we haven't found the SDK logs to be terribly helpful because we use the |
The caller can trivially filter out the SDKs logs - e.g., log_env allows the user to set which crates they want logs from. I think the SDK should be giving enough information for users of the SDK to have a good idea of what's happening. The logs here give additional information about what is happening inside of the retry policy. It can be helpful to know what the state of the retry policy is. I've personally used these logs to understand which errors were being retried. |
Sorry, I wasn't suggesting that all logging should be removed. Retry state is a really good example of something that is useful because it can present internal state the caller wouldn't otherwise see. I just think that the pattern of logging an |
@gorzell identifies the specific concern I was trying to address with this PR. My thought is that logging an error condition and returning an Error with the same detail isn't needed. The |
|
I believe that the logs do encode additional information. The user won't necessarily be familiar with the workings on the retry policy, and thus they may not know that for example, a 419 does not get retried by a 503 does. These logs alert the user that an error has occurred (something they may already know) which will not be retried (potentially new information). |
|
Would it be an acceptable compromise to add that information as context to the error such that the control as to what to do with the information is in the hands of the consumer, including acting on the issue? |
Improving the information/context in the Error would be my strong preference for providing additional information to the caller (when possible/practical). |
I suppose so? I still don't quite understand why this log is problematic. It provides additional context in a way that can trivially filtered from logs if users don't want that level of detail. The fact that the error won't be retried isn't really context about the error but rather context to understand how your operation is being processed. It makes more sense to me that it would be logged and not attached to the error, but I'm obviously missing something here 😊 |
This log entry generates errors on multiple common patterns that should not result in the SDK generating error logs.
As an example, the recommended method for checking if a blob exists is to read it's properties. If a blob does not exist, it generates a 404 response, which causes an error here.
Given the SDK returns an error, this should be left up to the SDK consumer if these warrants logging an error.