(BIDS-2872) copy ratelimit over from v1 #647
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guybrush
force-pushed
the
BIDS-2872/ratelimit
branch
from
b176c51 to
012c097
Compare
guybrush
force-pushed
the
BIDS-2872/ratelimit
branch
from
012c097 to
545074c
Compare
LuccaBitfly
requested changes
Jul 31, 2024
Comment on lines
+28
to
+30
| SecondTimeWindow = "second" | ||
| HourTimeWindow = "hour" | ||
| MonthTimeWindow = "month" |
There was a problem hiding this comment.
Minor:
Suggested change
| SecondTimeWindow = "second" | |
| HourTimeWindow = "hour" | |
| MonthTimeWindow = "month" | |
| SecondTimeWindow TimeWindow = "second" | |
| HourTimeWindow TimeWindow = "hour" | |
| MonthTimeWindow TimeWindow = "month" |
Comment on lines
+223
to
+265
| firstRun := true | ||
| for { | ||
| err := updateWeights(firstRun) | ||
| if err != nil { | ||
| log.Error(err, "error updating weights", 0) | ||
| time.Sleep(time.Second * 2) | ||
| continue | ||
| } | ||
| if firstRun { | ||
| initializedWg.Done() | ||
| firstRun = false | ||
| } | ||
| time.Sleep(updateInterval) | ||
| } | ||
| }() | ||
| go func() { | ||
| firstRun := true | ||
| for { | ||
| err := updateRateLimits() | ||
| if err != nil { | ||
| log.Error(err, "error updating ratelimits", 0) | ||
| time.Sleep(time.Second * 2) | ||
| continue | ||
| } | ||
| if firstRun { | ||
| initializedWg.Done() | ||
| firstRun = false | ||
| } | ||
| time.Sleep(updateInterval) | ||
| } | ||
| }() | ||
| go func() { | ||
| firstRun := true | ||
| for { | ||
| updateRedisStatus() | ||
| if firstRun { | ||
| initializedWg.Done() | ||
| firstRun = false | ||
| } | ||
| time.Sleep(time.Second * 1) | ||
| } | ||
| }() | ||
|
|
There was a problem hiding this comment.
Minor: could write all that that "first run" logic in a seperate func to avoid code duplication
| Bucket string `db:"bucket"` | ||
| ValidFrom time.Time `db:"valid_from"` | ||
| }{} | ||
| err := db.FrontendWriterDB.Select(&dbWeights, "SELECT DISTINCT ON (endpoint) endpoint, bucket, weight, valid_from FROM api_weights WHERE valid_from <= NOW() ORDER BY endpoint, valid_from DESC") |
There was a problem hiding this comment.
You may want to introduce contexts with timeouts for the queries here
| } | ||
|
|
||
| for _, dbRl := range dbRateLimits { | ||
| k := fmt.Sprintf("%s/%d", dbRl.Bucket, dbRl.UserID) |
There was a problem hiding this comment.
Should use a func here that returns the key string from bucket+user id, rather than having this specific string format in multiple places.
Comment on lines
+752
to
+761
| rateLimitSecondKey := fmt.Sprintf("rl:c:s:%s:%d", res.Bucket, res.UserId) | ||
| rateLimitHourKey := fmt.Sprintf("rl:c:h:%04d-%02d-%02d-%02d:%s:%d", startUtc.Year(), startUtc.Month(), startUtc.Day(), startUtc.Hour(), res.Bucket, res.UserId) | ||
| rateLimitMonthKey := fmt.Sprintf("rl:c:m:%04d-%02d:%s:%d", startUtc.Year(), startUtc.Month(), res.Bucket, res.UserId) | ||
| statsKey := fmt.Sprintf("rl:s:%04d-%02d-%02d-%02d:%d:%s:%s:%s", startUtc.Year(), startUtc.Month(), startUtc.Day(), startUtc.Hour(), res.UserId, res.Key, res.Route, res.Bucket) | ||
| if !res.IsValidKey { | ||
| normedIP := "ip_" + strings.ReplaceAll(ip, ":", "_") | ||
| rateLimitSecondKey = fmt.Sprintf("rl:c:s:%s:%s", res.Bucket, normedIP) | ||
| rateLimitHourKey = fmt.Sprintf("rl:c:h:%04d-%02d-%02d-%02d:%s:%s", startUtc.Year(), startUtc.Month(), startUtc.Day(), startUtc.Hour(), res.Bucket, normedIP) | ||
| rateLimitMonthKey = fmt.Sprintf("rl:c:m:%04d-%02d:%s:%s", startUtc.Year(), startUtc.Month(), res.Bucket, normedIP) | ||
| statsKey = fmt.Sprintf("rl:s:%04d-%02d-%02d-%02d:%d:%s:%s:%s", startUtc.Year(), startUtc.Month(), startUtc.Day(), startUtc.Hour(), res.UserId, "nokey", res.Route, res.Bucket) |
There was a problem hiding this comment.
All these keys should have their own func to generate them
Comment on lines
+768
to
+860
|
|
||
| if res.RateLimit.Second > 0 { | ||
| rateLimitSecond = pipe.IncrBy(ctx, rateLimitSecondKey, weight) | ||
| pipe.ExpireNX(ctx, rateLimitSecondKey, time.Second) | ||
| } | ||
|
|
||
| if res.RateLimit.Hour > 0 { | ||
| rateLimitHour = pipe.IncrBy(ctx, rateLimitHourKey, weight) | ||
| pipe.ExpireAt(ctx, rateLimitHourKey, nextHourUtc.Add(time.Second*60)) // expire 1 minute after the window to make sure we do not miss any requests due to time-sync | ||
| res.RedisKeys = append(res.RedisKeys, RedisKey{rateLimitHourKey, nextHourUtc.Add(time.Second * 60)}) | ||
| } | ||
|
|
||
| if res.RateLimit.Month > 0 { | ||
| rateLimitMonth = pipe.IncrBy(ctx, rateLimitMonthKey, weight) | ||
| pipe.ExpireAt(ctx, rateLimitMonthKey, nextMonthUtc.Add(time.Second*60)) // expire 1 minute after the window to make sure we do not miss any requests due to time-sync | ||
| res.RedisKeys = append(res.RedisKeys, RedisKey{rateLimitMonthKey, nextMonthUtc.Add(time.Second * 60)}) | ||
| } | ||
|
|
||
| pipe.Incr(ctx, statsKey) | ||
| _, err := pipe.Exec(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if res.RateLimit.Month > 0 && rateLimitMonth.Val() > res.RateLimit.Month { | ||
| res.Limit = res.RateLimit.Month | ||
| res.Remaining = 0 | ||
| res.Reset = int64(timeUntilNextMonthUtc.Seconds()) | ||
| res.Window = MonthTimeWindow | ||
| res.BlockRequest = true | ||
| } else if res.RateLimit.Hour > 0 && rateLimitHour.Val() > res.RateLimit.Hour { | ||
| res.Limit = res.RateLimit.Hour | ||
| res.Remaining = 0 | ||
| res.Reset = int64(timeUntilNextHourUtc.Seconds()) | ||
| res.Window = HourTimeWindow | ||
| res.BlockRequest = true | ||
| } else if res.RateLimit.Second > 0 && rateLimitSecond.Val() > res.RateLimit.Second { | ||
| res.Limit = res.RateLimit.Second | ||
| res.Remaining = 0 | ||
| res.Reset = int64(1) | ||
| res.Window = SecondTimeWindow | ||
| res.BlockRequest = true | ||
| } else { | ||
| res.Limit = res.RateLimit.Second | ||
| res.Remaining = res.RateLimit.Second - rateLimitSecond.Val() | ||
| res.Reset = int64(1) | ||
| res.Window = SecondTimeWindow | ||
| } | ||
|
|
||
| if res.RateLimit.Second > 0 { | ||
| res.RemainingSecond = res.RateLimit.Second - rateLimitSecond.Val() | ||
| if res.RemainingSecond < 0 { | ||
| res.RemainingSecond = 0 | ||
| } | ||
| } | ||
| if res.RateLimit.Hour > 0 { | ||
| res.RemainingHour = res.RateLimit.Hour - rateLimitHour.Val() | ||
| if res.RemainingHour < 0 { | ||
| res.RemainingHour = 0 | ||
| } | ||
| } | ||
| if res.RateLimit.Month > 0 { | ||
| res.RemainingMonth = res.RateLimit.Month - rateLimitMonth.Val() | ||
| if res.RemainingMonth < 0 { | ||
| res.RemainingMonth = 0 | ||
| } | ||
| } | ||
|
|
||
| // normalize limit-headers to keep them consistent with previous versions | ||
| if res.RateLimit.Month > 0 { | ||
| res.LimitMonth = res.RateLimit.Month | ||
| } else { | ||
| res.LimitMonth = max(res.RateLimit.Month, res.RateLimit.Hour, res.RateLimit.Second) | ||
| res.RemainingMonth = max(res.RemainingMonth, res.RemainingHour, res.RemainingSecond) | ||
| } | ||
| res.LimitDay = res.LimitMonth | ||
| res.RemainingDay = res.RemainingMonth | ||
|
|
||
| if res.RateLimit.Hour > 0 { | ||
| res.LimitHour = res.RateLimit.Hour | ||
| } else { | ||
| res.LimitHour = res.LimitMonth | ||
| res.RemainingHour = res.RemainingMonth | ||
| } | ||
| res.LimitMinute = res.LimitHour | ||
| res.RemainingMinute = res.RemainingHour | ||
|
|
||
| if res.RateLimit.Second > 0 { | ||
| res.LimitSecond = res.RateLimit.Second | ||
| } else { | ||
| res.LimitSecond = res.LimitHour | ||
| } | ||
|
|
There was a problem hiding this comment.
A lot of the logic here is repeated 3 times for each time window. IMO you should extract common logic into a seperate function (as far as is possible), so that the code looks something like this:
doLogic1(...) // second-related arguments
doLogic1(...) // hour-related arguments
doLogic1(...) // month-related arguments
doLogic2(...) // second-related arguments
doLogic2(...) // hour-related arguments
doLogic2(...) // month-related arguments
Also: Is there a specific reason why all the "logics" have to happen for each time window, instead of doing everything at once? Like this
doAllLogic(...) // second-related arguments
doAllLogic(...) // hour-related arguments
doAllLogic(...) // month-related arguments
Comment on lines
+115
to
+127
| Limit int64 | ||
| LimitSecond int64 | ||
| LimitMinute int64 | ||
| LimitHour int64 | ||
| LimitDay int64 | ||
| LimitMonth int64 | ||
|
|
||
| Remaining int64 | ||
| RemainingSecond int64 | ||
| RemainingMinute int64 | ||
| RemainingHour int64 | ||
| RemainingDay int64 | ||
| RemainingMonth int64 |
There was a problem hiding this comment.
Minor: Could have a struct
type TimeFrames struct {
Value int64
Second int64
Minute int64
Hour int64
Day int64
Month int64
}and then
Suggested change
| Limit int64 | |
| LimitSecond int64 | |
| LimitMinute int64 | |
| LimitHour int64 | |
| LimitDay int64 | |
| LimitMonth int64 | |
| Remaining int64 | |
| RemainingSecond int64 | |
| RemainingMinute int64 | |
| RemainingHour int64 | |
| RemainingDay int64 | |
| RemainingMonth int64 | |
| Limit TimeFrames | |
| Remaining TimeFrames |
There was a problem hiding this comment.
Minor: While reviewing this I had to "jump" all over the place, because funcs that coherently fit together are sometimes defined at two completely different places. E.g. HttpMiddleware calls rateLimitRequest, but for some reason there are a lot of other unrelated funcs between them. Getting back to the calling func after reading through the called func can be a bit jarring.
guybrush
force-pushed
the
BIDS-2872/ratelimit
branch
from
faaa65f to
5978301
Compare
Deploying beaconchain with
|
| Latest commit: |
5978301
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9c7c5224.beaconchain.pages.dev |
| Branch Preview URL: | https://bids-2872-ratelimit.beaconchain.pages.dev |
|
thanks for the review, all the arguments and suggestions are valid but i think its better to keep code-parity with v1 for now. i would have to change v1 and for some changes would need to up go version in go.mod to use newer features. willl keep this code-review in mind when i touch the code next time, thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.