Allow customization of weight per request for rate limiting #505
Conversation
Allow customization of weight per request for rate limiting
jiang925
force-pushed
the
jiang925/dynamic-weight
branch
from
9f97a8e to
1c02f54
Compare
|
Hi @jiang925 , Thank you very much for the contribution! On the one hand this seems like an interesting feature, while on the other I have concerns about whether this may add unnecessary complexity to the library and the docs/API if this is not used a lot by users. I want to give it some more thought though. 🤔 Thanks again! |
There was a problem hiding this comment.
I would support this. It adds flexibility that would make Rack::Attack more useful for APIs in particular. I don't think it adds much complexity. Users who don't use it don't even need to know about it. And it would not change its performance at all, since increment already supports values other than 1. The only suggestion I have is to add one test.
(I use "we" and "our" here to mean people who care about Rack::Attack. :) )
If we're concerned about future compatibility and complexity, and think there might one day be additional parameters to the count method, we could define weight as an optional keyword argument (weight: 1) rather than an argument with a default (weight = 1). This would allow a 4th and 5th argument to be passed in without having to specify a nondefault weight. However this doesn't seem very likely. I think weight = 1 is fine.
A common user error might be to try to specify a noninteger weight (both memcache and redis support only integer values). We could .to_i the input before passing it to do_count, to try to minimize the impact of this mistake in a lightweight way.
And let's think about the edge case of weight 0. Because throttling depends on count returning the current value, invoking it with a weight of 0 can't short-circuit (unless we want to define that a weight=0 request never throttles, in which case we should short-circuit ourselves). I checked current versions of Rails' MemCacheStore and Dalli's Client, and good news, neither currently short-circuits. It might be worth adding a test to our suite checking for a 429. This would ensure that none of the configurations in Appraisal returns nil or 0 without hitting cache.
| assert_equal 429, last_response.status | ||
| assert_nil last_response.headers["Retry-After"] | ||
| assert_equal "Retry later\n", last_response.body | ||
|
|
There was a problem hiding this comment.
Here's a good place to test that a weight of zero also returns a 429. (Add request.env["X-Lightweight"] to the lambda?)
Test suggestion: jamiemccarthy@6ec96eb
There was a problem hiding this comment.
@jamiemccarthy Thank you for the suggestion and sorry for the delay! I missed all these notifications. I'll take a look soon and get back!
| @@ -244,7 +244,7 @@ Throttle state is stored in a [configurable cache](#cache-store-configuration) ( | |||
|
|
|||
| #### `throttle(name, options, &block)` | |||
|
|
|||
| Name your custom throttle, provide `limit` and `period` as options, and make your ruby-block argument return the __discriminator__. This discriminator is how you tell rack-attack whether you're limiting per IP address, per user email or any other. | |||
| Name your custom throttle, provide `limit`, `period` and `weight` as options, and make your ruby-block argument return the __discriminator__. This discriminator is how you tell rack-attack whether you're limiting per IP address, per user email or any other. | |||
There was a problem hiding this comment.
nit: may worth mention weight is an optional option
Use case:
We have an API to allow the client to fetch data by a list of ids. The API allows up to a few thousand ids. In order to better throttling experience, we want to throttle based on the number of ids being fetch. Add a weight option to specify the rate to deduct the limit.
Added tests and tests pass.