Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding delta_reset_ms to TAKE and TAKEELEVATED responses #80

Merged
merged 4 commits into from
Oct 7, 2024
Merged

feat: adding delta_reset_ms to TAKE and TAKEELEVATED responses #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b38b72
adding delta_reset_ms to TAKE and TAKEELEVATED responses
pubalokta Sep 30, 2024
a5fb096
fix tests in makefile
pubalokta Sep 30, 2024
10ff153
avoid negative delta_reset_ms
pubalokta Oct 3, 2024
4695339
added test coverage on delta_reset_ms for unlimited buckets
pubalokta Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
export CLUSTER_NO_TLS_VALIDATION=true

test-standalone-setup:
docker-compose up -d
docker compose up -d

test-standalone-teardown:
docker-compose down
docker compose down

test-cluster-setup:
docker-compose -f docker-compose-cluster.yml up -d
docker compose -f docker-compose-cluster.yml up -d

test-cluster-teardown:
docker-compose -f docker-compose-cluster.yml down
docker compose -f docker-compose-cluster.yml down

test-cluster:
npm run test-cluster
Expand Down
4 changes: 3 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,8 @@ The result object has:
- `conformant` (boolean): true if the requested amount is conformant to the limit.
- `remaining` (int): the amount of remaining tokens in the bucket.
- `reset` (int / unix timestamp): unix timestamp of the date when the bucket will be full again.
- `limit` (int): the size of the bucket.
- `limit` (int): the size of the bucket.
- `delta_reset_ms` (int): the time remaining until the bucket is full again, expressed in milliseconds from the current time.

## TAKEELEVATED

Expand Down Expand Up @@ -305,6 +306,7 @@ The result object has:
- `remaining` (int): the amount of remaining tokens in the bucket.
- `reset` (int / unix timestamp): unix timestamp of the date when the bucket will be full again.
- `limit` (int): the size of the bucket.
- `delta_reset_ms` (int): the time remaining until the bucket is full again, expressed in milliseconds from the current time.
- `elevated_limits` (object)
- `triggered` (boolean): true if ERL was triggered in the current request.
- `activated` (boolean): true if ERL is activated. Not necessarily triggered in this call.
Expand Down
3 changes: 3 additions & 0 deletions lib/db.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ class LimitDBRedis extends EventEmitter {
conformant: true,
remaining: bucketKeyConfig.size,
reset: Math.ceil(Date.now() / 1000),
delta_reset_ms: 0,
limit: bucketKeyConfig.size,
delayed: false,
});
Expand Down Expand Up @@ -283,6 +284,7 @@ class LimitDBRedis extends EventEmitter {
reset: Math.ceil(reset / 1000),
limit: bucketKeyConfig.size,
delayed: false,
delta_reset_ms: Math.max(reset - currentMS, 0)
ecita marked this conversation as resolved.
Show resolved Hide resolved
};
if (bucketKeyConfig.skip_n_calls > 0) {
this.callCounts.set(key, { res, count: 0 });
Expand Down Expand Up @@ -345,6 +347,7 @@ class LimitDBRedis extends EventEmitter {
quota_allocated: elevated_limits.erl_quota,
erl_activation_period_seconds: elevated_limits.erl_activation_period_seconds,
},
delta_reset_ms: Math.max(reset - currentMS, 0),
};
if (bucketKeyConfig.skip_n_calls > 0) {
this.callCounts.set(key, { res, count: 0 });
Expand Down
51 changes: 51 additions & 0 deletions test/db.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ module.exports.tests = (clientCreator) => {
assert.ok(result.conformant);
assert.equal(result.remaining, 9);
assert.closeTo(result.reset, now / 1000, 3);
assert.closeTo(result.delta_reset_ms, (result.limit - result.remaining) * 1000/buckets.ip.per_second, 3);
ecita marked this conversation as resolved.
Show resolved Hide resolved
assert.equal(result.limit, 10);
done();
});
Expand All @@ -291,6 +292,7 @@ module.exports.tests = (clientCreator) => {
assert.ok(result.conformant);
assert.equal(result.remaining, 9);
assert.closeTo(result.reset, now / 1000, 3);
assert.closeTo(result.delta_reset_ms, (result.limit - result.remaining) * 1000/buckets.ip.per_second, 3);
assert.equal(result.limit, 10);
done();
});
Expand All @@ -309,6 +311,7 @@ module.exports.tests = (clientCreator) => {
assert.notOk(result.conformant);
assert.equal(result.remaining, 10);
assert.closeTo(result.reset, now / 1000, 3);
assert.closeTo(result.delta_reset_ms, (result.limit - result.remaining) * 1000/buckets.ip.per_second, 3);
assert.equal(result.limit, 10);
done();
});
Expand Down Expand Up @@ -431,6 +434,7 @@ module.exports.tests = (clientCreator) => {
assert.ok(lastResult.conformant);
assert.equal(lastResult.remaining, 1);
assert.closeTo(lastResult.reset, now / 1000, 3);
assert.closeTo(lastResult.delta_reset_ms, (lastResult.limit - lastResult.remaining) * 1000/buckets.ip.per_second, 100);
assert.equal(lastResult.limit, 10);
done();
});
Expand All @@ -446,6 +450,7 @@ module.exports.tests = (clientCreator) => {
assert.ok(result.conformant);
assert.equal(result.remaining, 0);
assert.closeTo(result.reset, now / 1000 + 1800, 1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why they added an extra 1800 here (maybe because of an override?)

Wondering whether we should consider and incorporate it for delta_reset_ms assertion too

Copy link
Author

@pubalokta pubalokta Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bucket is configured at 2 tokens per hour. As it's a sliding window it calculates it needs to add 1 token every 30 minutes (1800 secs).
And it's indirectly set in the delta_reset_ms. After the calculations, it'll be 1800 secs as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't fully understand why we are considering "half an hour" here. Can we :watercooler: on it after this sometime?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you have a good way to describe it in text. :D

assert.closeTo(result.delta_reset_ms, (result.limit - result.remaining) * 3600000/buckets.ip.overrides['10.0.0.1'].per_hour, 1);
assert.equal(result.limit, 1);
done();
});
Expand All @@ -460,6 +465,7 @@ module.exports.tests = (clientCreator) => {
assert.equal(response.limit, 100);
assert.equal(response.remaining, 100);
assert.closeTo(response.reset, now / 1000, 1);
assert.closeTo(response.delta_reset_ms, (response.limit - response.remaining) * 1000/buckets.ip.per_second, 1);
done();
});
});
Expand Down Expand Up @@ -695,6 +701,46 @@ module.exports.tests = (clientCreator) => {
});
});
});

describe(`${testParams.name} delta_reset_ms`, () => {
it('should reset the bucket after the specified interval', (done) => {
db.configurateBuckets({ 'test_bucket': { size: 100, per_second: 100 } });
const params = { ...testParams.params, type: 'test_bucket', key: 'delta_key', count: 100 };
testParams.take(params, (err, res) => {
if (err) {
done(err);
}
assert.isTrue(res.conformant);
assert.equal(res.remaining, 0);
assert.notEqual(res.delta_reset_ms, 0);

setTimeout(() => {
params.count = 1;
testParams.take(params, (err, res) => {
if (err) {
done(err);
}
assert.isTrue(res.conformant);
assert.notEqual(res.delta_reset_ms, 0);
done();
});
}, res.delta_reset_ms);
});
});

it('should set delta_reset_ms to 0 when bucket is unlimited', (done) => {
db.configurateBuckets({ 'test_bucket': { size: 100, unlimited: true } });
const params = { ...testParams.params, type: 'test_bucket', key: 'delta_key', count: 100 };
testParams.take(params, (err, res) => {
if (err) {
done(err);
}
assert.isTrue(res.conformant);
assert.equal(res.delta_reset_ms, 0);
done();
});
});
});
});
});

Expand All @@ -720,6 +766,7 @@ module.exports.tests = (clientCreator) => {
}
const dayFromNow = Date.now() + oneDayInMs;
assert.closeTo(response.reset, dayFromNow / 1000, 3);
assert.closeTo(response.delta_reset_ms, (response.limit - response.remaining) * 24*60*60*1000, 3);
done();
});
});
Expand All @@ -737,6 +784,7 @@ module.exports.tests = (clientCreator) => {

const dayFromNow = Date.now() + oneDayInMs;
assert.closeTo(response.reset, dayFromNow / 1000, 3);
assert.closeTo(response.delta_reset_ms, (response.limit - response.remaining) * 24*60*60*1000, 3);
done();
});
});
Expand Down Expand Up @@ -2146,6 +2194,7 @@ module.exports.tests = (clientCreator) => {
if (err) return done(err);
const dayFromNow = Date.now() + oneDayInMs;
assert.closeTo(response.reset, dayFromNow / 1000, 3);
assert.closeTo(response.delta_reset_ms, (response.limit - response.remaining) * 24*60*60*1000, 3);
done();
});
});
Expand All @@ -2165,6 +2214,7 @@ module.exports.tests = (clientCreator) => {
assert.equal(response.remaining, 3);
const dayFromNow = Date.now() + oneDayInMs;
assert.closeTo(response.reset, dayFromNow / 1000, 3);
assert.closeTo(response.delta_reset_ms, (response.limit - response.remaining) * 24*60*60*1000, 3);
done();
});
});
Expand Down Expand Up @@ -2297,6 +2347,7 @@ module.exports.tests = (clientCreator) => {
assert.notOk(response.delayed);
assert.equal(response.remaining, 9);
assert.closeTo(response.reset, now / 1000, 3);
assert.closeTo(response.delta_reset_ms, (response.limit - response.remaining) * 1000/buckets.ip.per_second, 3);
done();
});
});
Expand Down