Integrate redis rate limiter with saas connectors

[earmenda]

Closes #1221

Code Changes

• Added RateLimitPeriod and RateLimitConfig to schema package as the model to be used by our connector config
• Added rate_limits to SaaSRequest and SaaSConfig models
• Changed RateLimiter implementation to work with a list of RateLimitRequest. This means that one call to limit can respect multiple limits. Because of this we now decrement counts when a limit is breached.
• Changed AuthenticatedClient to take in SaasRequest as parameter
• Implemented build_rate_limit_requests in AuthenticatedClient to build RateLimitRequest based on config
• Call RateLimiter.limit within AuthenticatedClient
• Implement end to end integration test for rate limiter

Steps to Confirm

• Manual testing
• Current tests for regression
• Added full end to end integration test

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.

[earmenda]

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

[adamsachs]

i think everything here looks really good @earmenda. i really like how you've brought the rate limiter into the saas framework - it's super clean and easy to follow.

in the application code, only some minor comments asking for a bit of clarification on certain points.

i do think you could build on your test updates a bit further. particularly, i'd like to see some
"end-to-end" coverage actually evaluating that the rate limiter config is correctly making its way into the saas execution framework (see comment on L103 saas_config.py). happy to touch base offline if what i mean by that isn't clear.

[earmenda]

@adamsachs thanks for your in-depth review. I addressed most of the things discussed but there's a couple things I'm putting some thought into still. Let me know what you think!

[adamsachs]

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

[earmenda]

sorry about the deleted comment, was trying to understand if that's why I couldn't resolve all conversations.

[earmenda]

@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 RateLimitConfig to have an enabled field. Limits are now defined within a limits field.

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_config in constructor. Created a new method get_client_config which returns the client config based on the current class state.
• Changes to class state are now only done through set_privacy_request_state and set_saas_request_state methods. 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_client without any arguments or additional methods to change state in AuthenticatedClient.
• Removed as many usages of # type: ignore as I could.

AuthenticatedClient

• Removed logic for determining correct client config. It now only uses the client config passed in the constructor. This works because SaasConnector now sends only the correct config by using its get_client_config method.

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!

[earmenda]

@conceptualshark do you have some time to take a look at new docs I wrote for saas connector rate limiting?

[adamsachs]

overall i like all the updates @earmenda! the client code is more transparent now IMO, and i like the UX you've enabled with rate limiter overrides.

just some minor comments that i don't think are blockers, so I'm approving. but i do think they're worth taking a look at and possibly addressing.

[adamsachs]

latest changes look to address what i mentioned really nicely, thanks for that.

overall, this looks great - let's get it merged now!

[earmenda]

@adamsachs Thanks again for the thorough review, your comments have been really helpful.