sqlstats: generate PCT_RUNTIME sort when requested #123846
Conversation
|
This change is |
dhartunian
added
backport-23.1.x
There was a problem hiding this comment.
Nice tests. I think in the future (as part of the redesign for this api) it'd be cool to do something like this where we can programmatically do this for all of the available sort options in the request.
Just the one comment with the sort and internal options.
| &serverpb.CombinedStatementsStatsRequest{ | ||
| FetchMode: &serverpb.CombinedStatementsStatsRequest_FetchMode{ | ||
| StatsType: serverpb.CombinedStatementsStatsRequest_StmtStatsOnly, | ||
| Sort: serverpb.StatsSortOptions(serverpb.StatsSortOptions_value[sortString]), |
There was a problem hiding this comment.
I think you're missing the limit here, and I don't think internal is a field on the request.
There was a problem hiding this comment.
Oh, I missed that internal was being passed below in the function - ignore that part.
There was a problem hiding this comment.
I just took out internal as a flag in the test cases, and added start/end
dhartunian
force-pushed
the
fix-missing-sort-for-pct-runtime
branch
2 times, most recently
from
e563642
to
314bcf8
Compare
| --ARGS-- | ||
| [1970-01-01 00:00:01 +0000 UTC 1970-01-01 00:00:02 +0000 UTC 100] | ||
|
|
||
| query sort=SERVICE_LAT limit=100 start=1 end=2 |
There was a problem hiding this comment.
nit: Could we have some test cases where limit is something other than 100 / when it's omitted? Just to verify it's putting the correct value.
Previously, when a request was made to the combined stats endpoint on CRDB with a `PCT_RUNTIME` sort, we'd default to a `svcLatency` sort instead because of a missing switch case. This change fills that gap and adds some datadriven tests to inspect the resulting `WHERE` clauses. Fixes: cockroachdb#123841 Epic: CRDB-37544 Release note (ui change): Viewing SQL Activity sorted by `% of Runtime` now correctly sorts entries by the runtime amount.
dhartunian
force-pushed
the
fix-missing-sort-for-pct-runtime
branch
from
314bcf8
to
a8636af
Compare
|
bors r=xinhaoz |
Previously, when a request was made to the combined stats endpoint on CRDB with a
PCT_RUNTIMEsort, we'd default to asvcLatencysort instead because of a missing switch case.This change fills that gap and adds some datadriven tests to inspect the resulting
WHEREclauses.Fixes: #123841
Epic: CRDB-37544
Release note (ui change): Viewing SQL Activity sorted by
% of Runtimenow correctly sorts entries by the runtime amount.