tyk-docs icon indicating copy to clipboard operation
tyk-docs copied to clipboard

[DX-1337, TT-12124] Update rate limiter docs with Fixed Window Rate Limiter

Open titpetric opened this issue 9 months ago • 8 comments

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

titpetric avatar May 13 '24 16:05 titpetric

PR Description updated to latest commit (https://github.com/TykTechnologies/tyk-docs/commit/173a2789dcf2b93116d0b316cd850ed8603084bd)

github-actions[bot] avatar May 13 '24 16:05 github-actions[bot]

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 filetyk-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 lineThe default behaviour with the Redis Rate Limiter is that the rate-limit calculations are performed on-thread.

relevant filetyk-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 filetyk-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 lineThe 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 filetyk-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

github-actions[bot] avatar May 13 '24 16:05 github-actions[bot]

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar May 13 '24 16:05 netlify[bot]

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Add an explanation of drl_threshold to clarify its impact on rate limiter selection

Consider providing a brief explanation or example of how the drl_threshold value affects
the choice between DRL and Redis Rate Limiter to enhance understanding for users
unfamiliar with these concepts.

tyk-docs/content/getting-started/key-concepts/rate-limiting.md [72]

-Tyk switches between these two modes using the `drl_threshold`.
+Tyk switches between these two modes using the `drl_threshold`, which determines the cutoff point for choosing between the DRL and Redis Rate Limiter based on request rates.
 
Suggestion importance[1-10]: 7

Why: The suggestion correctly identifies an area where additional explanation could help users understand the functionality better. It is relevant and enhances the documentation.

7
Add a direct link to detailed information on the sliding window log algorithm

To enhance the documentation's utility, consider linking directly to a section that
explains the sliding window log algorithm in more detail, rather than just linking to the
Redis documentation homepage.

tyk-docs/content/getting-started/key-concepts/rate-limiting.md [76]

-The Redis Rate Limiter provides 100% accuracy, however instead of using the token bucket algorithm it uses the sliding window log algorithm.
+The Redis Rate Limiter provides 100% accuracy, however instead of using the token bucket algorithm it uses the sliding window log algorithm. For more details on this algorithm, see [Sliding Window Log Algorithm Details](https://redis.io/topics/data-types-intro#redis-lists).
 
Suggestion importance[1-10]: 7

Why: 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 section

To improve clarity and accuracy, consider revising the description of the Redis Rate
Limiter's impact to specify what "minimal impact" entails, especially in terms of memory
and CPU usage.

tyk-docs/content/getting-started/key-concepts/rate-limiting.md [86-87]

-> Redis impact is minimal. A simple counter with expiry is created for every window, and removed when the window elapses.
+> Redis impact is minimal, primarily affecting memory usage with a simple counter that expires and is removed after each window. CPU usage is not significantly impacted.
 
Suggestion importance[1-10]: 6

Why: 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 Limiter

It would be beneficial to add a note on potential drawbacks or limitations of the Fixed
Window Rate Limiter, such as its susceptibility to rate spikes just before the window
resets, which can lead to uneven traffic distribution.

tyk-docs/content/getting-started/key-concepts/rate-limiting.md [80]

-The Fixed Window Rate Limiter will limit the number of requests in a particular window in time.
+The Fixed Window Rate Limiter will limit the number of requests in a particular window in time. Note: This method can lead to uneven traffic distribution, especially if a large number of requests occur just before the window resets.
 
Suggestion importance[1-10]: 6

Why: Adding a note about potential drawbacks of the Fixed Window Rate Limiter is beneficial for a more balanced understanding of its behavior.

6

github-actions[bot] avatar May 13 '24 16:05 github-actions[bot]

@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)

titpetric avatar May 14 '24 13:05 titpetric

PR looks good to me. Thanks.

andyo-tyk avatar May 15 '24 10:05 andyo-tyk

@andrei-tyk @lghiur Are you ok to approve this ready for merge and release?

dcs3spp avatar May 16 '24 08:05 dcs3spp

@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. 🙏

andyo-tyk avatar May 16 '24 08:05 andyo-tyk

@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

dcs3spp avatar May 17 '24 15:05 dcs3spp