Skip to content
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

Lower the default send_attempts option to 3 #760

Merged
merged 1 commit into from
Feb 1, 2019

Conversation

stayallive
Copy link
Collaborator

Because we use the RetryPlugin to handle the send attempts we should consider lowering this since it has an exponential backoff and with the 6 retries I think (if I'm reading it correctly) the total time it tries to send a request (ignoring the time it takes to fail the request) is about 32 seconds which is way to long (since default request timeout in php is 30 seconds).

Retry backoff calculation converted to seconds:

>>> (pow(2, 0) * 500000) / 1000000
=> 0.5
>>> (pow(2, 1) * 500000) / 1000000
=> 1
>>> (pow(2, 2) * 500000) / 1000000
=> 2
>>> (pow(2, 3) * 500000) / 1000000
=> 4
>>> (pow(2, 4) * 500000) / 1000000
=> 8
>>> (pow(2, 5) * 500000) / 1000000
=> 16

The 3,5 seconds backoff with 3 retries seems way more sensible to me considering the request also takes some time to actually timeout or generate an error.

I'm also not sure what the current request timeout even is since we no longer have a option for that it seems... might need to limit that to give a 30s PHP request timeout a chance to cleanly exit after submitting an event to Sentry fails.

@ste93cry
Copy link
Collaborator

ste93cry commented Feb 1, 2019

I'm also not sure what the current request timeout even is since we no longer have a option for that it seems

We don't have this option anymore since tt doesn't makes sense to have HTTP-related options if the transport can be changed to something else than that. Actually probably we could just drop this option too for the same reason along with the usage of the RetryPlugin, and if an user wants this feature he can configure its own Httplug transport and set the options as he prefers

@stayallive stayallive merged commit bf9184f into master Feb 1, 2019
@stayallive stayallive deleted the lower_send_attempts branch February 1, 2019 15:06
@stayallive
Copy link
Collaborator Author

It depends, if the universal SDK requirements expect the SDK's to retry a few times we should leave it in, if it's not required we should indeed remove it and document on how to implement it yourself.

Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO lowering the retry and leaving it as it is is a good middle ground.

Maybe we should document this default?

@mfb
Copy link
Contributor

mfb commented Feb 1, 2019

How do we setup "fire and forget" behavior? Set send attempts to 0? (Personally, I prefer "fire and forget" because if something is wrong with your system, waiting and then repeatedly trying to send exception to external service can actually make things worse.)

@stayallive
Copy link
Collaborator Author

stayallive commented Feb 1, 2019

I agree, and setting it to 0 should indeed disable the retrying to get to that behaviour.

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

Successfully merging this pull request may close these issues.

5 participants