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

[TT-12840, DX-1620] Added explanation of bug fix missed in 5.4 RN #5260

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

andyo-tyk
Copy link
Contributor

@andyo-tyk andyo-tyk commented Aug 16, 2024

DX-1620


Preview Link

https://deploy-preview-5260--tyk-docs.netlify.app/docs/nightly/getting-started/key-concepts/rate-limiting/#combining-multiple-policies-configuring-rate-limits
https://deploy-preview-5260--tyk-docs.netlify.app/docs/nightly/product-stack/tyk-gateway/release-notes/version-5.4/


Description

We missed a bug fix from the 5.4 release notes, as it was fixed as part of another change. Adding this to the release notes and also an explanation of the functionality to the main docs.


PR Type

Documentation, Enhancement


Description

  • Added documentation explaining how Tyk combines multiple policies with rate limits, highlighting the corrected behavior in version 5.4.0.
  • Updated the release notes to include details of a bug fix in the calculation of effective rate limits when multiple policies are applied to a key.
  • Provided a detailed explanation of the previous incorrect behavior and the new corrected logic for rate limit calculations.

Changes walkthrough 📝

Relevant files
Documentation
rate-limiting.md
Document combining multiple rate limit policies and bug fix

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

  • Added explanation for combining multiple policies with rate limits.
  • Clarified the corrected behavior for rate limit calculation.
  • Included a note about the bug fix in rate limit calculation.
  • +14/-0   
    version-5.4.md
    Update release notes with rate limit calculation fix         

    tyk-docs/content/product-stack/tyk-gateway/release-notes/version-5.4.md

  • Updated release notes to include a bug fix for rate limit calculation.
  • Explained the impact of the fix on existing policies.
  • Added a detailed description of the previous and corrected behavior.
  • +16/-1   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link
    Contributor

    github-actions bot commented Aug 16, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Simplify and clarify the explanation of how Tyk calculates effective rate limits when multiple policies are applied

    To enhance clarity and avoid potential confusion, consider rephrasing the
    explanation about how Tyk calculates the effective rate limit when multiple policies
    are applied. The current wording might be too technical for users unfamiliar with
    the concepts of rate and per. Simplifying the language and providing a clearer
    example could improve understanding.

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

    -If more than one policy defining a rate limit is applied to a key then Tyk will apply the highest request rate permitted by any of the policies that defines a rate limit.
    +When multiple policies with rate limits are applied to a key, Tyk selects the highest permissible request rate from these policies.
     
    -If `rate` and `per` are configured in multiple policies applied to the same key then the Gateway will determine the effective rate limit configured for each policy and apply the highest to the key.
    +Specifically, if policies have different `rate` and `per` settings, Tyk calculates the effective rate limit for each and applies the highest. For example, if one policy allows 90 requests every 30 seconds (3 requests per second) and another allows 100 requests every 10 seconds (10 requests per second), Tyk will apply the 10 requests per second limit from the second policy.
     
    -Given, policy A with `rate` set to 90 and `per` set to 30 seconds (3rps) and policy B with `rate` set to 100 and `per` set to 10 seconds (10rps). If both are applied to a key, Tyk will take the rate limit from policy B as it results in a higher effective request rate (10rps).
    -
    Suggestion importance[1-10]: 7

    Why: The suggestion improves clarity and readability by simplifying the explanation of how Tyk calculates effective rate limits, making it more accessible to users unfamiliar with technical terms. However, it does not address a critical issue, so it receives a moderate score.

    7
    Add a direct link to the detailed explanation of the bug fix in the release notes for better navigation and understanding

    It is recommended to provide a direct link to the detailed explanation of the bug
    fix in the release notes. This will help users quickly navigate to the relevant
    section for a comprehensive understanding of the changes and their impact.

    tyk-docs/content/product-stack/tyk-gateway/release-notes/version-5.4.md [32]

    -We have fixed a bug in the way that Tyk calculates the [key-level rate limit]({{< ref "getting-started/key-concepts/rate-limiting#key-level-rate-limiting" >}}) when multiple policies are applied to the same key. This fix alters the logic used to calculate the effective rate limit and so may lead to a different rate limit being applied to keys generated from your existing policies. See the [change log](#fixed) for details of the change.
    +We have fixed a bug in the way that Tyk calculates the [key-level rate limit]({{< ref "getting-started/key-concepts/rate-limiting#key-level-rate-limiting" >}}) when multiple policies are applied to the same key. This fix alters the logic used to calculate the effective rate limit and so may lead to a different rate limit being applied to keys generated from your existing policies. For a detailed explanation, see the [change log](#fixed) and the [detailed section on rate limiting](#combining-multiple-policies-configuring-rate-limits).
     
    Suggestion importance[1-10]: 6

    Why: Adding a direct link to the detailed explanation enhances user navigation and understanding of the changes, which is a helpful improvement for documentation usability. However, it is not a critical enhancement, so it receives a moderate score.

    6

    @andyo-tyk andyo-tyk changed the title Added explanation of bug fix missed in 5.4 RN [DX-1620, TT-12840] Added explanation of bug fix missed in 5.4 RN Aug 16, 2024
    Copy link

    netlify bot commented Aug 16, 2024

    PS. Pls add /docs/nightly to the end of url

    Name Link
    🔨 Latest commit 8233c5f
    🔍 Latest deploy log https://app.netlify.com/sites/tyk-docs/deploys/66c4b31dbd097400087fda48
    😎 Deploy Preview https://deploy-preview-5260--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.

    @andyo-tyk andyo-tyk added the now label Aug 19, 2024
    @andyo-tyk andyo-tyk changed the title [DX-1620, TT-12840] Added explanation of bug fix missed in 5.4 RN [TT-12840, DX-1620] Added explanation of bug fix missed in 5.4 RN Aug 19, 2024
    Copy link
    Contributor

    @dcs3spp dcs3spp left a comment

    Choose a reason for hiding this comment

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

    PR LGTM, not sure if there was a typo in example given in change log, should 100 requests be updated to 90 requests in 2nd paragraph of the fixed change-log item?

    @dcs3spp
    Copy link
    Contributor

    dcs3spp commented Aug 20, 2024

    PR LGTM, @lghiur @OldStubbsy would you be able to peer review and if ok approve and then I will merge and release

    @dcs3spp dcs3spp force-pushed the 5.4-rate-limiter-fix branch from b479618 to 561083d Compare August 20, 2024 10:18
    Copy link

    @OldStubbsy OldStubbsy left a comment

    Choose a reason for hiding this comment

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

    Looks good to me. Thanks!

    @dcs3spp dcs3spp force-pushed the 5.4-rate-limiter-fix branch from 15529a6 to 8233c5f Compare August 20, 2024 15:15
    @dcs3spp
    Copy link
    Contributor

    dcs3spp commented Aug 20, 2024

    Thanks @lghiur @OldStubbsy for peer approval, I'll publish PR for @andyo-tyk now

    @dcs3spp dcs3spp merged commit a63f1ba into master Aug 20, 2024
    9 checks passed
    @dcs3spp dcs3spp deleted the 5.4-rate-limiter-fix branch August 20, 2024 15:20
    @dcs3spp
    Copy link
    Contributor

    dcs3spp commented Aug 20, 2024

    /release to release-5.5

    Copy link

    tykbot bot commented Aug 20, 2024

    Working on it! Note that it can take a few minutes.

    tykbot bot pushed a commit that referenced this pull request Aug 20, 2024
    )
    
    * Added explanation of bug fix missed in 5.4 RN
    
    (cherry picked from commit a63f1ba)
    Copy link

    tykbot bot commented Aug 20, 2024

    @dcs3spp Succesfully merged PR

    @dcs3spp
    Copy link
    Contributor

    dcs3spp commented Aug 20, 2024

    /release to release-5.4

    Copy link

    tykbot bot commented Aug 20, 2024

    Working on it! Note that it can take a few minutes.

    tykbot bot pushed a commit that referenced this pull request Aug 20, 2024
    )
    
    * Added explanation of bug fix missed in 5.4 RN
    
    (cherry picked from commit a63f1ba)
    Copy link

    tykbot bot commented Aug 20, 2024

    @dcs3spp Succesfully merged PR

    buger added a commit that referenced this pull request Aug 20, 2024
    …fix missed in 5.4 RN (#5260)
    
    [TT-12840, DX-1620] Added explanation of bug fix missed in 5.4 RN (#5260)
    
    * Added explanation of bug fix missed in 5.4 RN
    buger added a commit that referenced this pull request Aug 20, 2024
    …fix missed in 5.4 RN (#5260)
    
    [TT-12840, DX-1620] Added explanation of bug fix missed in 5.4 RN (#5260)
    
    * Added explanation of bug fix missed in 5.4 RN
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants