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

Integrate Ratelimiter Headers and Cron Cleanup Timer with Playground UI #66

Closed
lucifercr07 opened this issue Oct 9, 2024 · 15 comments
Closed

Comments

@lucifercr07
Copy link
Contributor

Description

The goal of this issue is to enhance the Playground UI by integrating ratelimiter headers and a cron cleanup timer. These headers and timer values should be displayed on the UI whenever the page is loaded or when a command response is received as part of an API call.

The ratelimiter headers will show the current rate limits (e.g., the number of commands allowed and the remaining commands for the current window). Additionally, the cron cleanup timer will display the remaining time before the next scheduled cleanup of keys in DiceDB.

image

Tasks

  • Integrate Ratelimit Headers in the UI:

    • Extract x-ratelimit-limit, x-ratelimit-remaining, and x-ratelimit-reset from the API response headers.
    • Display these values in the UI, either in the header or as part of a status bar.
    • Ensure that the values are updated when new commands are executed.
  • Display the cron cleanup timer and update it as necessary by getting info from backend (playground-mono) .

  • Ensure the UI fetches and displays these values on initial page load and on every command response from the server.

  • Write unit tests for the UI components.

  • Verify the rate limit headers are updated as expected when interacting with the command interface.

Cron Cleanup Timer: A scheduled task on the backend will run at configurable intervals to clean up expired keys in DiceDB. The UI should show the time remaining before the next cleanup.

These headers should be extracted from the API responses and displayed in the UI whenever the user executes commands (e.g., /shell/exec/).

Rate Limiting Headers: The API will return the following headers:
x-ratelimit-limit: Maximum number of requests allowed.
x-ratelimit-remaining: Number of requests remaining in the current window.
x-ratelimit-reset: Timestamp of when the rate limit will reset.

@aasifkhan7
Copy link
Contributor

@lucifercr07 I'd like to take this up.

@lucifercr07
Copy link
Contributor Author

@aasifkhan7 assigned, thanks for contributing. Please let me know if any details required regarding this.

@aasifkhan7
Copy link
Contributor

#67
@lucifercr07 I've submitted an initial draft of the PR. I'm still working on figuring out how to fetch the next cleanup time. This is my first contribution to an open-source project, so feedback would be highly appreciated if any improvements are needed. Thanks!

@lucifercr07
Copy link
Contributor Author

@aasifkhan7 would you like to continue work on this, draft PR is raised here: DiceDB/playground-mono#43

@aasifkhan7
Copy link
Contributor

aasifkhan7 commented Oct 21, 2024

Sure @lucifercr07 let me take a look at the PR

@vanshavenger
Copy link

@lucifercr07 also when we execute a command in Playground the first time, the remaining request updates to 998 , it should be 999,
shouldn't the line be addRateLimitHeaders(w, limit, limit-(requestCount), requestCount, currentWindow+int64(window)) this.
as very first request count is 1.

Or is there anything i am missing here, correct me if i am wrong.

@vanshavenger
Copy link

Also regarding on initial page load we have to send a post request to any one of the command to get the headers as the headers are attached to the /shell/cmd/{cmd} endpoint, but this way it will took away one request each time when the page first loads for the user, can we have something like anoither endpoint for this that returns its ratelimits for the initial page load.

Also kindly share the discord link so that i can discuss these pointers there instead of here.

@lucifercr07
Copy link
Contributor Author

@aasifkhan7 any updates on this?

@aasifkhan7
Copy link
Contributor

@lucifercr07, in the PR DiceDB/playground-mono#43, I noticed that the exposed header is x-last-cleanup-time. However, the UI is intended to display "cleanup time left." How can I retrieve the context for the "next cleanup time"?

@lucifercr07
Copy link
Contributor Author

@aasifkhan7 since we know that cron cleanup frequency happens every 15 mins, we can get next cleanup time by time.Unix(lastCleanupTime, 0).Add(cronFrequencyInterval) - time.Now()). Correct?
Let me know if you think we should pass X-Next-Cleanup-Time we can also do that change on backend shouldn't be a big one.

@lucifercr07
Copy link
Contributor Author

@vanshavenger yes we can integrate that with our existing /health API to avoid values being populated after first call. Let me know if you would like to work on this.
Ref: #66 (comment)

@aasifkhan7
Copy link
Contributor

@lucifercr07, I think we should start sending X-Next-Cleanup-Time since the UI lacks visibility into the 15-minute interval set in the backend. Currently, only the backend knows about this frequency, but the UI has no context of it. Sending the next cleanup time directly would simplify things. Also, I'd like to take on the corresponding change in mono (it'd be my first contribution to a Golang project). Let me know your thoughts!

@lucifercr07
Copy link
Contributor Author

@aasifkhan7 please go ahead and implement, created an issue for it: DiceDB/playground-mono#47
Please comment on it so that I can assign.

@aasifkhan7
Copy link
Contributor

@lucifercr07 please review these PRs:
#76
DiceDB/playground-mono#48

@lucifercr07
Copy link
Contributor Author

Closing merged as part of #76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants