-
Notifications
You must be signed in to change notification settings - Fork 4k
ui, sql: Record KV CPU Time in statement and transaction statistics #158398
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
base: master
Are you sure you want to change the base?
ui, sql: Record KV CPU Time in statement and transaction statistics #158398
Conversation
77852ac to
ab654a6
Compare
kyle-a-wong
left a comment
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.
Looks good, juste have a small request about the new field name
@kyle-a-wong reviewed 16 of 19 files at r1, 1 of 1 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jasonlmfong)
pkg/sql/appstatspb/app_stats.proto line 148 at r1 (raw file):
// representative of the "synchronous" portion of work performed by KV // (ex/ not replication related work). optional NumericStat kv_cpu_time = 38 [(gogoproto.nullable) = false, (gogoproto.customname) = "KVCPUTime"];
Can we name this kv_cpu_time_nanos to match the naming convention of cpu_time_nanos in ExecStats?
pkg/sql/appstatspb/app_stats.proto line 200 at r1 (raw file):
// representative of the "synchronous" portion of work performed by KV // (ex/ not replication related work). optional NumericStat kv_cpu_time = 12 [(gogoproto.nullable) = false, (gogoproto.customname) = "KVCPUTime"];
Same here
pkg/sql/sqlstats/ssprovider.go line 88 at r1 (raw file):
RowsRead int64 RowsWritten int64 KVCPUTime int64
Same comment here, can we name it `KVCPUTimeNanos
jasonlmfong
left a comment
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.
nit: on top of what Kyle said, would it also make sense to replace cpu in go/ts to SQLcpu to contrast the KVcpu even more?
otherwise
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @alyshanjahani-crl and @kyle-a-wong)
pkg/sql/appstatspb/app_stats.proto line 148 at r1 (raw file):
Previously, kyle-a-wong (Kyle Wong) wrote…
Can we name this
kv_cpu_time_nanosto match the naming convention ofcpu_time_nanosin ExecStats?
agree with this one
This commit starts recording the newly added kvCPUTime field from topLevelQueryStats and exposes it in the UI. Fixes: https://cockroachlabs.atlassian.net/browse/CRDB-57265 Release note (ui change): KV CPU Time is now recorded to statement_statistics and transaction_statistics and is displayed in the SQL Activity page.
ab654a6 to
9a84082
Compare
|
Renamed to KVCPUTimeNanos For future reference though, can we not use "requested changes" for nit's / trivial changes / renames. |




This commit starts recording the newly added kvCPUTime field from topLevelQueryStats and exposes it in the UI.
Fixes: https://cockroachlabs.atlassian.net/browse/CRDB-57265 Release note (ui change): KV CPU Time is now recorded to statement_statistics and transaction_statistics and is displayed in the SQL Activity page.