Integrate redis rate limiter with saas connectors
Closes #1221
Code Changes
- [x] Added
RateLimitPeriodandRateLimitConfigto schema package as the model to be used by our connector config - [x] Added
rate_limitstoSaaSRequestandSaaSConfigmodels - [x] Changed
RateLimiterimplementation to work with a list ofRateLimitRequest. This means that one call tolimitcan respect multiple limits. Because of this we now decrement counts when a limit is breached. - [x] Changed
AuthenticatedClientto take depend on aSaasRequestinstead ofClientConfig - [x] Implemented
build_rate_limit_requestsinAuthenticatedClientto buildRateLimitRequestbased on config - [x] Call
RateLimiter.limitwithinAuthenticatedClient
Steps to Confirm
- [ ] Manual testing
- [ ] Current tests for regression(could still add some more automated testing I think)
Pre-Merge Checklist
- [ ] All CI Pipelines Succeeded
- Documentation Updated:
- [ ] documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
- [ ] documentation issue created (tag docs-team to complete issue separately)
- [ ] Issue Requirements are Met
- [ ] Relevant Follow-Up Issues Created
- [ ] Update
CHANGELOG.md
Description Of Changes
Model and integration changes for rate limiter from #1189.
We decided to allow the rate limiter to allow multiple limits which meant changing current limiter implementation. If we want to respect multiple limits simultaneously then it is not enough to just increment the rate limiter as it did before. If one limit succeeds and another fails, then the rate limiter should decrement the succeeding limit as a call will not be made.
One place where I could see us having further discussion is whether request level limits should override or add on to connector rate limits. This is implemented in build_rate_limit_requests.
@adamsachs n @galvana, I think this is in a pretty good place to get some feedback. I'll also add some comments for some things which I think need more attention.
@earmenda thanks for addressing the feedback - i think this looks just about good to go, besides one mypy issue it seems.
i also put down one minor comment about a potential enhancement of one of your tests - but that's not a blocker.
sorry about the deleted comment, was trying to understand if that's why I couldn't resolve all conversations.
@adamsachs @galvana I made some major changes since we reviewed this last week. As I was writing documentation I realized that there was some confusing user experience with my original idea. I decided that if a rate limit config is set on the client config AND at the request endpoint then it should work as an override. Some reasons for this:
- More inline the current design of client config override. From an UX perspective and implementation as well.
- Adding the limits at both levels doesn't really add much usefulness for users. I couldn't think of a use case which wasn't possible by treating it as an override instead.
- Allows for more use cases. By treating this as an override we can define a rate limit in client config and disable the limit for a specific request endpoint. To allow for this I changed the model of
RateLimitConfigto have anenabledfield. Limits are now defined within alimitsfield.
I also decided to redo the refactoring that I had undone because I wanted to simplify the SaasConnector and AuthenticatedClient a little bit. Here is the changes to each:
SaasConnector:
- No longer sets
client_configin constructor. Created a new methodget_client_configwhich returns the client config based on the current class state. - Changes to class state are now only done through
set_privacy_request_stateandset_saas_request_statemethods. This should make it more obvious to the developer where the class state is being modified. - Creating a client can now just be done by calling
create_clientwithout any arguments or additional methods to change state inAuthenticatedClient. - Removed as many usages of
# type: ignoreas I could.
AuthenticatedClient
- Removed logic for determining correct client config. It now only uses the client config passed in the constructor. This works because
SaasConnectornow sends only the correct config by using itsget_client_configmethod.
Sorry for the refactoring to this PR but it was closely tied to the implementation of this feature. I am pretty comfortable merging this now after review. Documentation is also done!
@conceptualshark do you have some time to take a look at new docs I wrote for saas connector rate limiting?
@adamsachs Thanks again for the thorough review, your comments have been really helpful.