-
Notifications
You must be signed in to change notification settings - Fork 461
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
When not retrying a request, log reason at info level #1189
Conversation
This patch follows up #1187 by having `shouldRetry` return a second string value when no retry should be attempted which contains a short informational message as to why there should be no retry. A caller can use this for logging/informational purposes to make the retry system less opaque.
&http.Response{}, | ||
int(MaxNetworkRetries), | ||
)) | ||
t.Run("DontRetryOnExceededRetries", func(t *testing.T) { |
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.
I also refactored this test case to use subtests, which is a pretty typical pattern in Go. This has a few advantages:
- Subtests run inside their own closure
func ... { ... }
so variables are localized to them, which means variables can't leak into other subtests and keeps things generally cleaner. - Go tooling allows us to run any specific subtest with
-name
so we can target exactly one of them without having to run every single one.
int(MaxNetworkRetries), | ||
)) | ||
t.Run("DontRetryOnExceededRetries", func(t *testing.T) { | ||
shouldRetry, _ := c.shouldRetry( |
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.
I'm not checking against the returned message because:
- The boolean is by far the more important value.
- Messages aren't super likely to regress in an important way.
- Not checking the messages means that it's easier for us to change one because we don't have to update the test case as well.
return false, "context deadline exceeded" | ||
default: | ||
return false, fmt.Sprintf("unknown context error: %v", req.Context().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.
This is really nice.
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.
LGTM 🚢
Thank you Chris! |
Released as 72.6.0. |
* try test fixes * only run unit tests * add logs to locker test * try smth * run all tests * test * test * disable wait on played back recordings * fix test translator * fix revenue contract * reset ci * remove accidnetal changes from test_evergreen_order * remove accidental puts * move env in translator to before * fix gem audit error * fix bundle audit * fix tc
This patch follows up #1187 by having
shouldRetry
return a secondstring value when no retry should be attempted which contains a short
informational message as to why there should be no retry. A caller can
use this for logging/informational purposes to make the retry system
less opaque.
r? @ctrudeau-stripe For review.
Also cc @remi-stripe who requested this feature.
cc @stripe/api-libraries