Skip to content
This repository has been archived by the owner on Jun 14, 2023. It is now read-only.

fix(sql): handle commit/rollback error when tracing transcation #40

Merged
merged 7 commits into from
Dec 28, 2021

Conversation

kagaya85
Copy link
Member

Now, sql plugin will close a span when commit/rollback returns error, it may call span.End() repeatly and cause a panic, we want return the database/sql errors to the upper instead of get a panic.

Add defer function to handle the return error and will not call span.End() when error is not nil

fix #38

@kagaya85 kagaya85 requested a review from arugal November 23, 2021 08:58
@kagaya85
Copy link
Member Author

How about add the error message to the span and close it even the commit failed, to avoid unexpected panic, this may need add a method like span.IsEnd() to check if the transcation span is closed. If we don't call span.End() when commit failed, the previous related span in this transcation period may not be reported.

@wu-sheng
Copy link
Member

Now, sql plugin will close a span when commit/rollback returns error, it may call span.End() repeatly and cause a panic, we want return the database/sql errors to the upper instead of get a panic.
Add defer function to handle the return error and will not call span.End() when error is not nil

I think we should consider separating the span, because as you mentioned in the codes, if there is connection leak(commit error, but no rollback called) or rollback fails, the span wouldn't be closed as expected, which is unexpected.

The tracing system is designed to tell the truth, rather than not working as well as original codes.

You could check the SkyWalking Java agent's JDBC plugin, commit and rollback have there own spans, then no panic and always presents the complete trace.

sql/tx.go Outdated Show resolved Hide resolved
@wu-sheng wu-sheng added the bug Something isn't working label Nov 23, 2021
@wu-sheng wu-sheng added this to the 1.4.0 milestone Nov 23, 2021
wu-sheng
wu-sheng previously approved these changes Dec 27, 2021
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM, wait for @arugal

@kagaya85
Copy link
Member Author

Please hold this PR, we consider using separate Begin/Commit/Rollback span instead of the Tx span, in addition, there are two considerations may need to discuss:

  • After using the separate span, the trace will not show the entire execution process for each transcation directly, for example, understanding one transcation cost time will become difficult
  • Span validity check is also needed in this PR, even in septarate tx span, we need to ensure the parent span is still valid when start a new span to avoid panic, I can improve this in another PR

@wu-sheng
Copy link
Member

After using the separate span, the trace will not show the entire execution process for each transcation directly, for example, understanding one transcation cost time will become difficult

You could calculate the whole time by using tag or log in the span, such as commit or rollback. Also, the backend provides the logic endpoint concept(logic name + duration in tags) for you to measure things like this.

Span validity check is also needed in this PR, even in septarate tx span, we need to ensure the parent span is still valid when start a new span to avoid panic, I can improve this in another PR

You could make the choice whether in this PR or through another one.

sql/go.mod Outdated Show resolved Hide resolved
@wu-sheng wu-sheng merged commit 5c98d6e into SkyAPM:master Dec 28, 2021
@kagaya85 kagaya85 deleted the fix-tx-panic branch December 28, 2021 13:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rollback error after mysql database transaction Commit
2 participants