-
Notifications
You must be signed in to change notification settings - Fork 199
fix agent retry
on connection level failures
#623
Conversation
@@ -107,7 +115,7 @@ pub async fn send_retry_reqwest< | |||
max_elapsed_time: Some(max_elapsed_time), | |||
..ExponentialBackoff::default() | |||
}, | |||
|err, _| println!("Transient error: {}", err), | |||
|err, _| warn!("transient http error: {}", err), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add the url in the message here it might be helpful when debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the URL here isn't easily accessible. At this context, we have a Builder, which we'd have to TryClone to get access into the URL.
I agree that it would be valuable. Perhaps a follow-up can refactor this to make it accessible?
Hello @bmc-msft! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 18 hours, a condition that will be fulfilled in about 16 minutes. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
In debugging the connection retry issues, I dug into this more.
Apparently, some of hyper's connection errors are not mapped to std::io::Error, rendering the existing downcast impl ineffective.
As such, this PR makes the following updates:
reqwest
calls aconnection
error is considered transient.warn
macro such that the events show up in application insights.http://localhost:81/
, which shouldn't be listening on any system.Fixes #263