sql: include sampled stats in TestSampledStatsCollection#60424
sql: include sampled stats in TestSampledStatsCollection#60424craig[bot] merged 1 commit intocockroachdb:masterfrom asubiotto:rttm
Conversation
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
The last commit
Reviewed 2 of 2 files at r5.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @asubiotto)
pkg/sql/instrumentation_test.go, line 83 at r5 (raw file):
require.Equal(t, int64(1), stats.mu.data.ExecStatCollectionCount, "expected to have collected exactly one set of execution stats") require.Greater(t, stats.mu.data.RowsRead.Mean, float64(0), "expected statement to have read at least one row") require.Greater(t, stats.mu.data.MaxMemUsage.Mean, float64(0), "expected statement to have requested bytes")
nit: to me "to have requested bytes" reads like it is more about BytesRead and not MaxMemUsage, maybe expected statement to request some RAM or something like that?
TestSampledStatsCollection would previously only check that stats that are collected regardless of the sample rate are returned. These types of stats (rows/bytes read) are propagated using metadata, rather than the trace. This resulted in us silently failing to collect any stats when sampling was enabled once the tracing mode was reverted back to legacy. To avoid this kind of thing happening again, this commit adds a check that max memory usage is reported to be non-zero. Release note: None (this is a new feature that has no user impact yet)
asubiotto
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/instrumentation_test.go, line 83 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: to me "to have requested bytes" reads like it is more about BytesRead and not MaxMemUsage, maybe
expected statement to request some RAMor something like that?
Done.
|
TFTR bors r=yuzefovich |
|
Build succeeded: |
Depends on #59992, which is required for this new regression test to pass.
TestSampledStatsCollection would previously only check that stats that are
collected regardless of the sample rate are returned. These types of stats
(rows/bytes read) are propagated using metadata, rather than the trace.
This resulted in us silently failing to collect any stats when sampling was
enabled once the tracing mode was reverted back to legacy. To avoid this kind
of thing happening again, this commit adds a check that max memory usage is
reported to be non-zero.
Release note: None (this is a new feature that has no user impact yet)