Skip to content
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: add plan gist to sampled query telemetry log #83027

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

THardy98
Copy link

@THardy98 THardy98 commented Jun 17, 2022

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.

@THardy98 THardy98 requested a review from a team June 17, 2022 01:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@THardy98 THardy98 force-pushed the sampled_query_add_plan_gist branch from 462349e to e47a109 Compare June 17, 2022 01:16
@THardy98
Copy link
Author

THardy98 commented Jun 17, 2022

@vy-ton currently added the plan_gist in its base64 encoded string form. Is this the form that we wanted, or the decoded plan gist rows?

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 "PlanGist":"‹x›", the 'x' marker is where the plan gist would've been). If we aren't worried about PII/sensitive application data in the plan gist, we can remove these markers.

@cucaroach
Copy link
Contributor

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?

@THardy98
Copy link
Author

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

@THardy98 THardy98 force-pushed the sampled_query_add_plan_gist branch 2 times, most recently from a7e5336 to 5a6b198 Compare June 28, 2022 13:33
@THardy98 THardy98 requested a review from a team June 28, 2022 17:32
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 5 files at r2, all commit messages.
Reviewable status: :shipit: 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

@THardy98
Copy link
Author

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.
@THardy98 THardy98 force-pushed the sampled_query_add_plan_gist branch from 5a6b198 to 39a562c Compare June 28, 2022 18:08
Copy link
Author

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @THardy98)


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 :)

@THardy98 THardy98 requested a review from maryliag June 28, 2022 19:53
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 5 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @THardy98)

@THardy98
Copy link
Author

TYFR :)

@THardy98
Copy link
Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 28, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jun 29, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jun 29, 2022

Build failed (retrying...):

@craig craig bot merged commit 0917fdc into cockroachdb:master Jun 29, 2022
@craig
Copy link
Contributor

craig bot commented Jun 29, 2022

Build succeeded:

@THardy98
Copy link
Author

blathers backport 22.1 21.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enrich the existing query log format for cloud telemetry
4 participants