sql: add stmt error code to execution insights tables #96676
Conversation
|
This change is |
gtr
force-pushed
the
failed-stmt-insights
branch
from
a9ede2f
to
c98bccf
Compare
gtr
changed the title
gtr
force-pushed
the
failed-stmt-insights
branch
from
c98bccf
to
4bff927
Compare
gtr
force-pushed
the
failed-stmt-insights
branch
3 times, most recently
from
0898d57
to
6770868
Compare
There was a problem hiding this comment.
can you update the file pkg/sql/sqlstats/insights/integration/insights_test.go to add the new column on the test TestInsightsIntegration and also create a new one testing the cases where you have errors (when the statement is slow so it would also hit that detector, but one that would still be fast and only hit the failed detector)
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @gtr)
-- commits line 4 at r1:
these should be 2 different PRs, since they're not related
also, looks like the changes here are fixing 94381 and not just "part of"
gtr
force-pushed
the
failed-stmt-insights
branch
from
6770868
to
812615c
Compare
There was a problem hiding this comment.
Sure thing! I put the slow and failure detectors tests in pkg/sql/sqlstats/insights/detector_test.go.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
these should be 2 different PRs, since they're not related
also, looks like the changes here are fixing 94381 and not just "part of"
Done.
gtr
force-pushed
the
failed-stmt-insights
branch
from
812615c
to
5d3175e
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, gtr (Gerardo Torres Castro) wrote…
Done.
You tagged the wrong issue here.
This PR is dealing with 87785, and should be Part Of, since you haven't finish and still need to add to the UI.
The other PR which I imagine you're creating with the updates on the detector is the one where you tag as Fixes 94381
gtr
force-pushed
the
failed-stmt-insights
branch
from
5d3175e
to
aec1c9c
Compare
Part of: cockroachdb#87785. Previously, the insights subsystem did not keep track of `error code` for failed executions and only contained a `FailedExecution` value for the problem field. This commit adds the `error_code` column to the `crdb_internal.{cluster/node}_execution_insights` virtual tables. This commit also bubbles up that error code at the transaction level for writing into the `crdb_internal.{cluster/node}_txn_execution_insights` virtual table. Release note (sql change): Adds `error_code` column to the `crdb_internal.{cluster/node}_execution_insights` virtual tables, which contains the error code for a failed execution. Adds `last_error_code` column to the `crdb_internal.{cluster/node}_txn_execution_insights` virtual tables, which is the error code of the last failed statement in that transaction.
gtr
force-pushed
the
failed-stmt-insights
branch
from
aec1c9c
to
6c783f3
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/insights/integration/insights_test.go line 157 at r3 (raw file):
var cpuSQLNanos int64 var errorCode string err = row.Scan(&query, &startInsights, &endInsights, &implicitTxn, &cpuSQLNanos, &errorCode)
you need to add some test here with a failed execution and confirm that the error code is showing up as expected.
You should check in both statement and transaction insights
There was a problem hiding this comment.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
You tagged the wrong issue here.
This PR is dealing with 87785, and should be Part Of, since you haven't finish and still need to add to the UI.
The other PR which I imagine you're creating with the updates on the detector is the one where you tag as Fixes 94381
Opened two PRs for the two issues:
- insights: detect failed executions at the stmt and txn levels #97039 to detect failed executions
- sql: record error code for failed executions to execution insights tables #97046 to add error code for failed executions in the execution insights tables
pkg/sql/sqlstats/insights/integration/insights_test.go line 157 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you need to add some test here with a failed execution and confirm that the error code is showing up as expected.
You should check in both statement and transaction insights
Done.
Part of: #87785.
Previously, the insights subsystem did not keep track of
error codeforfailed executions and only contained a
FailedExecutionvalue for theproblem field. This commit adds the
error_codecolumn to thecrdb_internal.{cluster/node}_execution_insightsvirtual tables. Thiscommit also bubbles up that error code at the transaction level for
writing into the
crdb_internal.{cluster/node}_txn_execution_insightsvirtual table.
Release note (sql change): Adds
error_codecolumn to thecrdb_internal.{cluster/node}_execution_insightsvirtual tables, whichcontains the error code for a failed execution. Adds
last_error_codecolumn to the
crdb_internal.{cluster/node}_txn_execution_insightsvirtual tables, which is the error code of the last failed statement in
that transaction.