-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql, insights: record error message for failed stmts #110565
Conversation
cec95d1
to
3998167
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1, 9 of 10 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)
pkg/ccl/logictestccl/testdata/logic_test/crdb_internal
line 206 at r2 (raw file):
tenant_id capability_name capability_value 1 can_admin_split true 5 can_admin_split true
is this intentional? seems unrelated to this PR
pkg/sql/sqlstats/insights/integration/insights_test.go
line 209 at r2 (raw file):
// Create VIEWACTIVITY and VIEWACTIVITYREDACTED users. //const viewUser = "view_user"
looks like you need to do some cleanup on these comments
3998167
to
962731b
Compare
962731b
to
0ee97b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)
pkg/ccl/logictestccl/testdata/logic_test/crdb_internal
line 206 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
is this intentional? seems unrelated to this PR
I used --rewrite since I added some columns to the internal tables. Not sure why this part was changed, too. I'll check if removing this change will break the test.
pkg/sql/sqlstats/insights/integration/insights_test.go
line 209 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
looks like you need to do some cleanup on these comments
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3, 10 of 11 files at r4, 11 of 11 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 11 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, and @xinhaoz)
pkg/cli/zip_table_registry.go
line 165 at r5 (raw file):
"retries", "last_retry_reason", "error_code",
Shouldn't this be a sensitive column?
Previously, j82w (Jake) wrote…
The error code itself doesn't have any sensitive info from what I can tell. Looks like the retry reason was added in a separate commit despite it being listed as sensitive 🤔 . We can take it up with sql-foundations. |
0ee97b5
to
64645a3
Compare
bors r+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 11 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ericharmeling, @j82w, @maryliag, and @xinhaoz)
pkg/cli/zip_table_registry.go
line 165 at r5 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
The error code itself doesn't have any sensitive info from what I can tell. Looks like the retry reason was added in a separate commit despite it being listed as sensitive 🤔 . We can take it up with sql-foundations.
indeed error code is safe.
pkg/sql/crdb_internal.go
line 8089 at r6 (raw file):
errorMsg := tree.DNull if insight.Transaction.LastErrorMsg != "" && shouldRedactError {
can you store the error in the insight data structure as a redact.RedactableString
and call .Redact()
here instead of deleting everything?
bors r- |
Canceled. |
Previously, knz (Raphael 'kena' Poss) wrote…
I was referring to the |
Previously, knz (Raphael 'kena' Poss) wrote…
If it's a redacted string can we add it to the debug zip? |
redactable != redacted at the point it is still Redactable it still contains PII. Once redacted it does not. So debug zip by default can only capture redacted errors. |
4a1337f
to
4338f0f
Compare
Previously, j82w (Jake) wrote…
Filed #110938. |
4338f0f
to
5b24932
Compare
b02162f
to
118c717
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests.
LGTM modulo reverting the change to the logic test with the 'retry' clause.
Reviewed 1 of 11 files at r5, 11 of 11 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ericharmeling, @j82w, and @maryliag)
pkg/ccl/logictestccl/testdata/logic_test/crdb_internal
line 206 at r2 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
I used --rewrite since I added some columns to the internal tables. Not sure why this part was changed, too. I'll check if removing this change will break the test.
Yeah let's revert that. that seems incorrect. Generally "--rewrite" is unsafe/inconsistent with the "retry" clause in logic tests.
118c717
to
e54f5a9
Compare
This commit adds an additional return value to the function `HasViewActivityOrViewActivityRedactedRole` which returns true if the user has either `VIEWACTIVITY` or `VIEWACTIVITYREDACTED` roles. In addition to that, the function will now return a boolean indicating whether the user is not admin and has the `VIEWACTIVITYREDACTED` role. This simplifies areas where we need to redact values based on the presence of this role, allowing callers to skip an additional role check for admin and VIEWACTIVITYREDACTED. Epic: none Release note: None
Part of: cockroachdb#87785. Previously, the insights subsystem did not keep track of the error messages for failed executions, only the error codes. This commit updates the `[cluster|node]_execution_insights` and `[cluster|node]_txn_execution_insights` virtual tables to include a `last_error` column which contains the most recent error message. Release note (sql change): adds `last_error` column to the `[cluster|node]_execution_insights` and `[cluster|node]_txn_execution_insights` tables which keeps track of the error message for failed executions. Co-authored-by: gtr <gerardo@cockroachlabs.com>
e54f5a9
to
3b1adac
Compare
Thanks for the reviews! |
bors r- |
Canceled. |
Oops, mistakenly canceled bc I thought there was a CI issue. TFTR! |
bors timed out, courtesy merge bors r+ |
This PR was included in a batch that timed out, it will be automatically retried |
Build succeeded: |
Commit 1 sql: modify HasViewActivityOrViewActivityRedactedRole signature
This commit adds an additional return value to the function
HasViewActivityOrViewActivityRedactedRole
which returns true ifthe user has either
VIEWACTIVITY
orVIEWACTIVITYREDACTED
roles.In addition to that, the function will now return a boolean indicating
whether the user is not admin and has the
VIEWACTIVITYREDACTED
role.This simplifies areas where we need to redact values based on the
presence of this role, allowing callers to skip an additional role check
for admin and VIEWACTIVITYREDACTED.
Epic: none
Release note: None
Commit 2 sql, insights: record error message for failed stmts
Part of: #87785.
Previously, the insights subsystem did not keep track of the error messages for failed executions, only the error codes.
This commit updates the
[cluster|node]_execution_insights
and[cluster|node]_txn_execution_insights
virtual tables to include alast_error
column which contains the most recent error message.Release note (sql change): adds
last_error
column to the[cluster|node]_execution_insights
and[cluster|node]_txn_execution_insights
tables which keeps track of the error message for failed executions.