tyk-docs
tyk-docs copied to clipboard
[DX-1337, TT-12124] Update rate limiter docs with Fixed Window Rate Limiter
The PR changes the rate limiter documentation so:
- Capitalize Redis, Redis Rate Limiter
- Add Redis impact notes for each option
- Add Fixed Window Rate Limiter section
- Rewrite RRL/Sentinel/DRL Threshold sections to better explain behaviour
JIRA:
- https://tyktech.atlassian.net/browse/TT-12124 (task)
- https://tyktech.atlassian.net/browse/TT-11739 (parent)
For internal users - Please add a Jira DX PR ticket to the subject!
Preview Link
https://deploy-preview-4630--tyk-docs.netlify.app/nightly/docs/getting-started/key-concepts/rate-limiting/#fixed-window-rate-limiter
Description
Screenshots (if appropriate)
Checklist
- [x] I have added a preview link to the PR description.
- [x] I have reviewed the guidelines for contributing to this repository.
- [x] I have read the technical guidelines for contributing to this repository.
- [x] Make sure you have started your change off our latest
master
. - [ ] I labelled the PR
PR Description updated to latest commit (https://github.com/TykTechnologies/tyk-docs/commit/173a2789dcf2b93116d0b316cd850ed8603084bd)
PR Review 🔍
⏱️ Estimated effort to review [1-5] |
2, because the PR mainly involves documentation updates with clear, straightforward changes and a few added sections. The complexity is low and the changes are well-documented and easy to understand. |
🧪 Relevant tests |
No |
⚡ Possible issues |
Possible Inconsistency: The description of the Redis Rate Limiter's impact varies between sections, which might confuse readers. In one section, it mentions significant impact, and in another, it states minimal impact. |
🔒 Security concerns |
No |
Code feedback:
relevant file | tyk-docs/content/getting-started/key-concepts/rate-limiting.md |
suggestion |
Consider using consistent capitalization for "Redis Rate Limiter" throughout the document to maintain uniformity and avoid confusion. [medium] |
relevant line | The default behaviour with the Redis Rate Limiter is that the rate-limit calculations are performed on-thread. |
relevant file | tyk-docs/content/getting-started/key-concepts/rate-limiting.md |
suggestion |
It might be beneficial to clarify the impact of the Redis Rate Limiter in different scenarios to avoid contradictory statements and provide a clearer understanding for the reader. [important] |
relevant line | > Redis impact is significant. For each request, including blocked requests, an entry is written into the sliding window log, increasing memory use and CPU use due to the complexity of the list operations: ZADD, ZREMRANGE, ZCARD. Redis resource usage increases with traffic. |
relevant file | tyk-docs/content/getting-started/key-concepts/rate-limiting.md |
suggestion |
To enhance clarity, consider adding a brief explanation or example of how the Fixed Window Rate Limiter handles traffic bursts differently from other limiters. This could help users better understand its application scenarios. [medium] |
relevant line | The implementation does not handle traffic busts within a window. For any given `rate` in a window, the requests are processed without delay, until the rate limit is reached. If you need spike arrest behaviour, the Redis Rate Limiter should be used, which blocks traffic until the rate decreases. |
relevant file | tyk-docs/content/getting-started/key-concepts/rate-limiting.md |
suggestion |
Suggest adding a visual diagram or flowchart to illustrate how each rate limiting algorithm works, especially for the newly added Fixed Window Rate Limiter. This could greatly aid in understanding complex concepts for visual learners. [medium] |
relevant line | ### Fixed Window Rate Limiter |
PS. Pls add /docs/nightly to the end of url
Name | Link |
---|---|
Latest commit | ed3e37b666899fa641ac7e3160f2b00d186aac67 |
Latest deploy log | https://app.netlify.com/sites/tyk-docs/deploys/6647738bada3d70008cbe117 |
Deploy Preview | https://deploy-preview-4630--tyk-docs.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
PR Code Suggestions ✨
Category | Suggestion | Score |
Enhancement |
Add an explanation of
| 7 |
Add a direct link to detailed information on the sliding window log algorithmTo enhance the documentation's utility, consider linking directly to a section that tyk-docs/content/getting-started/key-concepts/rate-limiting.md [76]
Suggestion importance[1-10]: 7Why: Providing a direct link to detailed information about the sliding window log algorithm enhances the utility of the documentation by making it more informative and user-friendly. | 7 | |
Clarity |
Clarify the description of Redis's impact in the Fixed Window Rate Limiter sectionTo improve clarity and accuracy, consider revising the description of the Redis Rate tyk-docs/content/getting-started/key-concepts/rate-limiting.md [86-87]
Suggestion importance[1-10]: 6Why: This suggestion improves clarity by specifying what "minimal impact" entails, which is useful for understanding the resource usage of Redis in this context. | 6 |
Possible issue |
Highlight potential traffic distribution issues with the Fixed Window Rate LimiterIt would be beneficial to add a note on potential drawbacks or limitations of the Fixed tyk-docs/content/getting-started/key-concepts/rate-limiting.md [80]
Suggestion importance[1-10]: 6Why: Adding a note about potential drawbacks of the Fixed Window Rate Limiter is beneficial for a more balanced understanding of its behavior. | 6 |
@dcs3spp done and done, just give it another read if you're happy (~20 feedback items applied, DRL consistently used as acronym, RRL acronym removed as unused with a single reference)
PR looks good to me. Thanks.
@andrei-tyk @lghiur Are you ok to approve this ready for merge and release?
@dcs3spp I don't think everyone on the list needs to approve this - there's technical approval from @jeffy-mathew and I've given approval so should be OK to go IMHO. 🙏
@dcs3spp I don't think everyone on the list needs to approve this - there's technical approval from @jeffy-mathew and I've given approval so should be OK to go IMHO. 🙏
👌 Thanks @andyo-tyk I will merge to main branch now