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

add log #16831

Closed
wants to merge 5 commits into from
Closed

add log #16831

wants to merge 5 commits into from

Conversation

triump2020
Copy link
Contributor

@triump2020 triump2020 commented Jun 12, 2024

User description

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #16273

What this PR does / why we need it:

1.Add log


PR Type

Bug fix, Enhancement


Description

  • Replaced SetStaticTxnId and GetStaticTxnId methods with SetStaticTxnInfo and GetStaticTxnInfo to store and retrieve transaction debug information.
  • Added logging for successful transaction commits.
  • Updated transaction commit handling to store debug information.
  • Updated logging to use transaction debug information instead of transaction ID.

Changes walkthrough 📝

Relevant files
Enhancement
mysql_cmd_executor.go
Update transaction information storage method                       

pkg/frontend/mysql_cmd_executor.go

  • Changed SetStaticTxnId to SetStaticTxnInfo to store transaction debug
    information.
  • +1/-1     
    txn.go
    Add logging for transaction commits and update transaction info
    handling

    pkg/frontend/txn.go

  • Added logging for successful transaction commits.
  • Updated transaction commit handling to store debug information.
  • +10/-2   
    types.go
    Replace static transaction ID methods with transaction info methods

    pkg/frontend/types.go

  • Replaced SetStaticTxnId and GetStaticTxnId with SetStaticTxnInfo and
    GetStaticTxnInfo.
  • Added a new field txnInfo to store transaction debug information.
  • +14/-8   
    util.go
    Update logging to use transaction debug information           

    pkg/frontend/util.go

  • Updated logging to use transaction debug information instead of
    transaction ID.
  • +3/-3     

    💡 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 Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    3

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The order of operations in commitUnsafe method in txn.go has been altered. The transaction ID and debug information are now being set after the transaction commit attempt, which could potentially lead to incorrect or missing debug information if an error occurs during commit.

    Redundancy:
    The TODO:: comment in txn.go needs clarification or completion as it might be left unintentionally.

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 12, 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
    Possible bug
    Add a nil check before accessing DebugString() to prevent potential nil pointer dereference

    Consider checking if txnOp.Txn() is not nil before calling DebugString() to avoid
    potential nil pointer dereference.

    pkg/frontend/mysql_cmd_executor.go [2298]

    -ses.SetStaticTxnInfo(txnOp.Txn().DebugString())
    +if txnOp.Txn() != nil {
    +    ses.SetStaticTxnInfo(txnOp.Txn().DebugString())
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies a potential nil pointer dereference, which is a critical issue that could cause runtime crashes.

    8
    Enhancement
    Add a log statement before committing the transaction for better traceability

    Add a log statement before committing the transaction to provide better traceability and
    debugging information.

    pkg/frontend/txn.go [513]

    +ses.Debug(ctx2, "committing transaction", logutil.TxnIdField(th.txnOp.Txn().ID))
     err = th.txnOp.Commit(ctx2)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a log statement before a critical operation like committing a transaction is a good practice for better traceability and debugging, making this a valuable suggestion.

    7
    Combine if-else conditions to reduce redundancy and improve code clarity

    Combine the if-else conditions to reduce redundancy and improve code clarity.

    pkg/frontend/util.go [501-506]

    +logFields := []zapcore.Field{
    +    logutil.StatementField(str),
    +    logutil.StatusField(status.String()),
    +    logutil.TxnIdField(txnInfo),
    +}
     if status == success {
    -    ses.Debug(ctx, "query trace status", logutil.StatementField(str), logutil.StatusField(status.String()), logutil.TxnIdField(txnInfo))
    +    ses.Debug(ctx, "query trace status", logFields...)
         err = nil // make sure: it is nil for EndStatement
     } else {
    -    ses.Error(ctx, "query trace status", logutil.StatementField(str), logutil.StatusField(status.String()), logutil.ErrorField(err),
    -        logutil.TxnIdField(txnInfo))
    +    logFields = append(logFields, logutil.ErrorField(err))
    +    ses.Error(ctx, "query trace status", logFields...)
     }
     
    Suggestion importance[1-10]: 5

    Why: The suggestion to refactor the logging code to reduce redundancy is valid and improves code clarity, but the impact on functionality is minimal.

    5
    Maintainability
    Remove commented-out code to improve readability and maintainability

    Remove the commented-out code to improve code readability and maintainability.

    pkg/frontend/txn.go [511-512]

    -//commitTs := th.txnOp.Txn().CommitTS
    -//execCtx.ses.SetTxnId(th.txnOp.Txn().ID)
     
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Removing commented-out code is a good practice for maintaining clean and readable code, although it's not a critical issue.

    6

    pkg/frontend/txn.go Outdated Show resolved Hide resolved
    if status == success {
    ses.Debug(ctx, "query trace status", logutil.StatementField(str), logutil.StatusField(status.String()))
    ses.Debug(ctx, "query trace status", logutil.StatementField(str), logutil.StatusField(status.String()), logutil.TxnIdField(txnInfo))
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    默认是Info级别。Debug接口不回打印任何日志的。

    改为info后,打印会非常多。

    要判断确定sql才能改为info。

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    加了

    @jensenojs jensenojs requested a review from daviszhen June 12, 2024 11:05
    @@ -337,8 +338,10 @@ type FeSession interface {
    ResetFPrints()
    EnterFPrint(idx int)
    ExitFPrint(idx int)
    SetStaticTxnId(id []byte)
    GetStaticTxnId() uuid.UUID
    //SetStaticTxnId(id []byte)
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    不要都删掉吧。

    @@ -504,12 +511,16 @@ func (th *TxnHandler) commitUnsafe(execCtx *ExecCtx) error {
    if th.txnOp != nil {
    execCtx.ses.EnterFPrint(79)
    defer execCtx.ses.ExitFPrint(79)
    commitTs := th.txnOp.Txn().CommitTS
    execCtx.ses.SetTxnId(th.txnOp.Txn().ID)
    //commitTs := th.txnOp.Txn().CommitTS
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    不要了,都删掉吧。

    @triump2020 triump2020 closed this Jul 8, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix Enhancement 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.

    None yet

    4 participants