completions: Switch to daily window for rate limiting #51200
Conversation
After discussing the limits yesterday, we've decided to go with a daily cap vs a lower hourly cap to help most users have a productive, uninterrupted experience with cody.
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 60ca36f and faf80a2 or learn more. Open explanation
|
| @@ -2086,8 +2086,8 @@ | |||
| "default": "anthropic", | |||
| "enum": ["anthropic", "openai"] | |||
| }, | |||
| "perUserHourlyLimit": { | |||
| "description": "If > 0, enables the maximum number of completions requests allowed to be made by a single user account in an hour. On instances that allow anonymous requests, the rate limit is enforced by IP.", | |||
| "perUserDailyLimit": { | |||
There was a problem hiding this comment.
I'm not sure how we usually do site config structure migrations. What happens to the site config of existing deployments after this change? I see no migration, so if perUserHourlyLimit is set, the config state practically becomes invalid. Do we automatically disregard the old key?
There was a problem hiding this comment.
I added it a day ago and didn’t put it anywhere so I think this time we‘re fine just changing it’s name. Usually it’d be good to fall back, agreed 🙂
| // If it did exist before, it should have an expiry set already, so the TTL >= 0 | ||
| // makes sure that we don't overwrite it and restart the 1h bucket. | ||
| ttl, err := rstore.TTL(key) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to get TTL for rate limit counter") | ||
| } | ||
| if ttl < 0 { | ||
| if err := rstore.Expire(key, int(time.Hour/time.Second)); err != nil { | ||
| if err := rstore.Expire(key, int(24*time.Hour/time.Second)); err != nil { |
There was a problem hiding this comment.
Checked this from a migration perspective: the only caveat is that if a user already has a 1h window open then the daily limit will apply to it, but then in max. 1h, it'll expire, and the next 24h window will open with the correct limit. I think we can live with this. 🙂
Yep, we are going to roll this out tomorrow probably, and that should include documentation about it :) |
After discussing the limits yesterday, we've decided to go with a daily cap vs a lower hourly cap to help most users have a productive, uninterrupted experience with cody. (cherry picked from commit 8a3bc68)
After discussing the limits yesterday, we've decided to go with a daily cap vs a lower hourly cap to help most users have a productive, uninterrupted experience with cody. (cherry picked from commit 8a3bc68)
After discussing the limits yesterday, we've decided to go with a daily cap vs a lower hourly cap to help most users have a productive, uninterrupted experience with cody.
Test plan
Tests, tried that the expiry is set correctly.