New API

[ruslandoga]

This PR implements a new API #72 (comment) inspired by https://github.com/michalmuskala/plug_attack, https://github.com/farhadi/rate_limiter, and Ecto.Repo.

Closes #71

TODOs:

• update Hammer.Plug added documentation and updated CHANGELOG
• write the upgrade guide done
• maybe add integration tests that can be used by all backends (similar to Ecto)?
• add more ETS-based backends with different rate-limiting strategies (sliding window, leaky bucket, token bucket)?
• add atomics-based backends?

[epinault]

Thank you for the contributions. Overall I like the new direction of instantiating a backend allowing you to use multiple backend in parallel . API looks good but I left some comments and concerns. Before releasing 7.0, I would want also fix at a minimum the redis backend along. As most people use that one. I also like the idea of a distributed ETS, so why not shipping this as separate MR? and then we could have one with Atomic as well separately? I could also see after that a different algorithm for window bucket

[ruslandoga]

I've updated the Redis backend: ExHammer/hammer-backend-redis#56

I'll update Mnesia backend tomorrow. It would probably use https://www.erlang.org/doc/apps/mnesia/mnesia.html#dirty_update_counter/2

[ruslandoga]

I'm not sure about the Plug. I'll try to update it but right now I don't actually see much benefit from it.

Like, why is it better than

plug :rate_limit_videos when action in ...

defp rate_limit_videos(conn, _opts) do
  user_id = conn.assigns.current_user.id
  key = "videos:#{user_id}"
  scale = :timer.minutes(1)
  limit = 10

  case MyApp.RateLimit.hit(key, scale, limit) do
    {:allow, _count} ->
      conn

    {:deny, retry_after} ->
      conn
      |> put_resp_header("retry-after", Integer.to_string(div(retry_after, 1000)))
      |> send_resp(429, [])
      |> halt()
  end
end 

It doesn't even set any useful headers on the response. Just adds a confusing (IMO) abstraction. If it was up to me (it's not, I know) I would probably deprecate it and replace it with a guide :)

[epinault]

I'm not sure about the Plug. I'll try to update it but right now I don't actually see much benefit from it.

Like, why is it better than

plug :rate_limit_videos when action in ...

defp rate_limit_videos(conn, _opts) do
  user_id = conn.assigns.current_user.id
  key = "videos:#{user_id}"
  scale = :timer.minutes(1)
  limit = 10

  case MyApp.RateLimit.hit(key, scale, limit) do
    {:allow, _count} ->
      conn

    {:deny, retry_after} ->
      conn
      |> put_resp_header("retry-after", Integer.to_string(div(retry_after, 1000)))
      |> send_resp(429, [])
      |> halt()
  end
end 

It doesn't even set any useful headers on the response. Just adds a confusing (IMO) abstraction. If it was up to me (it's not, I know) I would probably deprecate it and replace it with a guide :)

I am game to remove it. it might be useful for some people to have an overall throtling but then we could provide them a guide to do so instead for now

[ruslandoga]

it might be useful for some people to have an overall throtling

It can also be added it to the endpoint module. Or the main router.

defmodule MyAppWeb.Endpoint do
  use Phoenix.Endpoint
  
  plug RemoteIP
  plug :rate_limit
  
  # ...
  
  defp rate_limit(conn, _opts) do
    key = "web_requests:#{:inet.ntoa(conn.remote_ip)}"
    scale = :timer.minutes(1)
    limit = 1000
    
    case MyApp.RateLimit.hit(key, scale, limit) do
      {:allow, _count} ->
        conn

      {:deny, retry_after} ->
        retry_after_seconds = div(retry_after, 1000)

        conn
        |> put_resp_header("retry-after", Integer.to_string(retry_after_seconds))
        |> send_resp(429, [])
        |> halt()
    end
  end
end

---

I think we can add a deprecation warning now, in init/1. It would be called once, only during compilation (unless in development), and it can print a similar "custom" made plug based on the options supplied.

[ruslandoga]

I've updated the Mnesia backend: ExHammer/hammer-backend-mnesia#36

[epinault]

I think you have some conflicts to resolve

[ruslandoga]

I think this was merged prematurely :)

Documentation needs more work. And the code too ...

What needs to be simplified / edited:

• https://github.com/ExHammer/hammer/tree/master?tab=readme-ov-file#default-algorithm
• https://github.com/ExHammer/hammer/tree/master?tab=readme-ov-file#example-usage
• https://github.com/ExHammer/hammer/blob/master/guides/Tutorial.md was just a draft :) The code examples need to be "copy-pastable", right now some of them contain syntax errors
• https://github.com/ExHammer/hammer/blob/master/guides/upgrade-v7.md needs quick and clear step-by-step instructions with code examples, right now it's not very useful and contains too much text, people won't read it. They would just scan it and do trial and error upgrade -- it can be much better
• https://github.com/ExHammer/hammer/blob/master/guides/distributed-ets.md was just a draft ...

[epinault]

Yes but that MR was getting too large and I backfilled some docs like the upgrade and few other things. Since this is an RC and I have not published it yet we are fine .

[epinault]

I'll fix up these file separately or you can submit PR on those items . Separating would be nice to avoid the PR to linger too long too

[epinault]

looking at them while not perfect I am not seeing what s wrong with them . this is doc and can be easily fix up. I am going to release it

as for the upgrade guide, it showing in order what you need to do to upgrade. it s pretty clear I would think and they can easily use AI and the step as prompt instructions. The better way would be to use igniter and provide and upgrade recipe to do it for them but would not be 100% prefect and require quite a bit of time to build

[ruslandoga]

I’d like the opportunity to decide when my work is complete before it’s included in a release. Could we hold off on releasing these changes until I confirm they’re ready?

And could we avoid merging backend PRs that still have TODOs in them, even if their purpose isn’t immediately clear? I’d also appreciate it if the TODOs could be left as-is for now.

I understand that it’s within your ability to merge PRs without prior discussion and to push commits on top, but I’d really appreciate the chance to discuss it first. It seems that some of the key details behind the changes I’ve made—and the overall vision I had in mind—might not be fully understood.

[epinault]

@ruslandoga I appreciate your contributions, but I need this project to move forward. So This is why I am pushing forward. I have been planning on similar work earlier this year and could not get to it. Your MR motivated me to get it moving. but it s taking a little longer and I do not want to duplicate the effort. So I am not quite following what is the problem to keep it moving forward here? I am just helping to move things forward for the community, Credits are given to you.

I have been doing OSS for a long time and frequently leave MR that the owner can choose to merge or not. I am confused what the issue is here? OSS is about collaboration and I appreciate your collaboration but not getting why you are having an issue with this work? I have had to take over MR that were issues in other tools/lib and fix them up to push it forward. Matter of fact this repo was dead for 4 years when I took it and it started to cause issues. so I have been doing the maintenance for while and clean up a lot of MR that were forgotten and unfinished as well as rework a lot to get it moved .

I can collaborate with you however you need, I had no idea this was an issue for you for me to contribute to your branch

As for the TODO, every project has different guideline and style, see the credo file I have committed which does not allow for TODO, I much prefer using issue to track down the work left as TODO get lost.

[epinault]

If you don't think you can work that way -- which is standard for most of OSS -- please revert all of my contributions.

I can work with you and we can chat maybe to have a better understanding with each others on how to collaborate moving forward. Would you like me to still remove it?

[ruslandoga]

Thank you for your thoughtful response and your willingness to collaborate further. I appreciate the effort you've put into maintaining this project and your openness to working together.

That said, open-source contributions are something I do for personal satisfaction and the joy of completing tasks to my own standards. This experience hasn't aligned with those motivations, so I believe it's best for me to move on.

Please feel free to use the work I've already contributed, and I wish you and the project continued success.