[SDK-4224] Allow configuration and disabling of retry strategy #216
Conversation
| domain, | ||
| management.WithClientCredentials(context.Background(), id, secret), | ||
| management.WithNoRetries(), |
There was a problem hiding this comment.
I understand it might be a bit late for this, but with this design it would be possible to use both WithRetries() and WithNoRetries() –even though the last used method would prevail. What about simply using management.WithRetries(RetryOptions{MaxRetries: 0}) ?
There was a problem hiding this comment.
(instead of WithNoRetries())
There was a problem hiding this comment.
Consider as well using factory methods if necessary, to reduce the verbosity and make the intent clearer.
There was a problem hiding this comment.
Also, what is the SDK already doing with other similar configuration options?
There was a problem hiding this comment.
Yeah I was a little undecided on this and initially had an Enabled property on the RetryOptions struct.
We currently have WithNoAuth0ClientInfo to disable the Auth0Client telemetry so that was part of the reason in going for WithNoRetries for this API
There was a problem hiding this comment.
Note that this being a new major version, it's an opportunity to go with the approach that makes the most sense for all methods involved (including WithNoAuth0ClientInfo), specially since that will set the pattern for any future API additions once the major is out. It's alright if you want to use WithNoRetries() anyway though.
There was a problem hiding this comment.
I think given that we'd most likely have keep WithNoAuth0ClientInfo (i.e. adding an Enabled flag to that struct isn't as straight forward), then I'd lean towards maintaining the existing WithX/WithNoX pattern as it does fit with Go
| var redirectsErrorRe = regexp.MustCompile(`stopped after \d+ redirects\z`) | ||
|
|
||
| // Matches the error returned by net/http when the TLS certificate is not trusted. | ||
| // When version 1.20 is the minimum supported this can be replaced by tls.CertificateVerificationError. |
There was a problem hiding this comment.
Note that if you're making a new major version it is acceptable to have it support only Go 1.20+. Specially since Go 1.21 is expected for August 2023, a mere 3 months away, which will drop support for 1.19.
There was a problem hiding this comment.
Yeah I think that's a fair shout, I'll maybe circle back to dropping 1.19 after checking one some of the users of this package have their compile target.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## beta #216 +/- ##
==========================================
- Coverage 95.17% 94.99% -0.18%
==========================================
Files 37 37
Lines 7192 7254 +62
==========================================
+ Hits 6845 6891 +46
- Misses 276 295 +19
+ Partials 71 68 -3
☔ View full report in Codecov by Sentry. |
This improves upon the existing retry logic implemented in the package by expanding the retry conditions, allowing configuration of the retry conditions and allowing disabling retries. By default the Management Client will retry a request 2 times, when there is a 429 status code or if there is an error returned (except for a select couple of errors). The configuration of the max retries and status codes that will be retried on by providing a `RetryOptions` struct with the required values with the `WithRetryOptions` function, and retries can be disabled by passing the `WithNoRetries` struct.
…nd retry behaviour
Co-authored-by: Rita Zerrizuela <zeta@widcket.com>
Co-authored-by: Rita Zerrizuela <zeta@widcket.com>
ewanharris
force-pushed
the
feat/SDK-4224-retry-improvements
branch
from
7ba548e to
3e9cc2e
Compare
🔧 Changes
This improves upon the existing retry logic implemented in the package by expanding the retry
conditions, allowing configuration of the retry conditions and allowing disabling retries.
By default the Management Client will retry a request 2 times, when there is a 429 status code
or if there is an error returned (except for a select couple of errors that are not recoverable). The
X-RateLimit-Resetis consulted to ensure that the request will work, unless the time until the request is more than the maximum wait time.The configuration of the max retries and status codes that will be retried on by providing a
RetryOptionsstruct with the required values with theWithRetryOptionsfunction, and retriescan be disabled by passing the
WithNoRetriesfunction📚 References
🔬 Testing
📝 Checklist
The SDK will now retry a request a maximum of 2 times by default, and will also retry on certain errors.