search

ConnectCorp / nexmo-client

Unofficial Nexmo Rest Client
MIT License
15 stars 13 forks source link

Added rate limiting to prevent submitting requests to the Nexmo APIs too quickly #12

Closed quinncomendant closed 8 years ago

quinncomendant commented 9 years ago

Nexmo will reject requests if they are submitted too quickly with a "429 Too Many Requests" error. The limit for the Developer API is 3 requests per second, and the SMS and Voice APIs limit at 30 requests per second (except US/Canada, it's 1 request per second). Details at https://help.nexmo.com/hc/en-us/articles/203993598

This commit includes changes to use a RateLimitSubscriber, which is a native hook for the Guzzle HTTP client. It times the previous request, and if it is faster than the minimum to stay below the rate limit, it will sleep long enough so the second request does not violate the rate limit. SMS and Voice requests are timed on a per-LVN basis so the delay will only occur when sending from the same LVN too quickly would trigger the limit, but sending from different LVNs will still be fast. Each service class is given a getRateLimit() method which returns the limit for that specific API endpoint.

vood commented 9 years ago

I'm not sure it should be a part of this lib. Does it make sense to extract it to a separate package?

vood commented 9 years ago

At Connect we have our own wrapper which is responsible for rate limiting. It should be a part of your framework not a part of this particular lib. See for instance http://www.yiiframework.com/doc-2.0/yii-filters-ratelimiter.html

quinncomendant commented 9 years ago

Hello Artem,

It's true there are many ways to implement a rate limiter, and users may want to integrate their own or leverage an existing package.

But what about this idea: no matter what method of rate limiting the user choses, the ultimate goal will always be to avoid exceeding the limits set by Nexmo (e.g., 1/sec for US→US messages). Positing this, if a user has their own rate limit layer, then a rate limiter built into the nexmo-client would never trigger. Or if the user doesn't have their own rate limiter, then one build into the nexmo-client would help prevent their requests from failing. In the former case, it doesn't hurt if it exists, and in the latter, it is a helpful fallback in case the user doesn't want to construct their own. In any case, it could be enabled with a configuration flag.

If we follow this philosophy, then nexmo-client can satisfy both situations. =) What do you think?

ghost commented 9 years ago

I support you Quinn. On Fri, Sep 11, 2015 at 7:34 AM Quinn Comendant notifications@github.com wrote:

Hello Artem,

It's true there are many ways to implement a rate limiter, and users may want to integrate their own or leverage an existing package.

But what about this idea: no matter what method of rate limiting the user choses, the ultimate goal will always be to avoid exceeding the limits set by Nexmo (e.g., 1/sec for US→US messages). Positing this, if a user has their own rate limit layer, then a rate limiter built into the nexmo-client would never trigger. Or if the user doesn't have their own rate limiter, then one build into the nexmo-client would help prevent their requests from failing. In the former case, it doesn't hurt if it exists, and in the latter, it is a helpful fallback in case the user doesn't want to construct their own. In any case, it could be enabled with a configuration flag.

If we follow this philosophy, then nexmo-client can satisfy both situations. =) What do you think?

— Reply to this email directly or view it on GitHub https://github.com/ConnectCorp/nexmo-client/pull/12#issuecomment-139450382 .

vood commented 9 years ago

@quinncomendant I see your point and do agree with it. However, I still think it should be a separate optional package. Three reasons for that:

So my suggestion: lest extract your solution into a separate nexmo-client-rate-limit package and add it as optional dependency to composer.json

quinncomendant commented 9 years ago

@vood, I agree with your points. However, I can't think how it would be possible to include this as an external package that is executed during the Service->exec() method. That is the logical location to attach a Guzzle client emitter, and is a good location to access the $params['from'] value (to use as a timer key) and the Service->getRateLimit() method (which is a very close analog to the Service->getEndpoint() method: both retrieve a setting that is specific to the service endpoint being called).

Do you have an idea how best to tie-in an external package to the nexmo-client functionality without exposing a complex plugins architecture? I'm open to all suggestions you might have.

Also, FYI, the work I've done on this was for a client who's budget has been exhausted; further work I'll only be able to manage when I have free time. But I would like to help bring this feature to the nexmo-client if people feel it will be useful.

vood commented 9 years ago

@quinncomendant oh, one more thing I've noticed after careful code review. Your implementation works only within a single process. It will not limit requests in the case when the same method is called twice within the rate limit timeframe but in separate processes. In order to implement this feature properly, you'll have to save the counter in some kind of shared key value store. Furhtermore, I'd recommend implementing at least four key value store backends: in memory (you already have it), APCu, Redis, Memcached.