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.

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: add fixed_window bucket option #79

Merged
merged 17 commits into from
Oct 16, 2024
Merged

feat: add fixed_window bucket option #79

merged 17 commits into from
Oct 16, 2024

Conversation

pubalokta
Copy link

@pubalokta pubalokta commented Sep 17, 2024

Original PR by Panga

Add

  • fixed_window bucket option when enabled refill at specified interval instead of granular.
  • fixed_window optional parameter in the take and takeElevated methods. The parameter takes precedence over the bucket option, making it easier to activate/deactivate using a feature flag on the client side.

Used for enforcing a rate limit policy that does not allow allow request rate to go over a the size in a given interval.

Examples

Default behavior
buckets = {
  ip: {
    size: 1000,
    per_second: 1000
  }
}

Result: Refill 1 token each 1 millisecond.

New fixed_window: true
buckets = {
  ip: {
    size: 1000,
    per_second: 1000,
    fixed_window: true
  }
}

Result: Refill 1000 tokens each 1000 milliseconds.

@pubalokta pubalokta requested a review from a team as a code owner September 17, 2024 14:36
@pubalokta pubalokta marked this pull request as draft September 19, 2024 09:01
@pubalokta pubalokta marked this pull request as ready for review September 25, 2024 09:11
@pubalokta pubalokta marked this pull request as draft September 26, 2024 11:04
@pubalokta pubalokta marked this pull request as ready for review October 7, 2024 15:14
Copy link

@kampde kampde left a comment

Choose a reason for hiding this comment

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

couldn't really review all, will continue tomorrow.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
### Use of fixed window on Take and TakeElevated
If you want to use fixed window on Take or TakeElevated, you can do so by setting the `fixed_window` property in the bucket configuration to `true` (default `false`). This will refill the bucket at the specified interval instead of granular.

On top of that, you can use the `fixed_window` property in the `configOverride` parameter to safely activate/deactivate the new fixed_window algorithm from the client on-demand.
Copy link

Choose a reason for hiding this comment

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

What would be the benefit or use case of being able to enable this on demand? What would happen if clients are doing requests with fixed window on and off all the time?

Copy link
Author

@pubalokta pubalokta Oct 8, 2024

Choose a reason for hiding this comment

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

What would be the benefit or use case of being able to enable this on demand?

We would be able to do a controlled rollout. Nowadays everyone is using sliding window; we want to introduce the fixed window algorithm but wanna make sure the transition is controlled. If something doesn't work as expected sending the fixed_window=false (feature flag) param will fallback to sliding window

What would happen if clients are doing requests with fixed window on and off all the time?

That shouldn't happen. It's the same risk for clients calling take with and without overrides.

@pubalokta pubalokta force-pushed the feat-fixed-window branch 3 times, most recently from dff9633 to 6e050bb Compare October 11, 2024 09:20
README.md Show resolved Hide resolved
README.md Outdated

If you want to use fixed window algorithm on Take or TakeElevated, you can do so by setting the `fixed_window` property in the bucket configuration to `true` (default `false`). This will refill the bucket at the specified interval instead of granular.

On top of that, you can use the `fixed_window` property in the `configOverride` parameter to safely activate/deactivate the new fixed_window algorithm from the client on-demand. This parameter acts as a feature flag to safely deploy the change to produciton.
Copy link

Choose a reason for hiding this comment

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

typo

Suggested change
On top of that, you can use the `fixed_window` property in the `configOverride` parameter to safely activate/deactivate the new fixed_window algorithm from the client on-demand. This parameter acts as a feature flag to safely deploy the change to produciton.
On top of that, you can use the `fixed_window` property in the `configOverride` parameter to safely activate/deactivate the new fixed_window algorithm from the client on-demand. This parameter acts as a feature flag to safely deploy the change to production.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

lib/take_elevated.lua Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@pubalokta pubalokta merged commit 6ecb349 into master Oct 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants