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-12285] Add smoothing, fix duplicate x-go-name #6346

Merged
merged 12 commits into from
Jun 17, 2024

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Jun 13, 2024

User description

Document Smoothing in Policy and SessionState definitions. Fix Policy ID duplicate x-go-name.


PR Type

enhancement, documentation


Description

  • Added a new RateLimitSmoothing schema to define rate limit smoothing settings, including properties like delay, enabled, step, threshold, and trigger.
  • Updated the Policy and SessionState definitions to include the smoothing property referencing the new RateLimitSmoothing schema.
  • Changed the x-go-name for the _id property in the Policy definition from ID to MID to fix a duplicate name issue.
  • Provided detailed descriptions and titles for the new RateLimitSmoothing schema and its properties.

Changes walkthrough 📝

Relevant files
Enhancement
swagger.yml
Add rate limit smoothing and update Policy schema               

swagger.yml

  • Added smoothing property to Policy and SessionState definitions.
  • Introduced RateLimitSmoothing schema with detailed description and
    properties.
  • Changed x-go-name for _id property in Policy definition to MID.
  • Added titles and descriptions for new and existing components.
  • +89/-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

    github-actions bot commented Jun 13, 2024

    API Changes

    --- prev.txt	2024-06-17 13:23:20.652887754 +0000
    +++ current.txt	2024-06-17 13:23:17.416863277 +0000
    @@ -1670,16 +1670,16 @@
     	// Enabled indicates if rate limit smoothing is active.
     	Enabled bool `json:"enabled" bson:"enabled"`
     
    -	// Threshold is the request rate above which smoothing is applied.
    +	// Threshold is the initial rate limit beyond which smoothing will be applied. It is a count of requests during the `per` interval and should be less than the maximum configured `rate`.
     	Threshold int64 `json:"threshold" bson:"threshold"`
     
    -	// Trigger is the step factor determining when smoothing events trigger.
    +	// Trigger is a fraction (typically in the range 0.1-1.0) of the step at which point a smoothing event will be emitted as the request rate approaches the current allowance.
     	Trigger float64 `json:"trigger" bson:"trigger"`
     
    -	// Step is the increment/decrement for adjusting the rate limit.
    +	// Step is the increment by which the current allowance will be increased or decreased each time a smoothing event is emitted.
     	Step int64 `json:"step" bson:"step"`
     
    -	// Delay is the minimum time between rate limit changes (in seconds).
    +	// Delay is a hold-off between smoothing events and controls how frequently the current allowance will step up or down (in seconds).
     	Delay int64 `json:"delay" bson:"delay"`
     }
         RateLimitSmoothing holds the rate smoothing configuration.
    @@ -1701,17 +1701,23 @@
         `threshold` after which to apply smoothing (minimum rate for window) -
         `trigger` configures at which fraction of a step a smoothing event is
         emitted - `step` is the value by which the rate allowance will get adjusted
    -    - `delay` is the amount of seconds between smoothing updates
    +    - `delay` is a hold-off in seconds providing a minimum period between rate
    +    allowance adjustments
     
    -    This is used to compute a request allowance. The request allowance will
    -    be smoothed between `threshold`, and the defined rate limits (maximum).
    -    The request allowance will be updated internally every `delay` seconds.
    -
    -    The `step * trigger` value is substracted from the request allowance, and
    -    if your request rate goes above that, then a RateLimitSmoothingUp event is
    -    emitted and the allowance is increased by `step`. A RateLimitSmoothingDown
    -    event is emitted when the request rate drops one step below that, and the
    -    allowance then decreases by step.
    +    To determine if the request rate is growing and needs to be smoothed,
    +    the `step * trigger` value is subtracted from the request allowance and,
    +    if the request rate goes above that, then a RateLimitSmoothingUp event is
    +    emitted and the rate allowance is increased by `step`.
    +
    +    Once the request allowance has been increased above the `threshold`,
    +    Tyk will start to check for decreasing request rate. When the request
    +    rate drops `step * (1 + trigger)` below the request allowance,
    +    a `RateLimitSmoothingDown` event is emitted and the rate allowance is
    +    decreased by `step`.
    +
    +    After the request allowance has been adjusted (up or down), the request
    +    rate will be checked again over the next `delay` seconds and, if required,
    +    further adjustment made to the rate allowance after the hold-off.
     
         For any allowance, events are emitted based on the following calculations:
     
    @@ -1721,17 +1727,28 @@
             a RateLimitSmoothingDown event is emitted and allowance decreases by
             `step`.
     
    -    Example: Allowance: 600, Current rate: 500, Step: 100, Trigger: 0.5
    +    Example: Threshold: 400, Request allowance: 600, Current rate: 500, Step:
    +    100, Trigger: 0.5.
     
    -      - To trigger a RateLimitSmoothingUp event, the request rate must exceed:
    -        Allowance - (Step * Trigger) Calculation: 600 - (100 * 0.5) = 550
    -        Exceeding a request rate of 550 will increase the allowance to 700
    -        (Allowance + Step).
    -
    -      - To trigger a RateLimitSmoothingDown event, the request rate must fall
    -        below: Allowance - (Step + (Step * Trigger)) Calculation: 600 - (100
    -        + (100 * 0.5)) = 450 As the request rate falls below 450, that will
    -        decrease the allowance to 500 (Allowance - Step).
    +    To trigger a RateLimitSmoothingUp event, the request rate must exceed:
    +
    +      - Calculation: Allowance - (Step * Trigger).
    +      - Example: 600 - (100 * 0.5) = `550`.
    +
    +    Exceeding a request rate of `550` will increase the allowance to 700
    +    (Allowance + Step).
    +
    +    To trigger a RateLimitSmoothingDown event, the request rate must fall below:
    +
    +      - Calculation: Allowance - (Step + (Step * Trigger)).
    +      - Example: 600 - (100 + (100 * 0.5)) = 450.
    +
    +    As the request rate falls below 450, that will decrease the allowance to 500
    +    (Allowance - Step).
    +
    +    The request allowance will be smoothed between `threshold`, and the defined
    +    `rate` limit (maximum). The request allowance will be updated internally
    +    every `delay` seconds.
     
     func (r *RateLimitSmoothing) Err() error
         Err checks the rate limit smoothing configuration for validity and returns

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    3

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    Ensure that the smoothing property is correctly implemented in both Policy and SessionState schemas. The reference to RateLimitSmoothing must be correctly resolved and function as intended.

    Data Validation:
    Verify that the data types and formats (e.g., integer, boolean, double) for the properties in the RateLimitSmoothing schema are appropriate and consistent with the system's requirements.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Verify and potentially revert the x-go-name change for _id to avoid breaking changes

    It seems that the x-go-name for _id was changed from ID to MID. This might lead to
    inconsistencies or conflicts if the ID is still being used elsewhere in the codebase or if
    external systems depend on the original naming. Consider verifying the impact of this
    change on other parts of the system or maintaining the original x-go-name if necessary.

    swagger.yml [2974]

    -x-go-name: MID
    +x-go-name: ID
     
    Suggestion importance[1-10]: 8

    Why: Changing the x-go-name from ID to MID could lead to inconsistencies or conflicts if the original name ID is still used elsewhere. This suggestion is crucial to avoid potential breaking changes in the system.

    8
    Best practice
    Ensure the RateLimitSmoothing schema is generic enough for all its references

    The smoothing object is added under multiple sections but it references the same schema
    #/components/schemas/RateLimitSmoothing. To maintain consistency and avoid duplication,
    ensure that the schema definition at this reference is generic enough to be applicable in
    all contexts where smoothing is used, or consider defining context-specific schemas if the
    usage varies significantly.

    swagger.yml [3043-3047]

     smoothing:
       type: object
       $ref: '#/components/schemas/RateLimitSmoothing'
    -  description: Smoothing contains rate limit smoothing settings.
    +  description: Smoothing contains rate limit smoothing settings applicable to all usage contexts.
       x-go-name: Smoothing
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to ensure the RateLimitSmoothing schema is generic enough for all contexts is valid and improves maintainability. However, it is not critical as long as the current schema meets all usage requirements.

    7
    Maintainability
    Simplify the description of RateLimitSmoothing to be more concise and move detailed explanations to external documentation

    The description field for RateLimitSmoothing is very detailed and includes implementation
    specifics which might be better suited in documentation rather than in the API schema.
    Consider simplifying the description in the schema to focus on what the configuration does
    at a high level, and move the detailed behavior explanation to external documentation.

    swagger.yml [3102-3106]

     description: |-
    -  Rate Limit Smoothing is a mechanism to dynamically adjust the request rate
    -  limits based on the current traffic patterns. It helps in managing request
    -  spikes by gradually increasing or decreasing the rate limit instead of making
    -  abrupt changes or blocking requests excessively.
    +  Rate Limit Smoothing dynamically adjusts request rates based on traffic patterns to manage request spikes.
     
    Suggestion importance[1-10]: 6

    Why: Simplifying the description field improves readability and maintainability of the schema. However, this is a minor improvement and not critical to the functionality.

    6
    Possible bug
    Correct the placement of the title attribute in the Policy object definition

    The title attribute for the Policy object is added at a lower hierarchical level which
    might be a mistake. It should be placed at the beginning of the object definition to
    correctly represent the title of the object.

    swagger.yml [3053]

     title: Policy represents a user policy
    +last_updated:
    +  type: string
    +  x-go-name: LastUpdates
     
    Suggestion importance[1-10]: 5

    Why: The suggestion to correct the placement of the title attribute is valid for maintaining proper schema structure. However, the current placement does not cause functional issues, making this a minor improvement.

    5

    Copy link
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    1 similar comment
    Copy link
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    Copy link
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    Copy link
    Contributor

    @andyo-tyk andyo-tyk left a comment

    Choose a reason for hiding this comment

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

    Suggested changes to improve consistency

    swagger.yml Outdated Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    swagger.yml Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    Copy link
    Contributor

    @andyo-tyk andyo-tyk left a comment

    Choose a reason for hiding this comment

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

    Thanks for the changes. 🙏

    @titpetric titpetric force-pushed the update/tt-12285/swagger-document-smoothing branch from cfc28cf to 653d40c Compare June 17, 2024 13:22
    @titpetric titpetric enabled auto-merge (squash) June 17, 2024 13:23
    Copy link

    sonarcloud bot commented Jun 17, 2024

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    @titpetric titpetric merged commit e10e7ef into master Jun 17, 2024
    36 checks passed
    @titpetric titpetric deleted the update/tt-12285/swagger-document-smoothing branch June 17, 2024 13:43
    @titpetric
    Copy link
    Contributor Author

    /release to release-5.4

    Copy link

    tykbot bot commented Jun 17, 2024

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

    tykbot bot pushed a commit that referenced this pull request Jun 17, 2024
    ### **User description**
    Document Smoothing in Policy and SessionState definitions. Fix Policy ID
    duplicate x-go-name.
    
    
    ___
    
    ### **PR Type**
    enhancement, documentation
    
    
    ___
    
    ### **Description**
    - Added a new `RateLimitSmoothing` schema to define rate limit smoothing
    settings, including properties like `delay`, `enabled`, `step`,
    `threshold`, and `trigger`.
    - Updated the `Policy` and `SessionState` definitions to include the
    `smoothing` property referencing the new `RateLimitSmoothing` schema.
    - Changed the `x-go-name` for the `_id` property in the `Policy`
    definition from `ID` to `MID` to fix a duplicate name issue.
    - Provided detailed descriptions and titles for the new
    `RateLimitSmoothing` schema and its properties.
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Enhancement
    </strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>swagger.yml</strong><dd><code>Add rate limit smoothing
    and update Policy schema</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; </dd></summary>
    <hr>
    
    swagger.yml
    <li>Added <code>smoothing</code> property to <code>Policy</code> and
    <code>SessionState</code> definitions.<br> <li> Introduced
    <code>RateLimitSmoothing</code> schema with detailed description and
    <br>properties.<br> <li> Changed <code>x-go-name</code> for
    <code>_id</code> property in <code>Policy</code> definition to
    <code>MID</code>.<br> <li> Added titles and descriptions for new and
    existing components.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6346/files#diff-8f3c4cb253eee09ae2401daa7279a8bbfbfd4168bb579c3ac0ee5c672d63bb2c">+89/-1</a>&nbsp;
    &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > 💡 **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    
    ---------
    
    Co-authored-by: Tit Petric <tit@tyk.io>
    Co-authored-by: andyo-tyk <99968932+andyo-tyk@users.noreply.github.com>
    
    (cherry picked from commit e10e7ef)
    Copy link

    tykbot bot commented Jun 17, 2024

    @titpetric Succesfully merged PR

    buger added a commit that referenced this pull request Jun 17, 2024
    …name (#6346)
    
    [TT-12285] Add smoothing, fix duplicate x-go-name (#6346)
    
    ### **User description**
    Document Smoothing in Policy and SessionState definitions. Fix Policy ID
    duplicate x-go-name.
    
    
    ___
    
    ### **PR Type**
    enhancement, documentation
    
    
    ___
    
    ### **Description**
    - Added a new `RateLimitSmoothing` schema to define rate limit smoothing
    settings, including properties like `delay`, `enabled`, `step`,
    `threshold`, and `trigger`.
    - Updated the `Policy` and `SessionState` definitions to include the
    `smoothing` property referencing the new `RateLimitSmoothing` schema.
    - Changed the `x-go-name` for the `_id` property in the `Policy`
    definition from `ID` to `MID` to fix a duplicate name issue.
    - Provided detailed descriptions and titles for the new
    `RateLimitSmoothing` schema and its properties.
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Enhancement
    </strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>swagger.yml</strong><dd><code>Add rate limit smoothing
    and update Policy schema</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; </dd></summary>
    <hr>
    
    swagger.yml
    <li>Added <code>smoothing</code> property to <code>Policy</code> and
    <code>SessionState</code> definitions.<br> <li> Introduced
    <code>RateLimitSmoothing</code> schema with detailed description and
    <br>properties.<br> <li> Changed <code>x-go-name</code> for
    <code>_id</code> property in <code>Policy</code> definition to
    <code>MID</code>.<br> <li> Added titles and descriptions for new and
    existing components.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6346/files#diff-8f3c4cb253eee09ae2401daa7279a8bbfbfd4168bb579c3ac0ee5c672d63bb2c">+89/-1</a>&nbsp;
    &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > 💡 **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    
    ---------
    
    Co-authored-by: Tit Petric <tit@tyk.io>
    Co-authored-by: andyo-tyk <99968932+andyo-tyk@users.noreply.github.com>
    @titpetric
    Copy link
    Contributor Author

    /release to release-5.4.0

    Copy link

    tykbot bot commented Jun 17, 2024

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

    tykbot bot pushed a commit that referenced this pull request Jun 17, 2024
    ### **User description**
    Document Smoothing in Policy and SessionState definitions. Fix Policy ID
    duplicate x-go-name.
    
    
    ___
    
    ### **PR Type**
    enhancement, documentation
    
    
    ___
    
    ### **Description**
    - Added a new `RateLimitSmoothing` schema to define rate limit smoothing
    settings, including properties like `delay`, `enabled`, `step`,
    `threshold`, and `trigger`.
    - Updated the `Policy` and `SessionState` definitions to include the
    `smoothing` property referencing the new `RateLimitSmoothing` schema.
    - Changed the `x-go-name` for the `_id` property in the `Policy`
    definition from `ID` to `MID` to fix a duplicate name issue.
    - Provided detailed descriptions and titles for the new
    `RateLimitSmoothing` schema and its properties.
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Enhancement
    </strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>swagger.yml</strong><dd><code>Add rate limit smoothing
    and update Policy schema</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; </dd></summary>
    <hr>
    
    swagger.yml
    <li>Added <code>smoothing</code> property to <code>Policy</code> and
    <code>SessionState</code> definitions.<br> <li> Introduced
    <code>RateLimitSmoothing</code> schema with detailed description and
    <br>properties.<br> <li> Changed <code>x-go-name</code> for
    <code>_id</code> property in <code>Policy</code> definition to
    <code>MID</code>.<br> <li> Added titles and descriptions for new and
    existing components.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6346/files#diff-8f3c4cb253eee09ae2401daa7279a8bbfbfd4168bb579c3ac0ee5c672d63bb2c">+89/-1</a>&nbsp;
    &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > 💡 **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    
    ---------
    
    Co-authored-by: Tit Petric <tit@tyk.io>
    Co-authored-by: andyo-tyk <99968932+andyo-tyk@users.noreply.github.com>
    
    (cherry picked from commit e10e7ef)
    Copy link

    tykbot bot commented Jun 17, 2024

    @titpetric Succesfully merged PR

    buger added a commit that referenced this pull request Jun 17, 2024
    …o-name (#6346)
    
    [TT-12285] Add smoothing, fix duplicate x-go-name (#6346)
    
    ### **User description**
    Document Smoothing in Policy and SessionState definitions. Fix Policy ID
    duplicate x-go-name.
    
    
    ___
    
    ### **PR Type**
    enhancement, documentation
    
    
    ___
    
    ### **Description**
    - Added a new `RateLimitSmoothing` schema to define rate limit smoothing
    settings, including properties like `delay`, `enabled`, `step`,
    `threshold`, and `trigger`.
    - Updated the `Policy` and `SessionState` definitions to include the
    `smoothing` property referencing the new `RateLimitSmoothing` schema.
    - Changed the `x-go-name` for the `_id` property in the `Policy`
    definition from `ID` to `MID` to fix a duplicate name issue.
    - Provided detailed descriptions and titles for the new
    `RateLimitSmoothing` schema and its properties.
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Enhancement
    </strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>swagger.yml</strong><dd><code>Add rate limit smoothing
    and update Policy schema</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; </dd></summary>
    <hr>
    
    swagger.yml
    <li>Added <code>smoothing</code> property to <code>Policy</code> and
    <code>SessionState</code> definitions.<br> <li> Introduced
    <code>RateLimitSmoothing</code> schema with detailed description and
    <br>properties.<br> <li> Changed <code>x-go-name</code> for
    <code>_id</code> property in <code>Policy</code> definition to
    <code>MID</code>.<br> <li> Added titles and descriptions for new and
    existing components.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6346/files#diff-8f3c4cb253eee09ae2401daa7279a8bbfbfd4168bb579c3ac0ee5c672d63bb2c">+89/-1</a>&nbsp;
    &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > 💡 **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    
    ---------
    
    Co-authored-by: Tit Petric <tit@tyk.io>
    Co-authored-by: andyo-tyk <99968932+andyo-tyk@users.noreply.github.com>
    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