storage,roachpb: expose pebble.InternalIteratorStats for scans

[sumeerbhola]

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

[cockroach-teamcity]

This change is 

[sumeerbhola]

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)

[yuzefovich]

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).

[nicktrav]

pkg/storage

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @Azhng, and @yuzefovich)

[sumeerbhola]

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.ScanStats in joinReader looks unnecessary. It is read into and then read from in the same method. The same is true of invertedJoiner.
• The implementations of execStatsForTrace in both joinReader and invertedJoiner look 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.ScanStats and execinfrapb.KVStats. Even if we add those, I don't think it will change any of the integration in these joiners.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @Azhng, and @yuzefovich)

[Azhng]

Reviewed 1 of 6 files at r1.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @andreimatei, @sumeerbhola, and @yuzefovich)

---

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?

[sumeerbhola]

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?

[sumeerbhola]

TFTRs!

[sumeerbhola]

bors r=azhng,yuzefovich,nicktrav

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[Azhng]

It depends on how useful it is to only have the current trace statements. Do you know the answer?

Hmm I don't have a definite answer 🤔 . I filed #77580 so we can track the plumbing work.