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

Merging to release-5.2: TT-11288 Revert logger mutex (#6103) #6104

Conversation

buger
Copy link
Member

@buger buger commented Mar 6, 2024

User description

TT-11288 Revert logger mutex (#6103)

User description

Description

Remove the mutex for the logger

Related Issue

TT-11288

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing
    functionality to change)
  • Refactoring or add test (improvements in base code or adds test
    coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning
    why it's required
  • I would like a code coverage CI quality gate exception and have
    explained why

Type

bug_fix


Description

  • Removed the mutex lock from the logger in the BaseMiddleware struct
    to address potential performance issues and simplify the code.
  • Updated methods (Base(), Logger(), SetName(), and
    SetRequestLogger()) in BaseMiddleware to operate without mutex
    synchronization.

Changes walkthrough

Relevant files
Bug fix
middleware.go
Remove Logger Mutex in BaseMiddleware                                       

gateway/middleware.go

  • Removed mutex (sync.Mutex) for logger in BaseMiddleware struct.
  • Simplified Base(), Logger(), SetName(), and SetRequestLogger() methods
    by removing mutex locks and unlocks.
  • Ensured logger instance is directly manipulated without mutex
    synchronization.
  • +8/-32   

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


    Type

    bug_fix


    Description

    • Removed the mutex lock from the logger in the BaseMiddleware struct to address potential performance issues and simplify the code.
    • Updated methods (Base(), Logger(), SetName(), and SetRequestLogger()) in BaseMiddleware to operate without mutex synchronization, enhancing performance and code simplicity.

    Changes walkthrough

    Relevant files
    Bug_fix
    middleware.go
    Remove Mutex Synchronization from Logger in BaseMiddleware

    gateway/middleware.go

  • Removed mutex (sync.Mutex) for logger in BaseMiddleware struct.
  • Updated Base(), Logger(), SetName(), and SetRequestLogger() methods to
    operate without mutex synchronization.
  • +8/-32   

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

    ## **User description**
    <!-- Provide a general summary of your changes in the Title above -->
    
    ## Description
    
    Remove the mutex for the  logger 
    
    ## Related Issue
    
    TT-11288
    
    ## Motivation and Context
    
    <!-- Why is this change required? What problem does it solve? -->
    
    ## How This Has Been Tested
    
    <!-- Please describe in detail how you tested your changes -->
    <!-- Include details of your testing environment, and the tests -->
    <!-- you ran to see how your change affects other areas of the code,
    etc. -->
    <!-- This information is helpful for reviewers and QA. -->
    
    ## Screenshots (if appropriate)
    
    ## Types of changes
    
    <!-- What types of changes does your code introduce? Put an `x` in all
    the boxes that apply: -->
    
    - [ ] Bug fix (non-breaking change which fixes an issue)
    - [ ] New feature (non-breaking change which adds functionality)
    - [ ] Breaking change (fix or feature that would cause existing
    functionality to change)
    - [ ] Refactoring or add test (improvements in base code or adds test
    coverage to functionality)
    
    ## Checklist
    
    <!-- Go over all the following points, and put an `x` in all the boxes
    that apply -->
    <!-- If there are no documentation updates required, mark the item as
    checked. -->
    <!-- Raise up any additional concerns not covered by the checklist. -->
    
    - [ ] I ensured that the documentation is up to date
    - [ ] I explained why this PR updates go.mod in detail with reasoning
    why it's required
    - [ ] I would like a code coverage CI quality gate exception and have
    explained why
    
    
    ___
    
    ## **Type**
    bug_fix
    
    
    ___
    
    ## **Description**
    - Removed the mutex lock from the logger in the `BaseMiddleware` struct
    to address potential performance issues and simplify the code.
    - Updated methods (`Base()`, `Logger()`, `SetName()`, and
    `SetRequestLogger()`) in `BaseMiddleware` to operate without mutex
    synchronization.
    
    
    ___
    
    
    
    ## **Changes walkthrough**
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Bug
    fix</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>middleware.go</strong><dd><code>Remove Logger Mutex in
    BaseMiddleware</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    gateway/middleware.go
    <li>Removed mutex (<code>sync.Mutex</code>) for logger in
    <code>BaseMiddleware</code> struct.<br> <li> Simplified
    <code>Base()</code>, <code>Logger()</code>, <code>SetName()</code>, and
    <code>SetRequestLogger()</code> methods <br>by removing mutex locks and
    unlocks.<br> <li> Ensured logger instance is directly manipulated
    without mutex <br>synchronization.
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6103/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1b">+8/-32</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
    
    (cherry picked from commit bb3ad6a)
    @buger buger enabled auto-merge (squash) March 6, 2024 13:45
    Copy link
    Contributor

    github-actions bot commented Mar 6, 2024

    PR Description updated to latest commit (ab65e98)

    Copy link
    Contributor

    github-actions bot commented Mar 6, 2024

    API Changes

    --- prev.txt	2024-03-06 13:46:02.087486844 +0000
    +++ current.txt	2024-03-06 13:45:58.647441224 +0000
    @@ -6771,8 +6771,8 @@
     type BaseMiddleware struct {
     	Spec  *APISpec
     	Proxy ReturningHttpHandler
    -	Gw    *Gateway `json:"-"`
     
    +	Gw *Gateway `json:"-"`
     	// Has unexported fields.
     }
         BaseMiddleware wraps up the ApiSpec and Proxy objects to be included in a
    @@ -6782,7 +6782,7 @@
         ApplyPolicies will check if any policies are loaded. If any are, it will
         overwrite the session state to use the policy values.
     
    -func (t *BaseMiddleware) Base() *BaseMiddleware
    +func (t BaseMiddleware) Base() *BaseMiddleware
     
     func (t BaseMiddleware) CheckSessionAndIdentityForValidKey(originalKey string, r *http.Request) (user.SessionState, bool)
         CheckSessionAndIdentityForValidKey will check first the Session store for a

    Copy link
    Contributor

    github-actions bot commented Mar 6, 2024

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and localized to a specific functionality (logger handling) within the middleware. The removal of mutex suggests a need to review thread safety and potential race conditions.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Thread Safety: Removing the mutex lock from the logger might introduce race conditions if the logger is accessed concurrently from multiple goroutines.

    Logger State: The removal of mutex and changes in logger handling could potentially affect the logger's state management across different middleware instances.

    🔒 Security concerns

    No

    Code feedback:
    relevant filegateway/middleware.go
    suggestion      

    Consider implementing a thread-safe singleton pattern for the logger instance if concurrent access is expected. This ensures that the logger remains safe to use across multiple goroutines without reintroducing the mutex. [important]

    relevant linelogger *logrus.Entry

    relevant filegateway/middleware.go
    suggestion      

    Verify that removing the mutex does not introduce race conditions by conducting thorough multi-threaded testing or by using tools like the Go race detector. [important]

    relevant line- loggerMu sync.Mutex

    relevant filegateway/middleware.go
    suggestion      

    Since Base() method now returns a pointer to the original BaseMiddleware instance, ensure that any modifications to the returned instance do not unintentionally affect the original. Consider returning a deep copy if isolation is needed. [medium]

    relevant linereturn &t

    relevant filegateway/middleware.go
    suggestion      

    Ensure that the removal of mutex and changes in the logger handling do not affect the logging context or metadata, especially in concurrent request scenarios. [medium]

    relevant linet.logger = t.Logger().WithField("mw", name)


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
    When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    

    With a configuration file, use the following template:

    [pr_reviewer]
    some_config1=...
    some_config2=...
    
    Utilizing extra instructions

    The review tool can be configured with extra instructions, which can be used to guide the model to a feedback tailored to the needs of your project.

    Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify the relevant sub-tool, and the relevant aspects of the PR that you want to emphasize.

    Examples for extra instructions:

    [pr_reviewer] # /review #
    extra_instructions="""
    In the 'possible issues' section, emphasize the following:
    - Does the code logic cover relevant edge cases?
    - Is the code logic clear and easy to understand?
    - Is the code logic efficient?
    ...
    """
    

    Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

    How to enable\disable automation
    • When you first install PR-Agent app, the default mode for the review tool is:
    pr_commands = ["/review", ...]
    

    meaning the review tool will run automatically on every PR, with the default configuration.
    Edit this field to enable/disable the tool, or to change the used configurations

    Auto-labels

    The review tool can auto-generate two specific types of labels for a PR:

    • a possible security issue label, that detects possible security issues (enable_review_labels_security flag)
    • a Review effort [1-5]: x label, where x is the estimated effort to review the PR (enable_review_labels_effort flag)
    Extra sub-tools

    The review tool provides a collection of possible feedbacks about a PR.
    It is recommended to review the possible options, and choose the ones relevant for your use case.
    Some of the feature that are disabled by default are quite useful, and should be considered for enabling. For example:
    require_score_review, require_soc2_ticket, and more.

    Auto-approve PRs

    By invoking:

    /review auto_approve
    

    The tool will automatically approve the PR, and add a comment with the approval.

    To ensure safety, the auto-approval feature is disabled by default. To enable auto-approval, you need to actively set in a pre-defined configuration file the following:

    [pr_reviewer]
    enable_auto_approval = true
    

    (this specific flag cannot be set with a command line argument, only in the configuration file, committed to the repository)

    You can also enable auto-approval only if the PR meets certain requirements, such as that the estimated_review_effort is equal or below a certain threshold, by adjusting the flag:

    [pr_reviewer]
    maximal_review_effort = 5
    
    More PR-Agent commands

    To invoke the PR-Agent, add a comment using one of the following commands:

    • /review: Request a review of your Pull Request.
    • /describe: Update the PR title and description based on the contents of the PR.
    • /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
    • /ask <QUESTION>: Ask a question about the PR.
    • /update_changelog: Update the changelog based on the PR's contents.
    • /add_docs 💎: Generate docstring for new components introduced in the PR.
    • /generate_labels 💎: Generate labels for the PR based on the PR's contents.
    • /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.

    See the tools guide for more details.
    To list the possible configuration parameters, add a /config comment.

    See the review usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    github-actions bot commented Mar 6, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Possible issue
    Reintroduce locking mechanism for logger to prevent race conditions.

    The removal of loggerMu mutex and the related locking mechanism might introduce race
    conditions when logger is accessed or modified concurrently. Consider reintroducing a
    locking mechanism or ensuring that logger operations are thread-safe by design.

    gateway/middleware.go [245-248]

     func (t *BaseMiddleware) Logger() (logger *logrus.Entry) {
    +    t.loggerMu.Lock()
    +    defer t.loggerMu.Unlock()
         if t.logger == nil {
             t.logger = logrus.NewEntry(log)
         }
     
    Ensure thread-safe modifications to the logger field in SetName and SetRequestLogger.

    The SetName and SetRequestLogger methods modify the logger field without any form of
    synchronization, which could lead to race conditions. Consider ensuring these
    modifications are thread-safe, possibly by reintroducing the mutex lock or by other means
    of synchronization.

    gateway/middleware.go [253-259]

     func (t *BaseMiddleware) SetName(name string) {
    +    t.loggerMu.Lock()
    +    defer t.loggerMu.Unlock()
         t.logger = t.Logger().WithField("mw", name)
     }
     func (t *BaseMiddleware) SetRequestLogger(r *http.Request) {
    +    t.loggerMu.Lock()
    +    defer t.loggerMu.Unlock()
         t.logger = t.Gw.getLogEntryForRequest(t.Logger(), r, ctxGetAuthToken(r), nil)
     }
     
    Ensure logger creation and modification are thread-safe to prevent concurrent map writes.

    The removal of synchronization around logger creation and modification might lead to
    concurrent map writes if the logger's internal state is modified concurrently. Consider
    using a concurrent-safe logger or reintroducing synchronization mechanisms.

    gateway/middleware.go [245-248]

    +// Example using a mutex to ensure thread-safety
     func (t *BaseMiddleware) Logger() (logger *logrus.Entry) {
    +    t.loggerMu.Lock()
    +    defer t.loggerMu.Unlock()
         if t.logger == nil {
             t.logger = logrus.NewEntry(log)
         }
     
    Enhancement
    Modify Base() to return a pointer to the original receiver to avoid unexpected behavior.

    The Base() method in BaseMiddleware struct returns a pointer to a copy of the receiver.
    This could lead to unexpected behavior since modifications to the returned BaseMiddleware
    won't affect the original. Consider returning a pointer to the original receiver instead.

    gateway/middleware.go [241-243]

    -func (t BaseMiddleware) Base() *BaseMiddleware {
    -    return &t
    +func (t *BaseMiddleware) Base() *BaseMiddleware {
    +    return t
     }
     
    Maintainability
    Clarify the purpose of the empty Init method or implement the necessary initialization logic.

    The Init method of BaseMiddleware is empty, which might be intentional but could also be
    an oversight. If it's supposed to initialize something, consider implementing the
    necessary logic. If it's intentionally left empty, a comment explaining why it's empty
    could help maintain clarity.

    gateway/middleware.go [261]

    +// Init is intentionally left empty. Future initializations for BaseMiddleware can be added here.
     func (t BaseMiddleware) Init() {}
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.
    When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    

    With a configuration file, use the following template:

    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    
    Enabling\disabling automation

    When you first install the app, the default mode for the improve tool is:

    pr_commands = ["/improve --pr_code_suggestions.summarize=true", ...]
    

    meaning the improve tool will run automatically on every PR, with summarization enabled. Delete this line to disable the tool from running automatically.

    Utilizing extra instructions

    Extra instructions are very important for the improve tool, since they enable to guide the model to suggestions that are more relevant to the specific needs of the project.

    Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify relevant aspects that you want the model to focus on.

    Examples for extra instructions:

    [pr_code_suggestions] # /improve #
    extra_instructions="""
    Emphasize the following aspects:
    - Does the code logic cover relevant edge cases?
    - Is the code logic clear and easy to understand?
    - Is the code logic efficient?
    ...
    """
    

    Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

    A note on code suggestions quality
    • While the current AI for code is getting better and better (GPT-4), it's not flawless. Not all the suggestions will be perfect, and a user should not accept all of them automatically.
    • Suggestions are not meant to be simplistic. Instead, they aim to give deep feedback and raise questions, ideas and thoughts to the user, who can then use his judgment, experience, and understanding of the code base.
    • Recommended to use the 'extra_instructions' field to guide the model to suggestions that are more relevant to the specific needs of the project, or use the custom suggestions 💎 tool
    • With large PRs, best quality will be obtained by using 'improve --extended' mode.
    More PR-Agent commands

    To invoke the PR-Agent, add a comment using one of the following commands:

    • /review: Request a review of your Pull Request.
    • /describe: Update the PR title and description based on the contents of the PR.
    • /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
    • /ask <QUESTION>: Ask a question about the PR.
    • /update_changelog: Update the changelog based on the PR's contents.
    • /add_docs 💎: Generate docstring for new components introduced in the PR.
    • /generate_labels 💎: Generate labels for the PR based on the PR's contents.
    • /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.

    See the tools guide for more details.
    To list the possible configuration parameters, add a /config comment.

    See the improve usage page for a more comprehensive guide on using this tool.

    Copy link

    sonarqubecloud bot commented Mar 6, 2024

    Quality Gate Failed Quality Gate failed

    Failed conditions
    64.5% Coverage on New Code (required ≥ 80%)

    See analysis details on SonarCloud

    @buger buger merged commit bbdd262 into release-5.2 Mar 6, 2024
    12 of 17 checks passed
    @buger buger deleted the merge/release-5.2/bb3ad6af0fe80757ff5e28c700f260868cad7a57 branch March 6, 2024 13:58
    @buger
    Copy link
    Member Author

    buger commented Mar 6, 2024

    API tests result: success
    Branch used: refs/pull/6104/merge
    Commit:
    Triggered by: pull_request (@buger)
    Execution page

    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.

    2 participants