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

[quinncomendant]

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]

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

[vood]

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]

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]

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
#12 (comment)
.

[vood]

@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:

• It is a good practice to keep the project as simple as possible and make sure it does only one thing
• Adding rate limiting feature breaks single responsibility principle
• It reduces overall unit test code coverage (your code doesn't have tests at all)

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]

@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]

@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.

[vood]

Never put your code into sleep. Better fix your client that calls the api too frequently. This is definitely an unexpected behaviour. I'd rather throw an exception here.