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

new ets #72

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .formatter.exs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Used by "mix format"
[
inputs: ["{mix,.formatter}.exs", "{config,lib,test}/**/*.{ex,exs}"]
inputs: ["{mix,.formatter}.exs", "{lib,test}/**/*.{ex,exs}"]
epinault marked this conversation as resolved.
Show resolved Hide resolved
]
8 changes: 3 additions & 5 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,9 @@ jobs:
fail-fast: false
matrix:
os: [ubuntu-22.04, ubuntu-20.04]
elixir_version: [1.12.3, 1.13.3, 1.14.1]
otp_version: [24, 25]
exclude:
- otp_version: 25
elixir_version: 1.12.3
elixir_version: [1.13, 1.14, 1.15]
ruslandoga marked this conversation as resolved.
Show resolved Hide resolved
otp_version: [24, 25, 26]

steps:
- uses: actions/checkout@v4

Expand Down
2 changes: 1 addition & 1 deletion .tool-versions
Original file line number Diff line number Diff line change
@@ -1 +1 @@
elixir 1.13.1
epinault marked this conversation as resolved.
Show resolved Hide resolved
elixir 1.15.7-otp-26
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## 7.0.0 - unreleased

- remove `poolboy` dependency
- improve ETS backend efficiency ~15x
Copy link
Contributor Author

@ruslandoga ruslandoga Nov 25, 2023

Choose a reason for hiding this comment

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

Under load it's actually x40+

x15 (according to Benchee) is when Poolboy is not overloaded

https://github.com/ruslandoga/rate_limit

- fix ETS backend overload
- remove global supervisor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This log is incomplete, I'll add more changes if the approaches in this PR are approved.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if we are going to make 7.0 , I would like to make another braking change. So maybe let’s just merge in master without bumping the version yet ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to see a migrating plan in the changelog from config to the supervisor

## 6.1.0 - 2022-06-13

### Changed
Expand Down
12 changes: 1 addition & 11 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ by adding `:hammer` to your list of dependencies in `mix.exs`:
```elixir
def deps do
[
{:hammer, "~> 6.1"}
{:hammer, "~> 7.0"}
]
end
```
Expand All @@ -38,7 +38,6 @@ Example:

```elixir
defmodule MyApp.VideoUpload do

def upload(video_data, user_id) do
case Hammer.check_rate("upload_video:#{user_id}", 60_000, 5) do
{:allow, _count} ->
Expand All @@ -47,7 +46,6 @@ defmodule MyApp.VideoUpload do
# deny the request
end
end

end
```

Expand All @@ -58,14 +56,6 @@ The `Hammer` module provides the following functions:
- `inspect_bucket(id, scale_ms, limit)`
- `delete_buckets(id)`

Backends are configured via `Mix.Config`:

```elixir
config :hammer,
backend: {Hammer.Backend.ETS, [expiry_ms: 60_000 * 60 * 4,
cleanup_interval_ms: 60_000 * 10]}
```

See the [Tutorial](https://hexdocs.pm/hammer/tutorial.html) for more.

See the [Hammer Testbed](https://github.com/ExHammer/hammer-testbed) app for an example of
Expand Down
12 changes: 0 additions & 12 deletions config/config.exs

This file was deleted.

210 changes: 42 additions & 168 deletions lib/hammer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,20 @@ defmodule Hammer do
@moduledoc """
Documentation for Hammer module.

This is the main API for the Hammer rate-limiter. This module assumes a
backend pool has been started, most likely by the Hammer application.
This is the main API for the Hammer rate-limiter. This module assumes the
backend has been started.

Example:

def start(_, _) do
children = [{Hammer.Backend.ETS, cleanup_interval_ms: :timer.seconds(5)}]
Supervisor.init(children, [])
end

"""

alias Hammer.Utils
@backend Application.compile_env(:hammer, :backend, Hammer.Backend.ETS)

@spec check_rate(id :: String.t(), scale_ms :: integer, limit :: integer) ::
{:allow, count :: integer}
| {:deny, limit :: integer}
| {:error, reason :: any}
@doc """
Check if the action you wish to perform is within the bounds of the rate-limit.

Expand All @@ -22,7 +26,7 @@ defmodule Hammer do
- `scale_ms`: Integer indicating size of bucket in milliseconds
- `limit`: Integer maximum count of actions within the bucket

Returns either `{:allow, count}`, `{:deny, limit}` or `{:error, reason}`
Returns either `{:allow, count}`, `{:deny, limit}`

Example:

Expand All @@ -34,85 +38,35 @@ defmodule Hammer do
# render an error page or something
end
"""
@spec check_rate(id :: String.t(), scale_ms :: integer, limit :: integer) ::
{:allow, count :: integer} | {:deny, limit :: integer} | {:error, reason :: any}
def check_rate(id, scale_ms, limit) do
check_rate(:single, id, scale_ms, limit)
check_rate_inc(id, scale_ms, limit, 1)
end

@spec check_rate(backend :: atom, id :: String.t(), scale_ms :: integer, limit :: integer) ::
{:allow, count :: integer}
| {:deny, limit :: integer}
| {:error, reason :: any}
@doc """
Same as `check_rate/3`, but allows specifying a backend.
"""
def check_rate(backend, id, scale_ms, limit) do
{stamp, key} = Utils.stamp_key(id, scale_ms)

case call_backend(backend, :count_hit, [key, stamp]) do
{:ok, count} ->
if count > limit do
{:deny, limit}
else
{:allow, count}
end

{:error, reason} ->
{:error, reason}
end
end

@spec check_rate_inc(
id :: String.t(),
scale_ms :: integer,
limit :: integer,
increment :: integer
) ::
{:allow, count :: integer}
| {:deny, limit :: integer}
| {:error, reason :: any}
@doc """
Same as check_rate/3, but allows the increment number to be specified.
This is useful for limiting apis which have some idea of 'cost', where the cost
of each hit can be specified.
"""
def check_rate_inc(id, scale_ms, limit, increment) do
check_rate_inc(:single, id, scale_ms, limit, increment)
end

@spec check_rate_inc(
backend :: atom,
id :: String.t(),
scale_ms :: integer,
limit :: integer,
increment :: integer
increment :: non_neg_integer
) ::
{:allow, count :: integer}
| {:deny, limit :: integer}
| {:error, reason :: any}
@doc """
Same as check_rate_inc/4, but allows specifying a backend.
"""
def check_rate_inc(backend, id, scale_ms, limit, increment) do
{stamp, key} = Utils.stamp_key(id, scale_ms)

case call_backend(backend, :count_hit, [key, stamp, increment]) do
{:ok, count} ->
if count > limit do
{:deny, limit}
else
{:allow, count}
end
{:allow, count :: integer} | {:deny, limit :: integer} | {:error, reason :: any}
def check_rate_inc(id, scale_ms, limit, increment) do
now = System.system_time(:millisecond)
now_bucket = div(now, scale_ms)
key = {id, now_bucket}
expires_at = (now_bucket + 1) * scale_ms

{:error, reason} ->
{:error, reason}
with {:ok, count} <- @backend.count_hit(key, increment, expires_at) do
if count <= limit, do: {:allow, count}, else: {:deny, limit}
end
end

@spec inspect_bucket(id :: String.t(), scale_ms :: integer, limit :: integer) ::
{:ok,
{count :: integer, count_remaining :: integer, ms_to_next_bucket :: integer,
created_at :: integer | nil, updated_at :: integer | nil}}
| {:error, reason :: any}
@doc """
Inspect bucket to get count, count_remaining, ms_to_next_bucket, created_at,
updated_at. This function is free of side-effects and should be called with
Expand All @@ -137,38 +91,29 @@ defmodule Hammer do
{:ok, {1, 2499, 29381612, 1450281014468, 1450281014468}}

"""
def inspect_bucket(id, scale_ms, limit) do
inspect_bucket(:single, id, scale_ms, limit)
end

@spec inspect_bucket(backend :: atom, id :: String.t(), scale_ms :: integer, limit :: integer) ::
@spec inspect_bucket(id :: String.t(), scale_ms :: integer, limit :: integer) ::
{:ok,
{count :: integer, count_remaining :: integer, ms_to_next_bucket :: integer,
created_at :: integer | nil, updated_at :: integer | nil}}
| {:error, reason :: any}
@doc """
Same as inspect_bucket/3, but allows specifying a backend
"""
def inspect_bucket(backend, id, scale_ms, limit) do
{stamp, key} = Utils.stamp_key(id, scale_ms)
ms_to_next_bucket = elem(key, 0) * scale_ms + scale_ms - stamp

case call_backend(backend, :get_bucket, [key]) do
{:ok, nil} ->
{:ok, {0, limit, ms_to_next_bucket, nil, nil}}

{:ok, {_, count, created_at, updated_at}} ->
count_remaining = if limit > count, do: limit - count, else: 0
{:ok, {count, count_remaining, ms_to_next_bucket, created_at, updated_at}}

{:error, reason} ->
{:error, reason}
def inspect_bucket(id, scale_ms, limit) do
now = System.system_time(:millisecond)
now_bucket = div(now, scale_ms)
key = {id, now_bucket}
ms_to_next_bucket = now_bucket * scale_ms + scale_ms - now

with {:ok, count} <- @backend.get_bucket(key) do
{:ok,
{
count,
max(limit - count, 0),
ms_to_next_bucket,
_created_at = nil,
_updated_at = nil
Copy link
Contributor Author

@ruslandoga ruslandoga Nov 25, 2023

Choose a reason for hiding this comment

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

I searched for Hammer.inspect_bucket on github and it doesn't seem like anyone is using created_at and updated_at values (almost everyone calls this function to get only ms_to_next_bucket). So I think created_at and updated_at can be removed, especially since created_at in ETS backend is incorrect after table pruning (cleanup).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but we don’t know how people use it in private repo ?

Copy link
Contributor Author

@ruslandoga ruslandoga Dec 22, 2023

Choose a reason for hiding this comment

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

I suggest dropping these values and reintroducing them back if someone complains. Personally, I don't see a scenario where they are useful, especially with ETS backend which cleans up the keys and makes created_at inaccurate.

Copy link

Choose a reason for hiding this comment

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

@ruslandoga I'd suggest adding a deprecation warning first, and then getting rid of them in the version after. That's how Elixir and core libraries do it.

}}
end
end

@spec delete_buckets(id :: String.t()) ::
{:ok, count :: integer}
| {:error, reason :: any}
@doc """
Delete all buckets belonging to the provided id, including the current one.
Effectively resets the rate-limit for the id.
Expand All @@ -186,79 +131,8 @@ defmodule Hammer do
{:ok, _count} = delete_buckets("file_uploads:\#{user_id}")

"""
@spec delete_buckets(id :: String.t()) :: {:ok, count :: integer} | {:error, reason :: any}
def delete_buckets(id) do
delete_buckets(:single, id)
end

@spec delete_buckets(backend :: atom, id :: String.t()) ::
{:ok, count :: integer}
| {:error, reason :: any}
@doc """
Same as delete_buckets/1, but allows specifying a backend
"""
def delete_buckets(backend, id) do
call_backend(backend, :delete_buckets, [id])
end

@spec make_rate_checker(id_prefix :: String.t(), scale_ms :: integer, limit :: integer) ::
(id :: String.t() ->
{:allow, count :: integer}
| {:deny, limit :: integer}
| {:error, reason :: any})
@doc """
Make a rate-checker function, with the given `id` prefix, scale_ms and limit.

Arguments:

- `id_prefix`: String prefix to the `id`
- `scale_ms`: Integer indicating size of bucket in milliseconds
- `limit`: Integer maximum count of actions within the bucket

Returns a function which accepts an `id` suffix, which will be combined with
the `id_prefix`. Calling this returned function is equivalent to:
`Hammer.check_rate("\#{id_prefix}\#{id}", scale_ms, limit)`

Example:

chat_rate_limiter = make_rate_checker("send_chat_message:", 60_000, 20)
user_id = 203517
case chat_rate_limiter.(user_id) do
{:allow, _count} ->
# allow chat message
{:deny, _limit} ->
# deny
end
"""
def make_rate_checker(id_prefix, scale_ms, limit) do
make_rate_checker(:single, id_prefix, scale_ms, limit)
end

@spec make_rate_checker(
backend :: atom,
id_prefix :: String.t(),
scale_ms :: integer,
limit :: integer
) ::
(id :: String.t() ->
{:allow, count :: integer}
| {:deny, limit :: integer}
| {:error, reason :: any})
@doc """
"""
def make_rate_checker(backend, id_prefix, scale_ms, limit) do
fn id ->
check_rate(backend, "#{id_prefix}#{id}", scale_ms, limit)
end
end

defp call_backend(which, function, args) do
pool = Utils.pool_name(which)
backend = Utils.get_backend_module(which)

:poolboy.transaction(
pool,
fn pid -> apply(backend, function, [pid | args]) end,
60_000
)
@backend.delete_buckets(id)
end
end
Loading