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

Optimize logs #16647

Merged
merged 5 commits into from
Jun 5, 2024
Merged

Conversation

qingxinhome
Copy link
Contributor

@qingxinhome qingxinhome commented Jun 5, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue https://github.com/matrixorigin/MO-Cloud/issues/3404

What this PR does / why we need it:

delete unused logs for upgrader and sqlExecutor


PR Type

Enhancement, Bug fix


Description

  • Removed redundant log statements in upgrade_strategy.go to reduce log clutter.
  • Updated SQL statement in upgrade_tenant_task.go to include update_at timestamp for better tracking.
  • Added logging in upgrade.go to record the time taken for tenant and cluster upgrade entries, aiding performance monitoring.
  • Removed unnecessary logging in sql_executor.go for received SQL execute requests to streamline logging output.

Changes walkthrough 📝

Relevant files
Enhancement
upgrade_strategy.go
Remove redundant logging in upgrade strategy                         

pkg/bootstrap/versions/upgrade_strategy.go

  • Removed redundant log statements after executing SQL commands.
+0/-3     
upgrade_tenant_task.go
Add update timestamp to tenant task state update                 

pkg/bootstrap/versions/upgrade_tenant_task.go

  • Updated SQL statement to include update_at timestamp.
+1/-1     
upgrade.go
Log time taken for tenant and cluster upgrades                     

pkg/bootstrap/versions/v1_2_0/upgrade.go

  • Added logging for time taken to complete tenant and cluster upgrade
    entries.
  • +18/-0   
    sql_executor.go
    Remove logging for received SQL execute request                   

    pkg/sql/compile/sql_executor.go

    • Removed log statement for received SQL execute request.
    +0/-5     

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

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and localized to specific functions within a few files. The modifications mainly involve logging adjustments and minor SQL updates, which are not complex.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Performance Concern: The addition of logging the duration for each upgrade entry in upgrade.go could potentially impact performance, especially if the upgrade process involves a large number of entries. Monitoring the impact on performance in a production-like environment would be advisable.

    🔒 Security concerns

    No

    @mergify mergify bot requested a review from sukki37 June 5, 2024 01:22
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Use parameterized queries to prevent SQL injection vulnerabilities

    Use parameterized queries instead of fmt.Sprintf to prevent SQL injection vulnerabilities
    and improve code safety.

    pkg/bootstrap/versions/upgrade_tenant_task.go [59]

    -sql := fmt.Sprintf("update %s set ready = %d, update_at = current_timestamp() where id = %d",
    +sql := "update " + catalog.MOUpgradeTenantTable + " set ready = ?, update_at = current_timestamp() where id = ?"
     
    Suggestion importance[1-10]: 9

    Why: Using parameterized queries is crucial for preventing SQL injection, which is a serious security concern. This suggestion correctly identifies a significant improvement in security practices.

    9
    Maintainability
    Add logging statements to track the progress of the upgrade process

    Consider adding logging statements back to provide visibility into the progress of the
    upgrade process. This can help in debugging and monitoring the upgrade steps.

    pkg/bootstrap/versions/upgrade_strategy.go [149]

     res.Close()
    +getLogger().Info("execute upgrade pre-sql completed", zap.String("upgrade entry", u.String()))
     
    Suggestion importance[1-10]: 6

    Why: Reintroducing logging statements can significantly aid in monitoring and debugging the upgrade process. However, the exact impact on functionality is moderate, hence the score.

    6
    Reintroduce logging for received SQL execute requests to aid in debugging and monitoring

    Reintroduce the removed logging statement to provide visibility into the received SQL
    execute request, which can be useful for debugging and monitoring.

    pkg/sql/compile/sql_executor.go [257]

    +logutil.Info("Received SQL execute request",
    +    zap.String("sql", sql),
    +    zap.String("txn-id", hex.EncodeToString(exec.opts.Txn().Txn().ID)))
     receiveAt := time.Now()
     
    Suggestion importance[1-10]: 6

    Why: Reintroducing logging can provide useful insights for debugging and monitoring, though it's not as critical as security-related changes.

    6
    Best practice
    Use more descriptive variable names to improve code readability

    Consider using a more descriptive variable name instead of start to improve code
    readability and maintainability.

    pkg/bootstrap/versions/v1_2_0/upgrade.go [70]

    -start := time.Now()
    +startTime := time.Now()
     
    Suggestion importance[1-10]: 4

    Why: While using more descriptive variable names improves readability, it's a relatively minor enhancement in the context of the overall codebase.

    4

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 5, 2024
    @qingxinhome qingxinhome changed the title Long time upgrade 1.2 dev Optimize logs Jun 5, 2024
    @mergify mergify bot merged commit eefded8 into matrixorigin:1.2-dev Jun 5, 2024
    17 of 18 checks passed
    @aylei aylei mentioned this pull request Jun 5, 2024
    @qingxinhome qingxinhome deleted the LongTimeUpgrade-1.2-dev branch June 12, 2024 01:50
    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.

    8 participants