storage,roachpb: expose pebble.InternalIteratorStats for scans#77512
storage,roachpb: expose pebble.InternalIteratorStats for scans#77512craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
The current IteratorStats only include information from the top of the tree (pebble.Iterator). The new InternalIteratorStats includes block bytes loaded (and cached), and stats for points iterated over when merging across levels in the log-structured merge tree. The latter includes points that were ignored because of range tombstones, but could not be cheaply skipped. These stats can be used to make inferences about root causes of slow scans. This change exposes these in roachpb.ScanStats and the trace span recorded when doing a mvcc get or scan. There are todos for plumbing all or part of it through the higher layers via roachpb.ScanStats and execinfrapb.KVStats. Informs cockroachdb#59069 Informs cockroachdb/pebble#1342 Release justification: low risk, and potentially high benefit observability increase for existing functionality. Release note: None
|
This change is |
sumeerbhola
left a comment
There was a problem hiding this comment.
This is a trivial PR, that exposes additional stats for scans.
@yuzefovich @Azhng : I am including you here because you reviewed #64503 by @jordanlewis, that initially added plumbing for stats.
@andreimatei : including you since you have expressed interest in the past, and because this is adding a slightly bigger tracing payload.
I would like to get this in for v22.1. One question is whether we have enough tracing for customer deployments that we will be able to see these stats and make inferences, or do we think we need to do the additional upstream plumbing that is currently listed as todos.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @Azhng, and @yuzefovich)
nicktrav
left a comment
There was a problem hiding this comment.
pkg/storage
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:
sumeerbhola
left a comment
There was a problem hiding this comment.
I'll defer to Archer on the observability side of things (my only comment on #64503 was this which I made after that PR was merged).
I don't have a good answer to that:
- The member variable of type
execinfra.ScanStatsinjoinReaderlooks unnecessary. It is read into and then read from in the same method. The same is true ofinvertedJoiner. - The implementations of
execStatsForTracein bothjoinReaderandinvertedJoinerlook similar, which relates to your question. - These implementations do hook into the same
ProcessorBaseNoHelper.ExecStatsForTrace, so at least there is generalization on when they are called. - The changes in my PR don't propagate to
execinfra.ScanStatsandexecinfrapb.KVStats. Even if we add those, I don't think it will change any of the integration in these joiners.
Reviewable status:
Azhng
left a comment
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1.
Reviewable status:
pkg/sql/execinfra/stats.go, line 60 at r1 (raw file):
// of each stat. // TODO(sql-observability): include other fields that are in roachpb.ScanStats, // here and in execinfrapb.KVStats.
How urgent does this need to be plumbed through?
It depends on how useful it is to only have the current trace statements. Do you know the answer? |
|
TFTRs! |
|
bors r=azhng,yuzefovich,nicktrav |
|
Build succeeded: |
Hmm I don't have a definite answer 🤔 . I filed #77580 so we can track the plumbing work. |
The current IteratorStats only include information from the top
of the tree (pebble.Iterator). The new InternalIteratorStats
includes block bytes loaded (and cached), and stats for points
iterated over when merging across levels in the log-structured
merge tree. The latter includes points that were ignored because
of range tombstones, but could not be cheaply skipped.
These stats can be used to make inferences about root causes
of slow scans.
This change exposes these in roachpb.ScanStats and the trace
span recorded when doing a mvcc get or scan. There are todos for
plumbing all or part of it through the higher layers via
roachpb.ScanStats and execinfrapb.KVStats.
Informs #59069
Informs cockroachdb/pebble#1342
Release justification: low risk, and potentially high benefit
observability increase for existing functionality.
Release note: None