server, sql: span stats returns logical MVCC stats

[zachlite]

Previously, the MVCC stats returned by span stats were mistakenly accumulated from each node. This commit fixes that, and adds clarifying comments.

Resolves: #108766 Epic: CRDB-30635
Release note (bug fix): A bug was fixed where a SpanStatsRequest would return post-replicated MVCC stats. Now, a SpanStatsRequest returns the logical MVCC stats for the requested span.

[cockroach-teamcity]

This change is 

[maryliag]

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @erikgrinaker, and @zachlite)

---

pkg/server/span_stats_server.go line 116 at r1 (raw file):

				res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes
				res.SpanToStats[spanStr].RemoteFileBytes += spanStats.RemoteFileBytes
				res.SpanToStats[spanStr].ExternalFileBytes += spanStats.ExternalFileBytes

there is no need for the rangeCount here?

[zachlite]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @erikgrinaker, and @maryliag)

---

pkg/server/span_stats_server.go line 116 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

there is no need for the rangeCount here?

No. If we did accumulate range count here, it would really be replica count.

[maryliag]

still want to see an approval from @dhartunian or @erikgrinaker

Reviewed 1 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian and @erikgrinaker)

[erikgrinaker]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @maryliag, and @zachlite)

---

-- commits line 9 at r1:
Span stats are new in 23.2, right? If so, there's no need to add a release note, since we haven't ever released the bug that we're fixing here.

---

pkg/roachpb/span_stats.proto line 40 at r1 (raw file):

message SpanStats {
  // TotalStats are the logical MVCC stats for the requested span.
  cockroach.storage.enginepb.MVCCStats total_stats = 1 [(gogoproto.nullable) = false];

nit: consider simply stats here. total_stats can be a bit misleading, since it's not a sum.

---

pkg/roachpb/span_stats.proto line 50 at r1 (raw file):

  // data in the span. NB; this *includes* files stored remotely, not just on
  // _local_ disk; see the RemoteFileBytes field below.
  // It represents a physical value is accumulated by the fan-out.

nit: "physical value across all replicas"

---

pkg/server/span_stats_server.go line 114 at r1 (raw file):

			if _, ok := responses[spanStr]; ok {
				// Accumulate values for physical quantities:
				res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes

nit: consider restructuring this to avoid duplicating fields, which also avoids future issues where developers may forget to update both sites. Something like:

for spanStr, spanStats := range nodeResponse.SpanToStats {
  // Accumulate values for physical quantities:
  res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes
  // ...other accumulated fields

  // Logical values: take the values from the node that responded first.
  // TODO: this should really be read from the leaseholder.
  if _, ok := responses[spanStr]; !ok {
    res.SpanToStats[spanStr].TotalStats = spanStats.TotalStats
    // ...other non-accumulated fields
  }
}

[zachlite]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian, @erikgrinaker, and @maryliag)

---

-- commits line 9 at r1:

Previously, erikgrinaker (Erik Grinaker) wrote…

Span stats are new in 23.2, right? If so, there's no need to add a release note, since we haven't ever released the bug that we're fixing here.

No, span stats were added in 23.1. The new functionality inSHOW RANGES WITH DETAILS is new in 23.2... and hopefully we'll be able to backport that to 23.1, but maybe not.

---

pkg/roachpb/span_stats.proto line 40 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: consider simply stats here. total_stats can be a bit misleading, since it's not a sum.

Ack - but I'm not going to touch right now, since I want to backport this.

---

pkg/roachpb/span_stats.proto line 50 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: "physical value across all replicas"

Done, thanks!

---

pkg/server/span_stats_server.go line 114 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: consider restructuring this to avoid duplicating fields, which also avoids future issues where developers may forget to update both sites. Something like:

for spanStr, spanStats := range nodeResponse.SpanToStats {
  // Accumulate values for physical quantities:
  res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes
  // ...other accumulated fields

  // Logical values: take the values from the node that responded first.
  // TODO: this should really be read from the leaseholder.
  if _, ok := responses[spanStr]; !ok {
    res.SpanToStats[spanStr].TotalStats = spanStats.TotalStats
    // ...other non-accumulated fields
  }
}

Done

[zachlite]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 3f3074f to blathers/backport-release-23.1-108852: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}