Conversation
|
I think snapping should also be removed from the bucket. The documentation says windows slide by 1 second at whole-second boundaries, not 10-second boundaries. |
|
What about something like this: func calculateBucketWithTime(now int64, entries []rateEntry, window
time.Duration) int64 {
// Window slides by 1 second at each whole-second boundary.
// At time T, the window covers [T - window + 1, T] inclusive.
// See: https://developer.fastly.com/reference/vcl/declarations/ratecounter
windowSec := int64(window.Seconds())
from := now - windowSec + 1
var bucket int64
for _, entry := range entries {
if entry.Timestamp >= from && entry.Timestamp <= now {
bucket += entry.Count
}
}
return bucket
}
func calculateRateWithTime(now int64, entries []rateEntry, window time.Duration)
float64 {
bucket := calculateBucketWithTime(now, entries, window)
if bucket == 0 {
return 0
}
return math.Floor(float64(bucket) / window.Seconds())
}(and adjust the bucket tests accordingly) |
@jedisct1 Implemented your suggestion and fixed the tests. |
|
Oh, indeed, the Fastly documentation doesn't seem to be correct. Here's something that should be closer: func calculateBucketWithTime(now int64, entries []rateEntry, window time.Duration) int64 {
currentEpoch := now / 10
numSlots := int64(window.Seconds()) / 10
var bucket int64
for _, entry := range entries {
entryEpoch := entry.Timestamp / 10
if entryEpoch > currentEpoch-numSlots && entryEpoch <= currentEpoch {
bucket += entry.Count
}
}
return bucket
}
func calculateRateWithTime(now int64, entries []rateEntry, window time.Duration) float64 {
windowSec := int64(window.Seconds())
cutoff := now - windowSec
var total int64
for _, entry := range entries {
if entry.Timestamp > cutoff {
total += entry.Count
}
}
if total == 0 {
return 0
}
return math.Floor(float64(total) / float64(windowSec))
}And the func (r *Ratecounter) Increment(entry string, delta int64) {
if _, ok := r.Clients[entry]; !ok {
r.Clients[entry] = []rateEntry{}
}
r.Clients[entry] = append(r.Clients[entry], rateEntry{
Count: delta,
Timestamp: time.Now().Unix(),
})
r.IsAccessible = true
} |
|
@jedisct1 sorry for the delay, I am still testing you code behavior. I will report the details when I finish. Unfortunately other priorities are taking over. |
| // Increment() increments access entry manually. | ||
| // This function should be called via ratelimit.ratecounter_increment() VCL function | ||
| func (r *Ratecounter) Increment(entry string, delta int64, window time.Duration) { | ||
| func (r *Ratecounter) Increment(entry string, delta int64) { |
There was a problem hiding this comment.
having window value passed to this function doesn't make sense. The effect is that instead of incrementing the latest entry, the code increments older entry which immediately goes away.
I really have hard time to see where window is beneficial
… most recently updated group rather than hardcoded client.ip
|
@jedisct1 After some testing I see that 10 sec snapping is only applicable to bucket variables ( There is no evidence of any snapping in rate computation. Current implementation of the rate computation simply |
| // Compare the rate and limit | ||
| rate := rc.Rate(entry, time.Duration(window)*time.Second) | ||
| return rate >= float64(limit), nil | ||
| return rate > float64(limit), nil |
There was a problem hiding this comment.
Fastly behavior is to trigger rate limit when calculated rate exceed specified limit. The rate just reaching the limit is OK
| type rateEntry struct { | ||
| Count int64 | ||
| Timestamp int64 // unix time second | ||
| Timestamp int64 // unix time millisecond |
There was a problem hiding this comment.
After testing with Fastly I found no evidence that their rate counting for check_rate is snapped to the whole second. Picked 1ms precision because it seems sufficient for all practical purposes (1000 times than minimal rate computation window).
| IsAccessible bool | ||
| // Last incremented entry (used to estimate values ratecounter.{NAME} rate and bucket variables) | ||
| // Also LastIncremented != nil serves as IsAccessible flag | ||
| LastIncremented *string |
There was a problem hiding this comment.
This serves dual purpose:
- Keeps track of last updated client key
- Replaces IsAccessible flag
|
Comparison results between falco (left side) and fastly (right side) Columns:Test environment:
Falco simulator:
Fastly service:
Test results:
Corresponding VCL code deployed on both Fastly and Falcoratecounter rcInc {}
ratecounter rc {}
penaltybox pb {}
sub vcl_recv {
declare local var.delta INTEGER;
set var.delta = std.atoi(req.http.x-delta);
declare local var.window INTEGER;
set var.window = std.atoi(req.http.x-window);
if (var.window == 0) {
set var.window = 1;
}
declare local var.limit INTEGER;
set var.limit = std.atoi(req.http.x-limit);
if (var.limit == 0) {
set var.limit = 10;
}
set req.http.x-time = time.start.msec;
set req.http.x-req-count = ratelimit.ratecounter_increment(rcInc, "group", 1);
set req.http.x-outcome = ratelimit.check_rate("group", rc, var.delta, var.window, var.limit, pb, 1m);
set req.http.x-penalized = ratelimit.penaltybox_has(pb, "group");
set req.http.x-bucket10s = ratecounter.rc.bucket.10s;
set req.http.x-bucket20s = ratecounter.rc.bucket.20s;
set req.http.x-bucket30s = ratecounter.rc.bucket.30s;
set req.http.x-bucket40s = ratecounter.rc.bucket.40s;
set req.http.x-bucket50s = ratecounter.rc.bucket.50s;
set req.http.x-bucket60s = ratecounter.rc.bucket.60s;
set req.http.x-rate1s = ratecounter.rc.rate.1s;
set req.http.x-rate10s = ratecounter.rc.rate.10s;
set req.http.x-rate60s = ratecounter.rc.rate.60s;
return (error);
}
sub vcl_error {
set obj.status = 200;
set obj.http.x-time = req.http.x-time;
set obj.http.x-req-count = req.http.x-req-count;
set obj.http.x-outcome = req.http.x-outcome;
set obj.http.x-penalized = req.http.x-penalized;
set obj.http.x-vcl-version = req.vcl.version;
set obj.http.x-bucket10s = req.http.x-bucket10s;
set obj.http.x-bucket20s = req.http.x-bucket20s;
set obj.http.x-bucket30s = req.http.x-bucket30s;
set obj.http.x-bucket40s = req.http.x-bucket40s;
set obj.http.x-bucket50s = req.http.x-bucket50s;
set obj.http.x-bucket60s = req.http.x-bucket60s;
set obj.http.x-rate1s = req.http.x-rate1s;
set obj.http.x-rate10s = req.http.x-rate10s;
set obj.http.x-rate60s = req.http.x-rate60s;
} |
2 participants
This PR attempts to fix 2 simulator issues::
This PR addresses the first issue by copying ratecounter and penaltybox maps from the old context to the newly created one as the context gets re-initialized for the next request. The downside of this approach is that ratecounter and penaltybox instances are never cleaned up even if updated VCL no longer declares them.