sql: add plan gist to sampled query telemetry log#83027
sql: add plan gist to sampled query telemetry log#83027craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
462349e to
e47a109
Compare
|
@vy-ton currently added the Providing the decoded plan gist rows logistically wouldn't be difficult, but I imagine some performance testing would be necessary if we were to decode a plan every time we sample a query for telemetry logging. On a separate note, I'm not sure what kind of PII exists in the plan gist. As it currently stands, the base64 encoded plan gist is marked for redaction. If the user decides to redact this information, we would essentially lose the plan gist (it would look something like |
|
I don’t know what a telemetry log is but just writing the gist sounds fine, the plan can be decoded with decode_plan_gist. The gist is just integer ids there's no PII in there. What's the use case/context for where we might want to decode the gist? |
I imagine for debugging/investigation purposes |
a7e5336 to
5a6b198
Compare
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 4 of 5 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @THardy98)
docs/generated/eventlog.md line 2461 at r2 (raw file):
| `CostEstimate` | Cost of the query as estimated by the optimizer. | no | | `Distribution` | The distribution of the DistSQL query plan (local, full, or partial). | no | | `PlanGist` | The query's plan gist bytes as a base64 encoded string. | yes |
plan gist is not sensitive
pkg/util/log/eventpb/json_encode_generated.go line 3294 at r2 (raw file):
printComma = true b = append(b, "\"PlanGist\":\""...) b = append(b, redact.StartMarker()...)
there is no need to redact the gist
pkg/sql/telemetry_logging_test.go line 235 at r2 (raw file):
} // Match plan gist on any non-empty string value. planGist := regexp.MustCompile("\"PlanGist\":(\"\\S+\")")
when this test is executed? we have cases where we don't have a plan gist, so we might not send this, which will fail your test
Only executes for the test cases above which produce plan gists. |
Partially resolves: cockroachdb#71328 This change adds a plan gist field to the sampled query telemetry log. The plan gist is written as a base64 encoded string. Release note (sql change): The sampled query telemetry log now includes a plan gist field. The plan gist field provides a compact representation of a logical plan for the sampled query, the field is written as a base64 encoded string.
5a6b198 to
39a562c
Compare
THardy98
left a comment
There was a problem hiding this comment.
Reviewable status:
docs/generated/eventlog.md line 2461 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
plan gist is not sensitive
Tagged as non-sensitive.
pkg/util/log/eventpb/json_encode_generated.go line 3294 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
there is no need to redact the gist
Ditto :)
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 1 of 5 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:
|
TYFR :) |
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
|
blathers backport 22.1 21.2 |
Partially resolves: #71328
This change adds a plan gist field to the sampled query telemetry log.
The plan gist is written as a base64 encoded string.
Release note (sql change): The sampled query telemetry log now includes
a plan gist field. The plan gist field provides a compact representation
of a logical plan for the sampled query, the field is written as a
base64 encoded string.