Skip to content

Conversation

@flimzy
Copy link
Contributor

@flimzy flimzy commented Dec 20, 2016

This fixes #31 .

Additional discussion may be warranted before a merge, though, in case the default behavior should possibly be changed.

As of now, I simply expose a hook.Retries field, which can be set to non-zero to enable retries. Otherwise, behavior remains the same as before.

This in preparation for running in a loop to handle retries.
@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+1.004%) to 83.702% when pulling 9cd97e9 on flimzy:retry into 3d7f059 on evalphobia:master.

@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+1.004%) to 83.702% when pulling b37bcdd on flimzy:retry into 3d7f059 on evalphobia:master.

@mcpherrinm
Copy link

It seems to me that you'd want to do some kind of exponential backoff here. If Sentry thinks it's overloaded, hammering it repeatedly seems like the wrong thing to do.

@evalphobia
Copy link
Owner

@flimzy Thank you for contributing. And sorry for my lateness.
429 error is the kind of error for rate limit, right?

As @mcpherrinm mentioned, I think it needs some waiting time to guarantee the effectiveness.
For example, HTTP library h2non/gentleman has plugin for retry and it implements waiting time to retry.
It use millisec scale, but Sentry's rate limit seems calcutlates the events on each minute, so the scale should be more than seconds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support retries in case of 429 response

4 participants