search

Jblew / firebase-functions-rate-limiter

Js/ts library that allows you to set per-time, per-user or per-anything limits for calling Firebase cloud functions
MIT License
100 stars 15 forks source link

REFACTOR: Store timestamps using milliseconds instead of seconds #41

Open Jeandcc opened 1 year ago

Jeandcc commented 1 year ago

Solves https://github.com/Jblew/firebase-functions-rate-limiter/issues/40

Motivation / Context

After running some tests with this repository, I was running into an issue of requests often not being accounted if they happened within one second from one another.

With the approach proposed by the pull request, the issue described above will happen less often, given that they would need to happen at the exact millisecond in order to overlap and cause requests to not be accounted.

Testing Instructions / How This Has Been Tested

Tests were slightly updated to account for the fact that timestamps will now be stored in milliseconds instead of seconds.

At a later point, we might want to simply rewrite the method setTimestampSeconds and its callers to start using milliseconds instead of seconds.

For now, we just multiplied the seconds by 1000 so we could get the ms. value.

Jblew commented 1 year ago

Wow! This is awesome change! I just had a look and I'm thrilled to merge it in. I'll run some tests on my functions on monday (max tuesday) and if that's ok I'm going to merge it! If this works, which I strongly believe, this is going to bring the limiter to another level with more granularity.

I'm curious — what was your use case? Did you deploy functions with this limiter at scale? If so, which backend did you use? I see you develop on Windows? (cross-env and shx)

Thanks again, looking forward to testing and merging, Best wishes, JJ

Jeandcc commented 1 year ago

Hey @Jblew!

I'm glad to see all the enthusiasm with this PR. There's no rush from my end, take your time to review it!

I must say that I loved the way this codebase was built, so kudos to you and to anyone that assisted you! Your effort towards implementing SOLID principles was great, and it made the process of understanding the codebase very easy!

As for my use-case: The way I deploy it or where I deploy it weren't really factors into me suggesting this change to the codebase. I was simply running some tests with this package while I had the database view opened, and I saw that even after 10 simultaneous API calls, the rate-limiting collection was still only registering one API call. After some digging, I found out that it was tracking the timestamp in seconds instead of milliseconds.

As for my development environment, yes! I do use windows because it's the OS in the machine that my company issued me. I wasn't able to make it fully compatible to Windows because the test and do_verify scripts use 'xargs', which I couldn't find a replacement in a timely manner. So, for running tests, I used my Linux device.

I'll be happy to answer any other questions you might have. All the best!

Jeandcc commented 1 year ago

Hi @Jblew!

Did you spot anything that needs to be reworked before merging this PR? Happy to tackle that if that's the case.

chicagocomputerclasses commented 1 year ago

Was this merged?