feat(libs/emails): Add retry with backoff for email-sender #19798
Conversation
nshirley
force-pushed
the
FXA-12591
branch
2 times, most recently
from
b18fb68 to
b0b6425
Compare
| retryWithBackoff<T>( | ||
| fn: () => Promise<T>, | ||
| { maxRetries: maxAttempts, backoffMs }: MailerConfig['retry'], | ||
| { template }: Pick<Email, 'template'> |
There was a problem hiding this comment.
Just realized, I don't love this, having to pass in the email for the template value which is just used in the statsd. I want to keep the Pick<> in case template becomes a stronger type, like a union type. But also, do we already have something to do this backing off, from a library we're already importing maybe?
But, I'm unsure a better way to do this outside of just hoisting the logic back into the sendMail function, but then that'll start getting bloated.
| doc: 'Always ignore retries for these email templates', | ||
| format: Array, | ||
| default: [ | ||
| /** No default for now */ |
There was a problem hiding this comment.
May not be necessary, but I figured there will be emails we never want to have retry if send fails. Maybe something like MFA codes?
| * @param fn The promise to retry | ||
| */ | ||
| retryWithBackoff<T>( | ||
| fn: () => Promise<T>, |
There was a problem hiding this comment.
TIL: this has to be a factory function you can't pass in just a Promise - with the latter, the loop will have reference to the initial promise, which once resolved or rejected will continue to return that value instead of executing again. With the factory approach, the loop gets a new instance of the promise each time, thus "making" the request again each time!
Because: - We want to enable retrying an email send if the send fails This Commit: - Adds a new retryWithBackoff for accounts-email-sender - Adds configuration to change maxRetries, backoffMs, and ignoreTempaltes - Adds tests for new functionality Closes: FXA-12591
| this.statsd.increment('email.send.retry', { | ||
| template, | ||
| }); | ||
| setTimeout(execute, backoffMs * Math.pow(2, attempts - 1)); |
There was a problem hiding this comment.
Should we have some kind of jitter in here, even a small amount so we're not always backing off a static amount?
Because:
This Commit:
Closes: FXA-12591
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.