Conversation
There was a problem hiding this comment.
Thanks for making the rate limits adjustable from the environment. The more values that keep getting added to the config.py, the more I'm favor of breaking it up, so that config files are closer to where they'll get used. Not for this PR, but worth noting.
| print(f"Status: {response.status_code}") | ||
| print(f"User: {response.json().get('name')}\n") | ||
|
|
||
| # Test rate limiting is active |
There was a problem hiding this comment.
Rate limiting test code is pretty similar across the 3 clients. There might be a DRY method on encapsulating the timing logic and passing the actual test as a function parameter. Not critical, but worth noting in case we want to adjust the timing logic or re-use it elsewhere.
|
|
||
| REQUEST_COUNT = 15 | ||
| TEST_SHEET_KEY = "17gjWh6YdrrX5r_ci9kF6COFSzxehKt53nMZPG5uTdp0" | ||
| CREDENTIALS_FILE = "gspread_credentials.json" |
There was a problem hiding this comment.
I'm aware these files aren't necessarily the focus of the PR, but it's concerning to have values from the env file leak (even if private keys weren't leaked). These should ideally be references to the settings object from the config file. I'm assuming these are temporary, but I can't merge the PR with this present.
| self.gc = self.create_credentials_from_env() | ||
| return self.gc | ||
|
|
||
| def create_credentials_from_env(self) -> gspread.Client: |
There was a problem hiding this comment.
Note that create_credentials in the gsheet/utils.py does the same thing; I'd prefer to not have duplicate functions so that function should be used instead. Again, since this isn't the focus of the PR, I'm assuming that this is not meant to be the final implementation and is here for testing, but I'm wary of formally merging.
| self.limiter.try_acquire("gsheets") | ||
| break | ||
| except BucketFullException: | ||
| time.sleep(0.1) |
There was a problem hiding this comment.
A bit odd to have a hard-coded value here, ideally this is changeable from either the function call or config. Not part of this PR, but it might be interesting to use a dynamic wait system to help predict when to check.
Additionally, I'm concerned about the infinite loop. It's not clear what exceptions limiter.try_acquire can produce or what's expected in this context. Ideally, there's another exit point (TimeOutException) to prevent this. Another issue preventing me from merging to main.
| return response | ||
|
|
||
| if attempt < self.max_retries: | ||
| time.sleep(2 ** attempt) |
There was a problem hiding this comment.
This also shouldn't be hard-coded value, since we may want to tweak it in the future (or possibly try to implement the same sleep time algorithm across each client. Not necessary to change for this PR, but something to consider in the follow-up.
|
The new code, especially regarding the Canvas client, looks good. The rate limiting appears to be implemented correctly, and the defaults represent what's listed in Canvas. Tests pass, but I've yet to test it properly in a deployment. The main issues preventing me from merging are
An explanation could resolve both of them (possibly in today's meeting), but some changes may be needed. |
2 participants
API rate limiting
Added rate limiting for Canvas API calls using the
requests-ratelimiterlibrary. Created a CanvasClient class insrc/utils/rate_limiting/canvas_api.pythat wraps all HTTP requests with aLimiterSession, which uses a leaky bucket algorithm to automatically throttle requests to 10/second (configurable viaCANVAS_RATE_LIMIT_PER_SECOND).Also added rate limiting for SendGrid and Google Sheets APIs using pyrate-limiter with manual acquire/block pattern, since their SDKs handle HTTP calls internally and cannot be wrapped with
LimiterSessionfromrequests-ratelimiter.Issues Fixed
Tests
Each client includes a minor throttle test script that verifies rate limiting is active by sending rapid requests and measuring throughput:
python -m src.utils.rate_limiting.canvas_minor_stress_testingpython -m src.utils.rate_limiting.sendgrid_minor_stress_testingpython -m src.utils.rate_limiting.gspread_minor_stress_testingNotes
For this PR, only the check-activity endpoint has been updated to use the new canvas_client. Other endpoints using Canvas/sendgrid/gspread API calls can be migrated in future PRs.
Notes on Library changes
Checklist