diff --git a/pkg/roachpb/span_stats.proto b/pkg/roachpb/span_stats.proto index d1c93ef998cc..70c53f414025 100644 --- a/pkg/roachpb/span_stats.proto +++ b/pkg/roachpb/span_stats.proto @@ -36,13 +36,18 @@ message SpanStatsRequest { } message SpanStats { + // TotalStats are the logical MVCC stats for the requested span. cockroach.storage.enginepb.MVCCStats total_stats = 1 [(gogoproto.nullable) = false]; // range_count measures the number of ranges that the request span falls within. // A SpanStatsResponse for a span that lies within a range, and whose start // key sorts after the range start, and whose end key sorts before the // range end, will have a range_count value of 1. int32 range_count = 2; - uint64 approximate_disk_bytes = 3; + // ApproximateDiskBytes is the approximate size "on-disk" in all files of the + // data in the span. NB; this *includes* files stored remotely, not just on + // _local_ disk; It represents a physical value across all replicas. + // NB: The explicit jsontag prevents 'omitempty` from being added by default. + uint64 approximate_disk_bytes = 3 [(gogoproto.jsontag) = "approximate_disk_bytes"]; } message SpanStatsResponse { diff --git a/pkg/server/span_stats_server.go b/pkg/server/span_stats_server.go index 8f2fe8941cd2..cac7db5d7c3a 100644 --- a/pkg/server/span_stats_server.go +++ b/pkg/server/span_stats_server.go @@ -45,6 +45,8 @@ func (s *systemStatusServer) spanStatsFanOut( res.SpanToStats[sp.String()] = &roachpb.SpanStats{} } + responses := make(map[string]struct{}) + spansPerNode, err := s.getSpansPerNode(ctx, req) if err != nil { return nil, err @@ -101,15 +103,22 @@ func (s *systemStatusServer) spanStatsFanOut( nodeResponse := resp.(*roachpb.SpanStatsResponse) + // Values of ApproximateDiskBytes should be physical values, but + // TotalStats (MVCC stats) should be the logical, pre-replicated value. + // Note: This implementation can return arbitrarily stale values, because instead of getting + // MVCC stats from the leaseholder, MVCC stats are taken from the node that responded first. + // See #108779. for spanStr, spanStats := range nodeResponse.SpanToStats { - // We are not counting replicas, so only consider range count - // if it has not been set. - if res.SpanToStats[spanStr].RangeCount == 0 { + // Accumulate physical values across all replicas: + res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes + + // 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 res.SpanToStats[spanStr].RangeCount = spanStats.RangeCount + responses[spanStr] = struct{}{} } - - res.SpanToStats[spanStr].TotalStats.Add(spanStats.TotalStats) - res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes } } diff --git a/pkg/server/span_stats_test.go b/pkg/server/span_stats_test.go index b9e73df33262..c78083315c63 100644 --- a/pkg/server/span_stats_test.go +++ b/pkg/server/span_stats_test.go @@ -142,12 +142,12 @@ func TestSpanStatsFanOut(t *testing.T) { } testCases := []testCase{ - {spans[0], 4, int64(numNodes * 6)}, - {spans[1], 1, int64(numNodes * 3)}, - {spans[2], 2, int64(numNodes * 5)}, - {spans[3], 2, int64(numNodes * 1)}, - {spans[4], 2, int64(numNodes * 3)}, - {spans[5], 1, int64(numNodes * 2)}, + {spans[0], 4, int64(6)}, + {spans[1], 1, int64(3)}, + {spans[2], 2, int64(5)}, + {spans[3], 2, int64(1)}, + {spans[4], 2, int64(3)}, + {spans[5], 1, int64(2)}, } testutils.SucceedsSoon(t, func() error {