kvserver, server: new implementation of SpanStats suitable for use with coalesced ranges

[zachlite]

This commit provides a re-implementation of fetching the MVCC stats
and range count for a given span. The previous implementation relied on
the assumption that 1 range can contain at most 1 table/index. Since
GH-79700, this assumption is no longer true.

This re-implementation allows secondary tenants to access span stats
via the serverpb.TenantStatusServer interface.

If a roachpb.SpanStatsRequest has a node_id value of 0, instead of a specific
node id, the response will yield cluster-wide values. To achieve this,
the server does a fan-out to nodes that are known to have replicas for the
span requested.

The interaction between tenants is illustrated below:

 System Tenant
┌────────────────────────────────────────────────────┐
│                                                    │
│                                                    │
│                                    KV Node fan-out │
│                                     ┌──────────┐   │
│                                     │          │   │
│     ┌───────────────────────────────▼────┐     │   │
│     │                                    │     │   │
│     │      server.systemStatusServer     ├─────┘   │
│     │                                    │         │
│     └────────────────┬───────────────────┘         │
│                      │                             │
│                      │                             │
│                      │     via TenantStatusServer  │
│                      ├──────────────┐              │
│                      │              │              │
│                      │              │              │
│                      │              ▼              │
│                      │        ┌───────────┐        │
│                      │        │           │        │
│                      │        │ SQLServer │        │
│                      │        │           │        │
│                      │        └───────────┘        │
│                      │                             │
└──────────────────────▼─────────────────────────────┘
                       │
                       │
                       │
                       │   roachpb.SpanStatsResponse
                       │
                       │
 Secondary Tenant      │
┌──────────────────────┼──────────────────────────────┐
│                      │                              │
│         ┌────────────▼─────────────┐                │
│         │            │             │                │
│         │   kvtenantccl.Connector  │                │
│         │                          │                │
│         └────────────┬─────────────┘                │
│                      │                              │
│                      │    via TenantStatusServer    │
│                      ├────────────────────┐         │
│                      │                    │         │
│                      │                    │         │
│                      │                    │         │
│    ┌─────────────────▼───────┐     ┌──────▼──────┐  │
│    │                         │     │             │  │
│    │    server.statusServer  │     │  SQLServer  │  │
│    │                         │     │             │  │
│    └─────────────────────────┘     └─────────────┘  │
│                                                     │
│                                                     │
│                                                     │
└─────────────────────────────────────────────────────┘

Part of: https://cockroachlabs.atlassian.net/browse/CRDB-22711
Release note: None

[cockroach-teamcity]

This change is 

[knz]

I am so happy to see this PR. Thank you zach.

As a minor point, would you be open to having this code inside the sql package or one of its sub-packages. This way we can also think about embedding this estimate in SQL functions that the SQL obs team is currently implementing (@THardy98)

Reviewable status: complete! 0 of 0 LGTMs obtained

[zachlite]

would you be open to having this code inside the sql package or one of its sub-packages.

Sure! Pending any feedback or issues with my implementation so far, I'll make this more accessible for other callers.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @zachlite)

---

pkg/server/status.go line 3415 at r1 (raw file):

		return status.SpanStats(ctx, req)
	}
	fetcher := rangestats.NewFetcher(s.db)

Before we move forward with the review, may I suggest you extract this code into a separate function, with a clear API interface.
It should probably take start/end keys as parameters. And return StoreKeySpanStats. Probably something like the same API interface as the original Compute function you've removed.

Inside this function, it would also be good to avoid accumulating the descriptors in an array. You can interleave the iteration with the extraction of the stats below.

[THardy98]

I am so happy to see this PR. Thank you zach.

As a minor point, would you be open to having this code inside the sql package or one of its sub-packages. This way we can also think about embedding this estimate in SQL functions that the SQL obs team is currently implementing (@THardy98)

Reviewable status: complete! 0 of 0 LGTMs obtained

Nice :)

Looking a bit further ahead, it would be really nice to be able to get stats for multiple spans at once - which I understand is outside the scope of the initial issue description:

It is possible that we may want to allow for multiple spans in the same request, but that decision is out of scope for this initial issue statement.

but would be incredibly useful on the SQL Obs side to get stats for many table spans.

Currently, we have to get table stats per table, which means a node fanout for each table span, where we could be dealing with 100s or 1000s of tables. It would be great to issue a single fanout for a collection of table spans, considering that (as I understand it), the actual range stats collection for multiple spans can be done cheaply.

Perhaps not for this PR, but would be really nice in a followup.

[zachlite]

@knz
I've made span stats available for consumption by pkg/sql by putting the logic behind a new spanstats.Accessor interface. The idea is that there will be two implementations of spanstats.Accessor, because access to KV is dependent on the type and location of the tenant.

So far, this PR includes the first implementation: spanstatsaccessor.LocalAccessor. This implementation is meant for consumers of span stats that are co-located on a KV node.
For the second implementation, I'm planning on kvtenantccl.Connector implementing the spanstats.Accessor interface.

And I'm sorry about the diff noise - I had to reclaim the name span_stats_server from my work on the Key Visualizer, so there's a file rename in this PR.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @knz, and @mgartner)

---

pkg/server/status.go line 3415 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Before we move forward with the review, may I suggest you extract this code into a separate function, with a clear API interface.
It should probably take start/end keys as parameters. And return StoreKeySpanStats. Probably something like the same API interface as the original Compute function you've removed.

Inside this function, it would also be good to avoid accumulating the descriptors in an array. You can interleave the iteration with the extraction of the stats below.

ack.

[knz]

Okay this PR is in a much better shape now. Thanks for the investment.
The main remaining topic I'd like to cover is the selection of node IDs.

In the code you're replacing, we have this function nodeIDsAndRangeCountForSpan() in pkg/server which is used by (s *adminServer) statsForSpan() to avoid going to every node to ask about stats for just a few ranges.

Your solution as currently implemented goes to talk to every node in every case. That seems a bit expensive.
Is there a way to be more intelligent here, by discovering which nodes have ranges for the span under consideration, and only calling InternalSpanStats on these nodes?

Reviewed 2 of 7 files at r1, 20 of 20 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @zachlite)

---

-- commits line 2 at r2:
Please massage this commit title / message to give it more structure.

---

pkg/kv/kvclient/spanstats/span_stats.go line 23 at r2 (raw file):

// boundary.
type Accessor interface {

nit: remove empty line.

---

pkg/kv/kvclient/spanstats/spanstatsaccessor/accessor.go line 27 at r2 (raw file):

	endKey roachpb.Key, nodeID roachpb.NodeID,
) (*serverpb.InternalSpanStatsResponse, error) {

nit: ditto

---

pkg/server/key_visualizer_server.go line 48 at r2 (raw file):

	ctx context.Context, req *keyvispb.UpdateBoundariesRequest,
) error {

nit: ditto

---

pkg/server/key_visualizer_server.go line 50 at r2 (raw file):

	encoded, err := protoutil.Marshal(req)

nit: remove empty line here too.

---

pkg/server/key_visualizer_server.go line 117 at r2 (raw file):

		"iterating nodes for key visualizer samples", dialFn, nodeFn,
		responseFn, errorFn)

ditto

---

pkg/server/key_visualizer_server.go line 140 at r2 (raw file):

// spans.
func verifySampleBoundariesEqual(fragments []keyvispb.Sample) bool {

ditto

---

pkg/server/server.go line 847 at r2 (raw file):

	// Instantiate the span stats server. There is a circular dependency
	// between server.spanStatsServer and server.systemStatusServer.
	spanStats := &spanStatsServer{

Can you coordinate with @dhartunian to double check how this is meant to work for secondary tenant servers.

---

pkg/server/serverpb/span_stats.proto line 43 at r2 (raw file):

service SpanStats {
  rpc GetSpanStats(InternalSpanStatsRequest) returns (InternalSpanStatsResponse) {}
}

nit: missing newline at end of file.

---

pkg/kv/kvserver/client_status_test.go line 81 at r2 (raw file):

		_ = tcase
		t.Errorf("implement me")
		//start, end := tcase.startKey, tcase.endKey

Don't forget to do this.

[zachlite]

Thanks for the feedback. I'll take a look.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @mgartner)

[zachlite]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dhartunian, @knz, and @mgartner)

---

pkg/kv/kvclient/spanstats/span_stats.go line 23 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: remove empty line.

Done.

---

pkg/kv/kvclient/spanstats/spanstatsaccessor/accessor.go line 27 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: ditto

Done.

---

pkg/server/key_visualizer_server.go line 48 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: ditto

Done.

---

pkg/server/key_visualizer_server.go line 50 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: remove empty line here too.

Done.

---

pkg/server/key_visualizer_server.go line 117 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

ditto

Done.

---

pkg/server/key_visualizer_server.go line 140 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

ditto

Done.

---

pkg/server/server.go line 847 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Can you coordinate with @dhartunian to double check how this is meant to work for secondary tenant servers.

👍

---

pkg/server/serverpb/span_stats.proto line 43 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: missing newline at end of file.

Done.

---

pkg/kv/kvserver/client_status_test.go line 81 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Don't forget to do this.

Done.

[knz]

nit: i've renamed the PR: the work is about enabling coalesced ranges and is not specific to multi-tenancy. (Coalesced ranges are pretty much necessary to CC serverless, but they would be useful to everyone else too, but we can't enable them until the issue you're working on here is fixed.)

Beyond that, this mostly looks good. I'm generally happy with where you brought this. Let's be extra careful about the authorization rule though.

After that, let's also be mindful that the folk who need to sign off on your approach (because they maintain this API infrastructure) are our obs-inf coworkers. So please go through the remaining review iterations with them as well.

Reviewed 23 of 27 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @zachlite)

---

-- commits line 2 at r4:
NB: the first line in a commit message has a special role, it is called the "commit title" and shows up in many places - the git log summary, patch headers, etc.

So it needs to be structured like a title. See this for guidance:

https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages#Commit-title

---

-- commits line 8 at r4:
Thanks for enumerating the 3 components.
For each of them, can you also sketch what client components use the new component and where in the source code this happens.

Generally, it would be useful to understand by reading the commit message which go interface / components you've connected to each other to make it work.

---

pkg/rpc/auth_tenant.go line 119 at r4 (raw file):

	case "/cockroach.server.serverpb.Status/SpanStats":
		return a.authTenant(tenID)

That won't do. You need to extend the verification to the start/end keys in the request, that they are properly contained inside the tenant keyspace.
See authRangeFeed for an example.