Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify Logging within Retry #2048

Merged
merged 7 commits into from Sep 6, 2018

Conversation

@kevinmeredith
Copy link
Contributor

@kevinmeredith kevinmeredith commented Sep 4, 2018

In short, this PR updates logging when considering whether to re-try or not.

https://gitter.im/http4s/http4s?at=5b8ec8fc60f9ee7aa4d980ca prompted this change.

@kevinmeredith kevinmeredith changed the title Modify Logging within Retry WIP - Modify Logging within Retry Sep 4, 2018
@kevinmeredith kevinmeredith force-pushed the kevinmeredith:move-log-message branch from 8a81a63 to df88ef3 Sep 4, 2018
@kevinmeredith kevinmeredith force-pushed the kevinmeredith:move-log-message branch from df88ef3 to 51f59b0 Sep 4, 2018
@kevinmeredith kevinmeredith changed the title WIP - Modify Logging within Retry Modify Logging within Retry Sep 4, 2018
@kevinmeredith kevinmeredith force-pushed the kevinmeredith:move-log-message branch from 51f59b0 to 8ab9cc4 Sep 4, 2018
@@ -41,9 +41,12 @@ object Retry {
case Some(duration) =>
// info instead of error(e), because e is not discarded
logger.info(
s"Request $req threw an exception on attempt #$attempts attempts. Giving up.")
s"Request $req threw an exception on attempt #$attempts with message: ${e.getMessage}. Retrying after $duration.")

This comment has been minimized.

@rossabaker

rossabaker Sep 4, 2018
Member

I think we should log the traces here. It could get noisy, but if we don't, we'll never see the discarded exception. (Line 42 should read "because we're trying again" and not "because e is discarded")

This comment has been minimized.

@kevinmeredith

kevinmeredith Sep 4, 2018
Author Contributor

Hi @rossabaker - I don't entirely follow. Could you please say more as to what changes you're recommending/stating? Thanks

This comment has been minimized.

@rossabaker

rossabaker Sep 4, 2018
Member

logger.info(e)(s"Request threw an exception on attempt #$attempts. Retrying after $duration")

Without that, all we get is ${e.getMessage}, which very often is null? (What? Where? Who knows!) When we pass (e) to logger.info(e), we can diagnose the intermittent failures that resolve themselves.

This comment has been minimized.

@kevinmeredith

kevinmeredith Sep 4, 2018
Author Contributor

Thanks for the explanation! I added this change in c794efa.

Credit: Ross Baker suggested this change.
Copy link
Member

@rossabaker rossabaker left a comment

After you're done, make sure to run test:scalafmt, and this should be ready.

@@ -40,8 +40,7 @@ object Retry {
policy(req, Left(e), attempts) match {
case Some(duration) =>
// info instead of error(e), because e is not discarded
logger.info(
s"Request $req threw an exception on attempt #$attempts with message: ${e.getMessage}. Retrying after $duration.")
logger.info(e)(s"Request threw an exception on attempt #$attempts. Retrying after $duration")

This comment has been minimized.

@rossabaker

rossabaker Sep 5, 2018
Member

Would change the line above, too: log e because it's discarded, but at info level because we're trying again.

This comment has been minimized.

@kevinmeredith

kevinmeredith Sep 5, 2018
Author Contributor

Good point. I'll change it too. Could you please explain:

log e because it's discarded

?

I don't understand this statement.

Thanks

@jmcardon
Copy link
Member

@jmcardon jmcardon commented Sep 6, 2018

👍 on CI pass

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

👍 on green after formatted.

@rossabaker rossabaker merged commit 5cc96e4 into http4s:release-0.18.x Sep 6, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants