sql, telemetry: show correctly redacted error message

[dhartunian]

Previously, the error string from an executed SQL statement was treated
as a string type which would get fully redacted on its way out to the
telemetry log.

This change writes the error type into a RedactableString which
preserves the redaction in the error as intended. This will preserve the
template string which we consider safe by default, for instance.

The CommonSQLExecDetails type in sql_audit_events.proto has been
modified to have the error_text field represented as a
RedactableString which helps us maintain the preservation of redaction
markers.

Resolves: #78353

Release note (ops change): Telemetry logs will now display more finely
redacted error messages from sql execution. Previously, the entire error
string was fully redacted.

[cockroach-teamcity]

This change is 

[abarganier]

Reviewable status: complete! 1 of 0 LGTMs obtained

[andreimatei]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @dhartunian)

---

pkg/util/log/eventpb/sql_audit_events.proto line 33 at r1 (raw file):

  string sqlstate = 3 [(gogoproto.customname) = "SQLSTATE", (gogoproto.jsontag) = ",omitempty", (gogoproto.moretags) = "redact:\"nonsensitive\""];
  // The text of the error if any.
  string error_text = 4 [(gogoproto.jsontag) = ",omitempty", (gogoproto.customtype) = "github.com/cockroachdb/redact.RedactableString", (gogoproto.nullable) = false, (gogoproto.moretags) = "redact:\"mixed\""];

is there a way to test this field? The test you've modified doesn't seems to deal with it... or, does it?

---

pkg/sql/telemetry_logging_test.go line 117 at r1 (raw file):

		expectedUnredactedTags  []string
		expectedApplicationName string
		expectedErr             string

nit: comment that empty means no error, and, if you're so inclined, take the opportunity to turn all the "unkeyed literals" below into "keyed literals" (i.e. {name: "truncate-table-query", query: ""...}) - as they should have been from the beginning - and then elide the "" for the error.

---

pkg/sql/telemetry_logging_test.go line 193 at r1 (raw file):

			stubTime := timeutil.FromUnixMicros(int64(execTimestamp * 1e6))
			st.setTime(stubTime)
			_, _ = db.DB.ExecContext(context.Background(), tc.query)

shouldn't we fail here for test cases where we're not expecting an error?

[rafiss]

thanks for the fix! is this getting backported?

[vy-ton]

thanks for the fix! is this getting backported?
+1, I need this backported to analyze IMPORT errors

[dhartunian]

yep, I'll backport. thanks for the reminder.
def for 22.1, and I will also attempt 21.2.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei)

---

pkg/util/log/eventpb/sql_audit_events.proto line 33 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

is there a way to test this field? The test you've modified doesn't seems to deal with it... or, does it?

yes, I've added an error text with partial redaction that's verified to return in the newly added test case.
If I don't make the change, the error will have redaction markers around the whole string like this:

    telemetry_logging_test.go:268: sql-error: missing error a role/user named ‹root› already exists in message {"Timestamp":1658157157275859295,"EventType":"sampled_query","Statement":"CREATE USER root","Tag":"CREATE ROLE","User":"root","ApplicationName":"telemetry-logging-test","ExecMode":"exec","SQLSTATE":"42710","ErrorText":"‹a role/user named root already exists›","Age":1.031134,"TxnCounter":19,"CostEstimate":0.02,"Distribution":"local","PlanGist":"Ais="}

---

pkg/sql/telemetry_logging_test.go line 117 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: comment that empty means no error, and, if you're so inclined, take the opportunity to turn all the "unkeyed literals" below into "keyed literals" (i.e. {name: "truncate-table-query", query: ""...}) - as they should have been from the beginning - and then elide the "" for the error.

done.

---

pkg/sql/telemetry_logging_test.go line 193 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

shouldn't we fail here for test cases where we're not expecting an error?

yep! done.

[dhartunian]

TFTRs

bors r=andreimatei,rafiss,abarganier

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 53f6a05 to blathers/backport-release-21.2-83807: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}