MONEI / Shopify-api-node

Node Shopify connector sponsored by MONEI
https://monei.com/shopify-payment-gateway/
MIT License
945 stars 278 forks source link

Add an option for automatic retries when hitting rate limits #541

Closed airhorns closed 2 years ago

airhorns commented 2 years ago

This adds opt-in retry logic to calls made to REST or GraphQL endpoints on Shopify. Many users of the Shopify API run into the restrictive rate limits, and there's one simple way you can work around them, which is to retry the request after the time has elapsed. This isn't the only strategy, but it's a really common one because it doesn't require any cross-process coordination or state. Instead of trying to guess before hand if a request is going to work or not (which is hard to do if you have many different processes on many different servers all making requests at the same time), this will just always try the request, and keep retrying for a while until it succeeds.

This approach uses Got's built in retry configuration option, so shopify-api-node doesn't have to implement any of the custom backoff logic or actual retry code, and instead just has to compute the time to wait before retrying which is kind of nice.

I think Shopify's restrictive rate limits burn just about every user of the Shopify APIs really soon after they get started using it -- the limits are pretty much the lowest in the business, and are a big part of working with the Shopify API. It's rare in my experience that you don't need to care about rate limiting, and so I think it'd be really great to have a safe-by-default rate limiting work around built into this library since many many users need something. I agree that a rate limit error is technically speaking a user error, but, why force every user to work around this error with bespoke code copied from a github issue? It's so common that I think it's worth building in. This approach is simple and robust (and used in the hosted Shopify sync product I'm working on), and I'd love for everyone else using this lib to not have to reinvent this every time. I took the time to try to make this good quality code that's tested to ease the maintenance burden as well.

I didn't turn on this behaviour by default so as to not make a breaking change, but for what it's worth, I think I will always turn it on when using this library in the future. We could consider turning it on by default in the next major version of shopify-api-node, but that's up to the maintainers of course!

Closes #251, references #324 and #199 and #377

lpinca commented 2 years ago

This should be mutually exclusive with the autoLimit option as it does not address https://github.com/MONEI/Shopify-api-node/issues/251#issuecomment-487965284. Also retrying on 5xx and network errors is questionable.

I would prefer something simpler where only 429 errors are retried if the autoLimit option is not set, similarly to how it is done for the 202 responses.

airhorns commented 2 years ago

This should be mutually exclusive with the autoLimit option as it does not address https://github.com/MONEI/Shopify-api-node/issues/251#issuecomment-487965284.

Ah right! I will update to throw in the constructor if both options are engaged.

Also retrying on 5xx and network errors is questionable.

Can you explain your hesitance a bit more here? A 500 error from Shopify is Shopify telling the client that Shopify is at fault and the client didn't do anything wrong. I think that means means that there's really nothing to do but retry. In my experience Shopify throws transient errors like this all the time and it seems quite convenient to not have to wrap shopify-api-node in yet some other retry library to catch one class of errors. Is there something you think users should do with those errors other than retry? Also there's prior art for this, got's default is 2 retries and automatic retrying of 5xx series responses as well as network errors: https://github.com/sindresorhus/got/blob/main/documentation/7-retry.md#retry . Also fwiw, here's what we see in production across all our shopify apps for shopify response codes -- enough 503s in there that this makes sense to me:

CleanShot 2022-06-22 at 14 05 58@2x
coveralls commented 2 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 2effa667786dca938e19218c2f5c3aaf23f947d5 on gadget-inc:built-in-retries into cbf9e10f1a0ff2c1e24863040038a45b8c5bc1c0 on MONEI:master.

lpinca commented 2 years ago

Thank you.