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

append log for upgrade and sqlExecutoer1.2 dev #16576

Conversation

qingxinhome
Copy link
Contributor

@qingxinhome qingxinhome commented Jun 1, 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:

append log for upgrader and sqlExecutor


PR Type

Bug fix, Enhancement


Description

  • Added detailed logging for pre-sql, upgrade sql, and post-sql execution steps in pkg/bootstrap/versions/upgrade_strategy.go.
  • Introduced timing and logging for each tenant and cluster upgrade entry in pkg/bootstrap/versions/v1_2_1/upgrade.go.
  • Added logging for received SQL execute requests in pkg/sql/compile/sql_executor.go.

Changes walkthrough 📝

Relevant files
Enhancement
upgrade_strategy.go
Add detailed logging for upgrade SQL execution steps         

pkg/bootstrap/versions/upgrade_strategy.go

  • Added error logging for pre-sql, upgrade sql, and post-sql execution.
  • Added info logging for the completion of pre-sql, upgrade sql, and
    post-sql execution.
  • +5/-0     
    upgrade.go
    Add timing and logging for tenant and cluster upgrade entries

    pkg/bootstrap/versions/v1_2_1/upgrade.go

  • Added timing for each tenant and cluster upgrade entry.
  • Added info logging for the completion of each upgrade entry with time
    cost.
  • +18/-0   
    sql_executor.go
    Log received SQL execute requests                                               

    pkg/sql/compile/sql_executor.go

    • Added logging for received SQL execute requests.
    +4/-0     

    💡 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]

    3, because the PR involves multiple files and adds logging at various stages of SQL execution, which requires understanding the flow of SQL transactions and the impact of logging on performance and readability. Reviewing the correct placement and content of logs also requires domain-specific knowledge.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Performance Impact: The addition of logging, especially error logging that includes stack traces or large context objects, could impact the performance of the system. It's important to ensure that logging does not significantly slow down the transaction execution or consume excessive resources.

    Error Handling: The use of logging after an error without rethrowing or handling the error might lead to unclear program states or missed error handling in the future if the code is modified.

    🔒 Security concerns

    No

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 1, 2024
    @mergify mergify bot requested a review from sukki37 June 1, 2024 11:48
    @mergify mergify bot added the kind/bug Something isn't working label Jun 1, 2024
    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
    Sanitize the SQL string before logging to enhance security and prevent SQL injection in logs

    To enhance security, consider sanitizing the SQL string before logging it to avoid
    potential SQL injection attacks in the logs.

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

    -zap.String("sql", sql)
    +zap.String("sql", sanitizeSQL(sql))
     
    +func sanitizeSQL(sql string) string {
    +    // Implement SQL sanitization logic here
    +    return sql
    +}
    +
    Suggestion importance[1-10]: 9

    Why: Addressing potential security vulnerabilities related to SQL injection in logs is crucial, making this a highly valuable suggestion.

    9
    Possible bug
    Add a nil check before closing the result set to prevent potential nil pointer dereference

    To avoid potential nil pointer dereference, ensure that res is not nil before calling
    res.Close().

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

    -res.Close()
    +if res != nil {
    +    res.Close()
    +}
     
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies a potential nil pointer dereference issue which is crucial for preventing runtime panics.

    8
    Maintainability
    Extract logging logic into a separate function to improve readability and maintainability

    To improve readability and maintainability, consider extracting the logging logic into a
    separate function.

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

    -getLogger().Info("execute upgrade pre-sql completed", zap.String("upgrade entry", u.String()))
    +logUpgradeCompletion("pre-sql", u)
     
    +func logUpgradeCompletion(stage string, u *UpgradeEntry) {
    +    getLogger().Info(fmt.Sprintf("execute upgrade %s completed", stage), zap.String("upgrade entry", u.String()))
    +}
    +
    Suggestion importance[1-10]: 6

    Why: The suggestion to refactor logging into a separate function is valid for maintainability, but it's not critical for functionality or performance.

    6
    Best practice
    Use a consistent key for logging time duration to ensure uniform log entries

    To ensure consistent logging format, consider using a consistent key for the time duration
    across different log entries.

    pkg/bootstrap/versions/v1_2_1/upgrade.go [74]

    -zap.Int64("time cost(ms)", duration.Milliseconds())
    +zap.Int64("duration_ms", duration.Milliseconds())
     
    Suggestion importance[1-10]: 5

    Why: The suggestion improves logging consistency which is good for maintainability and readability, though it's a minor enhancement.

    5

    @qingxinhome qingxinhome changed the title Cluster upgrade view log info hang 1.2 dev append log for upgrade and sqlExecutoer1.2 dev Jun 1, 2024
    @mergify mergify bot merged commit ddbc4fc into matrixorigin:1.2-dev Jun 3, 2024
    17 of 18 checks passed
    @aylei aylei mentioned this pull request Jun 5, 2024
    @qingxinhome qingxinhome deleted the ClusterUpgradeViewLog_infoHang-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
    Labels
    Bug fix Enhancement kind/bug Something isn't working Review effort [1-5]: 3 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    7 participants