Uh oh!
There was an error while loading. Please reload this page.
Add Edge Rate Limiting (ERL) Support - #45
Conversation
This is a pretty direct wrapping of the WIT with the addition of The composite `EdgeRateLimiter` which matches what is provided by the Rust SDK with `ERL`. Viceroy mostly has stubs for this functionality at this point in time, so the tests are not particularly substantive but do try to at least put some tracers through the hostcall boundary. There are several xfail tests which could be made to fail properly with viceory changes to do more faithfully match parts of the production impl.
Uh oh!
There was an error while loading. Please reload this page.
Lints for python 3.14 disallow string quoting types; importing annotations from __future__ defers type avaluations to avoid this problem and make the linter happy.
erikrose
left a comment
There was a problem hiding this comment.
Good wrapper. Just a couple of nits noticed.
On notes of philosophy, since we were just talking about that…
This one's value is in __contains__ and in the docs. Otherwise, it wouldn't need to exist. I think it's fine to add this, but it's also worth spending an hour or two thinking about whether these (+ all their tests) need to exist in such verbose fashion. Maybe there's a slimmer way where we can "add only the additions".
Thanks, Paul! Enjoy your weekend!
| from fastly_compute.erl import RateCounter, PenaltyBox | ||
| # Basic rate limiting | ||
| with RateCounter.open("api-counter") as counter: |
| :param name: The name of the rate counter | ||
| :return: RateCounter instance | ||
| :raises ~fastly_compute.exceptions.types.open_error.NotFound: If the rate counter doesn't exist | ||
| :raises ~fastly_compute.exceptions.types.open_error.InvalidSyntax: If the name is invalid |
There was a problem hiding this comment.
Do we know what constitutes a valid or too-long name? It'd be great to include those here so the descriptions actually convey some additional information.
There was a problem hiding this comment.
I hadn't spelunked XQD on this one as of yet, these ended up being copied from another open but it appears that the validation here is different. The ERL specific validation seems limited, but I think the safer option might be to reference the common base class for open errors (probably as a general course).
The lack of enforcement explicitly for name lengths and the like on some of these might be indicative of an issue as I think this could be abused to cause excessive memory allocations on the host (@dgohman-fastly referred to this in passing today).
I'll look at this angle a bit more next week.
There was a problem hiding this comment.
I'm glad you explained InvalidSyntax, as I couldn't have guessed its meaning in this context! But I wouldn't be at all opposed to adding "This may can also raise any other OpenError" or similar. That is, when it comes down to it, the spec enforced by the WIT.
| :param entry: Identifier for the client (e.g., IP address) | ||
| :param delta: Amount to increment the counter by | ||
| :param window: Time window in seconds for rate calculation. The host validates |
There was a problem hiding this comment.
Booyah: thank you for including the units for all of these. That's a question I had while reading the WIT. We should backport that stuff to the WIT.
| def get_name(self) -> str: | ||
| """Return the name of this penalty box. | ||
| :return: The name of the penalty box |
There was a problem hiding this comment.
A couple of places we have a :return: that just restates the first line of the docstring. I think we can save people's time by deleting them.
| Combines a :class:`RateCounter` and :class:`PenaltyBox` into a single | ||
| interface for simplified rate limiting operations. | ||
| :param rate_counter: Rate counter to use for counting |
There was a problem hiding this comment.
We needn't repeat what's in __init__().
| } | ||
| @on_viceroy | ||
| def rate_counter_open(cls, name): |
There was a problem hiding this comment.
I wouldn't say no to deleting this one and saving 3s on CI, as the next one does a superset of this. Just note in its docstring that it also tests opening. (However, if the retval should actually be used, disregard this ¶.)
The return value is unused; are you just smoketesting? If get_name() works under Viceroy, we ought to assert something about it.
| """Increment a counter and return None (no error).""" | ||
| with RateCounter.open(counter_name) as counter: | ||
| counter.increment(entry, delta) | ||
| return None |
There was a problem hiding this comment.
No need to explicitly return anything here, and there's no advantage in asserting that it returns None, as is done in test_increment(). If there were an error, it would raise an exception.
| is_limited = self.rate_counter_check_rate( | ||
| "test-counter", "test-penalty", "192.168.1.1", 1, 10, 100, 300 | ||
| ) | ||
| assert is_limited is False # Viceroy stub returns False |
There was a problem hiding this comment.
Thanks for the good comments about how Viceroy's stubs behave!
| """EdgeRateLimiter convenience wrapper tests.""" | ||
| VICEROY_CONFIG = { | ||
| "local_server": { |
There was a problem hiding this comment.
All 3 classes use similar fixtures. If we ever get sick of waiting for 3 different wasms to build, we could merge the classes or, fancier but more work, add some automatic test fixture coalescing. Back in the Django days, I had to write that, but I think pytest is smart enough to do it itself if we model it right.
There was a problem hiding this comment.
I'll try to reduce these; initially I had hoped there might have been a bit more I could do with configuration to aid testing using Viceroy but that wasn't the case (I considered building that out but decided to avoid that trail for now).
| """Add entry to penalty box.""" | ||
| with PenaltyBox.open(penalty_name) as penalty: | ||
| penalty.add(entry, ttl) | ||
| return None |
There was a problem hiding this comment.
Again, the return None and matching assert can go.
| self.close() | ||
| class PenaltyBox: |
There was a problem hiding this comment.
I just realized this whole class can be replaced with…
classPenaltyBox(wit_erl.PenaltyBox):
def__contains__(self, entry: str) ->bool:
"""Check if entry is in the penalty box using the 'in' operator. :param entry: Identifier to check :return: True if the entry is blocked, False otherwise :raises ~fastly_compute.exceptions.types.error.InvalidArgument: If parameters are invalid :raises ~fastly_compute.exceptions.types.error.GenericError: If an unexpected error occurs Example:: with PenaltyBox.open("blocklist") as penalty: if "192.168.1.1" in penalty: return Response("Blocked", status=403) """returnself.has(entry)…plus some Sphinx to add nicer docstrings to the other routines. The type hints in the stubs are even good. The only functional difference is that erl.PenaltyBox will have a has(), which isn't even a bad thing. I feel silly for not noticing this earlier.
Subclassing saves a bunch of indirection and makes the things we're actually adding stand out.
| from wit_world.imports import erl as wit_erl | ||
| class RateCounter: |
There was a problem hiding this comment.
After looking at PenaltyBox first, I realize this one is nothing but docs and can be turned into a bunch of RST.
posborne
commented
Jun 1, 2026
Closing this for now but keeping the changes around; ERL from this base was included as part of the #84 changes, more heavily leaning on the generated code layer based on some of the discussion here. I'm going to keep the branch around for now in case we abandon the approach proposed there. |
This is a pretty direct wrapping of the WIT with the addition of The composite
EdgeRateLimiterwhich matches what is provided by the Rust SDK withERL.Viceroy mostly has stubs for this functionality at this point in time, so the tests are not particularly substantive but do try to at least put some tracers through the hostcall boundary. There are several xfail tests which could be made to fail properly with viceory changes to do more faithfully match parts of the production impl.