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

outliers: collect statement fingerprint id #80989

Merged
merged 1 commit into from
May 6, 2022
Merged

outliers: collect statement fingerprint id #80989

merged 1 commit into from
May 6, 2022

Conversation

matthewtodd
Copy link
Contributor

This change also helps set us up for #79451, where we'll be working with
per-fingerprint statement latencies.

Release note: None

@matthewtodd matthewtodd requested a review from a team May 4, 2022 17:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@matthewtodd matthewtodd requested review from a team and removed request for a team May 4, 2022 17:18
Copy link
Contributor

@Azhng Azhng 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 @Azhng and @matthewtodd)


pkg/sql/crdb_internal.go line 6089 at r1 (raw file):

	transaction_id           UUID NOT NULL,
	statement_id             STRING NOT NULL,
	statement_fingerprint_id STRING NOT NULL

Hmm are we sure we want to have a string representation of the fingerprints ID? At least in SQL CLI, we haven't really committed to a pretty format of the fingerprint. Also encoding it here in the virtual table handler will make joining to the exsting stmt/txn stats table difficult.


pkg/sql/sqlstats/outliers/outliers.proto line 30 at r1 (raw file):

    bytes id = 1 [(gogoproto.customname) = "ID"];
    uint64 fingerprint_id = 3 [(gogoproto.customname) = "FingerprintID",
      (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.StmtFingerprintID"];

nit: maybe this can be below the latency_in_seconds since it's tag is higher :P

Copy link
Contributor Author

@matthewtodd matthewtodd 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 @Azhng)


pkg/sql/crdb_internal.go line 6089 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm are we sure we want to have a string representation of the fingerprints ID? At least in SQL CLI, we haven't really committed to a pretty format of the fingerprint. Also encoding it here in the virtual table handler will make joining to the exsting stmt/txn stats table difficult.

Good call, I was wondering about this and saw the base-10 string encoding elsewhere, but now I see we just use bytes, yes? Will change.


pkg/sql/sqlstats/outliers/outliers.proto line 30 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: maybe this can be below the latency_in_seconds since it's tag is higher :P

Good question. I was looking to have the ordering here mirror the column ordering in the node_execution_outliers table, assuming there's some logical grouping there. (In particular, having these ID fields at the top feels good.) Maybe I just renumber them since nobody cares yet?

This change also helps set us up for #79451, where we'll be working with
per-fingerprint statement latencies.

Release note: None
Copy link
Contributor

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @Azhng)

@matthewtodd
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented May 6, 2022

Build succeeded:

@craig craig bot merged commit 44c2fd5 into cockroachdb:master May 6, 2022
@matthewtodd matthewtodd deleted the outliers-stmt-fpt-id branch May 6, 2022 21:04
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.

3 participants