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

APIs to reset consumed tokens and get remaining tokens #84

Merged
merged 38 commits into from
Apr 15, 2024

Conversation

sourabpramanik
Copy link
Contributor

@sourabpramanik sourabpramanik commented Feb 29, 2024

This PR adds two new APIs:

  • Reset used tokens
  • Get remaining tokens
  • Add tests for both APIs

Feat: #80

Copy link

vercel bot commented Feb 29, 2024

@sourabpramanik is attempting to deploy a commit to the Upstash Team on Vercel.

A member of the Team first needs to authorize it.

@ogzhanolguncu
Copy link
Collaborator

Looks good, and let's add a reset button to next.js example

@sourabpramanik
Copy link
Contributor Author

For the get remaining API, do you guys have any suggestions for the implementation? I am trying to figure out a way where every algorithm will have their own method to retrieve the remaining tokens because the data type for each algorithm is different. So please let me know if you have any better ideas.

@ogzhanolguncu
Copy link
Collaborator

ogzhanolguncu commented Mar 6, 2024

I believe that as long as the commands remains in Lua script, it should be acceptable. We don't want to cause more round-trips than necessary.

@sourabpramanik sourabpramanik marked this pull request as draft March 10, 2024 11:21
@sourabpramanik
Copy link
Contributor Author

sourabpramanik commented Mar 10, 2024

@ogzhanolguncu @enesakar These are my proposed changes, the implementation of the algorithm does not change just that I have changed the Algorithm function return signature so that we can add get remaining tokens API for each different window as they use different methods to store tokens in Redis.

Changes applied to single region fixed window algorithm

@ogzhanolguncu
Copy link
Collaborator

const ratelimit = new Ratelimit({
  redis: kv,
  limiter: {getRemaining: _,limit: _},
});

Why do we expose getRemaining and limit to outside when they can't even initialize it that way?

I think initializer config signature should be the same. It's a bit confusing both for the users and devs. Imagine firing up your intellisense and seeing getRemaining there, but you can't do anything, I believe it's just confusing.

We should find another way to append getRemaining to ratelimit instance

@sourabpramanik
Copy link
Contributor Author

The signature doesn't have to be like this it can work like it used to do by providing the limiter function. I will refactor this

@sourabpramanik
Copy link
Contributor Author

const ratelimit = new Ratelimit({
  redis: kv,
  limiter: {getRemaining: _,limit: _},
});

Why do we expose getRemaining and limit to outside when they can't even initialize it that way?

I think initializer config signature should be the same. It's a bit confusing both for the users and devs. Imagine firing up your intellisense and seeing getRemaining there, but you can't do anything, I believe it's just confusing.

We should find another way to append getRemaining to ratelimit instance

Yeah I actually missed the intellisense 😅

@sourabpramanik
Copy link
Contributor Author

const ratelimit = new Ratelimit({
  redis: kv,
  limiter: {getRemaining: _,limit: _},
});

Why do we expose getRemaining and limit to outside when they can't even initialize it that way?

I think initializer config signature should be the same. It's a bit confusing both for the users and devs. Imagine firing up your intellisense and seeing getRemaining there, but you can't do anything, I believe it's just confusing.

We should find another way to append getRemaining to ratelimit instance

@ogzhanolguncu How about this implementation?

src/lua-scripts/single.ts Show resolved Hide resolved
src/ratelimit.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/resetUsedTokens.test.ts Outdated Show resolved Hide resolved
src/ratelimit.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@CahidArda CahidArda left a comment

Choose a reason for hiding this comment

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

hi @sourabpramanik,

Thanks a lot for the changes! I think the changes look good overall. I only made a small fix in 4c9002d.

I feel like we can leave the discussion about changing the API for another time.

@sourabpramanik
Copy link
Contributor Author

Thanks a lot for the changes! I think the changes look good overall. I only made a small fix in 4c9002d.

Thanks @CahidArda

@sourabpramanik
Copy link
Contributor Author

I feel like we can leave the discussion about changing the API for another time.

Yes I believe we can create a new issue for the same and discuss it throughout

Copy link
Collaborator

@ogzhanolguncu ogzhanolguncu left a comment

Choose a reason for hiding this comment

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

Looks great

@ogzhanolguncu ogzhanolguncu merged commit 1cb10c3 into upstash:main Apr 15, 2024
0 of 2 checks passed
@ogzhanolguncu ogzhanolguncu changed the title APIs to reset consumed tokens and get remaining tokens (WIP) APIs to reset consumed tokens and get remaining tokens Apr 15, 2024
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.

None yet

3 participants