Exponential backoff if Sentry server returns 429

[benvinegar]

cc @MaxBittker @LewisJEllis @mattrobenolt @JTCunning

[benvinegar]

Should there be a max duration, e.g. 2 minutes or something?

[mattrobenolt]

I'd suggest that we console.log or something that this is happening. That way we're not dropping things in the dark without any insight as to why if someone were to debug. Including ourselves.

[mattrobenolt]

imo we can do similar, if not the same behavior for a few different status codes. Not just 429.

We have cases of someone revoking a DSN, in which case SDKs will continue to spam the old key. Or the project is deleted entirely, with code still out in production continuing to spam. In theory, we could be more aggressive about these errors and just 100% shut down the SDK, but that might just be more complexity in code base.

• 400 status would be if the project_id didn't exist or some other fatal error. This would be safe to abort entirely or back off. This is not recoverable without re-configuring the client to fix the problem.
• 401 status is for unauthorized, and this is when the project id would match, but the DSN is invalid. So either the credentials are flat out wrong, or they have been revoked. This would be safe to abort entirely or back off. This is not recoverable without re-configuring the client to fix the problem.
• 403 status is overloaded for a couple reasons, one of them being Event dropped due to filter, so we wouldn't want to change behavior for this.
• 429, obvious. That's why we're here. But one thing worth noting, in lots of cases, we spit back a Retry-After header that would be applicable to use as a backoff time, if it's easy to read this. The response will say Retry-After: 10 to say, "chill for 10 seconds". If you can read this, it'd be preferable over backing off since this is an accurate time for when the rate limit is going to expire. If the header doesn't exist, exponential backoff is fine.

And on that note, I'm pretty sure raven-js now can be re-configured at runtime to change the DSN. If so, I think this reconfiguring should reset this backoff and start fresh. Since a new DSN likely would have a different outcome.

Just my thoughts on this. :)

[mattrobenolt]

X-Sentry-Error was added too, so you can attempt to fetch this and expose this as a "reason" for debugging and logging.

[benvinegar]

What typically appears there?

[mattrobenolt]

The actual reason.

So an example string would be like:

Invalid project_id: 10

or

Invalid api key

[benvinegar]

Okay. That could be useful for completely different reasons.

(Does Retry-After say something about rate limiting? Or completely independent?)

[mattrobenolt]

(Does Retry-After say something about rate limiting? Or completely independent?)

This is only related to rate limiting and won't exist on any other response as of today.

[benvinegar]

I moved the back off check to run after filters have run ... because, if you were going to filter out an error anyways (e.g. whitelistUrls or shouldSendCallback), you don't need to know that it would have been suppressed from the back off state.

[benvinegar]

@mattrobenolt – can you look at this again? I think I've addressed all your feedback (except X-SentryError, which I'd like to follow up with separately).

[mattrobenolt]

Looks good to me, but someone who knows js better than me should give final approval.

[mattrobenolt]

(Does Retry-After say something about rate limiting? Or completely independent?)

This is only related to rate limiting and won't exist on any other response as of today.

[MaxBittker]

looks great

[JTCunning]

I'm going to look at this PR in about 3 years and be very glad we did this.

Thanks <3