stmtdiagnostics: save the bundle on a statement timeout #88080
Conversation
|
This change is |
yuzefovich
force-pushed
the
stmt-diag-timeout
branch
from
64eaff3 to
84ee70b
Compare
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rytaft and @yuzefovich)
pkg/sql/stmtdiagnostics/statement_diagnostics.go line 483 at r1 (raw file):
var diagID CollectedInstanceID if ctx.Err() != nil { // If the context was canceled (likely due to a statement timeout), we
What if it wasn't a timeout? Are we potentially swallowing/hiding some real error? What if we get here and the statement hasn't timed out yet?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/stmtdiagnostics/statement_diagnostics.go line 483 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
What if it wasn't a timeout? Are we potentially swallowing/hiding some real error? What if we get here and the statement hasn't timed out yet?
More specifically if the code gets past this and then times out would the unit test fail?
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/sql/stmtdiagnostics/statement_diagnostics.go line 483 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
More specifically if the code gets past this and then times out would the unit test fail?
Maybe we should at least log the error?
yuzefovich
force-pushed
the
stmt-diag-timeout
branch
2 times, most recently
from
cb27705 to
53316a8
Compare
There was a problem hiding this comment.
The newly-added unit test exposed a minor bug (namely that the conditional request is not removed from the registry even after it was satisfied), which I'll fix separately. In order to make CI happy I had to move the test a bit.
Reviewable status:
pkg/sql/stmtdiagnostics/statement_diagnostics.go line 483 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Maybe we should at least log the error?
This code is only executed at the very end, after the query has completed running and we assembled everything for the bundle. If somehow the context gets an error only when we got here, it seems ok to proceed. It's true that we don't know the true reason for why the context is busted, so I added a deadline of 10 seconds for inserting the bundle. I also added logging of the error.
Previously if the traced statement is canceled due to a statement timeout, the statement bundle would be created but would fail on the insertion into the system table. This is suboptimal because we already did all the work to collect the bundle as well as it might be desired to see the partial trace, so this commit makes it so that the bundle is saved correctly. Release note (bug fix): Previously, when a statement bundle was collected for a query that results in an error due to a `statement_timeout`, the bundle would not be saved, and this is now fixed.
yuzefovich
force-pushed
the
stmt-diag-timeout
branch
from
53316a8 to
532274b
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:
|
TFTRs! bors r+ |
|
Build succeeded: |
Previously if the traced statement is canceled due to a statement timeout, the statement bundle would be created but would fail on the insertion into the system table. This is suboptimal because we already did all the work to collect the bundle as well as it might be desired to see the partial trace, so this commit makes it so that the bundle is saved correctly.
Fixes: #73477.
Epic: CRDB-14510
Release note (bug fix): Previously, when a statement bundle was collected for a query that results in an error due to a
statement_timeout, the bundle would not be saved, and this is now fixed.