rfc: query SSTable metrics

[raggar]

Draft RFC for querying SSTable metrics.

Related issue: #102604
Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

Nice!

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

---

docs/RFCS/query_sst_metrics.md line 10 at r1 (raw file):

# Summary

Looking to add the ability for users to query sstable metrics via their CockroachDB shell. This will be implemented using a set-genearting function (SRF) and will be used as follows

[nit] it's not really meant for users, more for us / operators to debug issues. I'd add that the sstable metrics are useful to debug storage issues pertaining to a specific key range.

[nit] generating

---

docs/RFCS/query_sst_metrics.md line 13 at r1 (raw file):

SELECT crdb_internal.engine_stats('start-key', 'end-key')

Let's do SELECT * FROM ... which outputs the columns like a table

---

docs/RFCS/query_sst_metrics.md line 17 at r1 (raw file):

or 

SELECT crdb_internal.engine_stats('start-key', 'end-key', store-id)

Let's do engine_stats(node-id, store-id, 'start-key', 'end-key') for consistency with the compaction function

---

docs/RFCS/query_sst_metrics.md line 27 at r1 (raw file):

## Cockroach Side

The proposed solution is creating a new set-generating function (SRF) which will be added in [built-in generator](https://github.com/cockroachdb/cockroach/blob/3526c9ca65a94bd751b23a65b3c96a1513c961bc/pkg/sql/sem/builtins/generator_builtins.go#L107) will need to be added. This SRF will need to send an RPC to each node in order to retrieve all the relevant SSTables. This can be achieved by calling [Dial](https://github.com/cockroachdb/cockroach/blob/7d8e56533549abedd7ceceeafc469ca4e224e4ed/pkg/rpc/nodedialer/nodedialer.go#L101) for each seperate node inside of a function that is part of `evalCtx`. This function will call out to the [StorageEngineClient](https://github.com/cockroachdb/cockroach/blob/213da1f9fb591d90dcea6590b31da8c55b0756f9/pkg/kv/kvserver/storage_engine_client.go#LL23) and be handled in the [stores server](https://github.com/cockroachdb/cockroach/blob/5b6302b2ed2a83f49b55329ce3cac5f6135d0aea/pkg/kv/kvserver/stores_server.go#L24) (see Pebble side). 

[nit] break up the lines (preferably at 100 columns)

[nit] we will be creating two overloads, for the two variants above. The latter one only talks to one node
[nit] separate

---

docs/RFCS/query_sst_metrics.md line 29 at r1 (raw file):

The proposed solution is creating a new set-generating function (SRF) which will be added in [built-in generator](https://github.com/cockroachdb/cockroach/blob/3526c9ca65a94bd751b23a65b3c96a1513c961bc/pkg/sql/sem/builtins/generator_builtins.go#L107) will need to be added. This SRF will need to send an RPC to each node in order to retrieve all the relevant SSTables. This can be achieved by calling [Dial](https://github.com/cockroachdb/cockroach/blob/7d8e56533549abedd7ceceeafc469ca4e224e4ed/pkg/rpc/nodedialer/nodedialer.go#L101) for each seperate node inside of a function that is part of `evalCtx`. This function will call out to the [StorageEngineClient](https://github.com/cockroachdb/cockroach/blob/213da1f9fb591d90dcea6590b31da8c55b0756f9/pkg/kv/kvserver/storage_engine_client.go#LL23) and be handled in the [stores server](https://github.com/cockroachdb/cockroach/blob/5b6302b2ed2a83f49b55329ce3cac5f6135d0aea/pkg/kv/kvserver/stores_server.go#L24) (see Pebble side). 

The SRF will be structured similar to [json_populate_record](https://github.com/cockroachdb/cockroach/blob/f97d24aa661d8e1561f27a740325ebdabd62c926/pkg/sql/sem/builtins/generator_builtins.go#L383-L385) using a generator to return each output row. All necessary data should be available on `FileMetadata`.

[nit] the FileMetadata comment isn't accurate and doesn't belong here (FileMetadata is internal to Pebble) - at this level, we'd use whatever DB.SSTables() returns.

---

docs/RFCS/query_sst_metrics.md line 33 at r1 (raw file):

## Pebble Side

Inside the store's server code is where Pebble will be used, specifically [DB.SSTables](https://github.com/cockroachdb/pebble/blob/25a8e9bb8d9586e5090979f24dec11712e9f4b3c/db.go#L1912). When calling `DB.SSTables` we will need to specify a `SSTableOption` which will be a function allowing us to filter SSTables for the key range specified by the user. Note for this step, we will use `FileMetadata` instead of `getTableProperties` to avoid reading metadata from storage and putting it in a cache. 

[nit] I'd rephrase the last sentence along the lines of "Note that the filtering can be performed based on the FileMetadata alone, allowing us to skip unnecessary getTableProperties calls (which can read metadata from storage and affect caches)."

[RaduBerinde]

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

---

docs/RFCS/query_sst_metrics.md line 35 at r4 (raw file):

Inside the store's server code is where Pebble will be used, specifically [DB.SSTables](https://github.com/cockroachdb/pebble/blob/25a8e9bb8d9586e5090979f24dec11712e9f4b3c/db.go#L1912). When calling `DB.SSTables` we will need to specify a `SSTableOption` which will be a function allowing us to filter SSTables for the key range specified by the user. Note that filtering can be performed based on the `FileMetadata` alone, which allows us to skip unnecessary `getTableProperties` calls (which can read metadata from storage and affect caches).

## General Steps 

[nit] implementation steps

---

docs/RFCS/query_sst_metrics.md line 51 at r4 (raw file):

- node_id 
- store_id 
- file_num

Question for other reviewrs - do we want level as a separate column maybe? Anything else? Note that the json props will contain all this information too.

[jbowens]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nicktrav, @RaduBerinde, and @RahulAggarwal1016)

---

docs/RFCS/query_sst_metrics.md line 27 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] break up the lines (preferably at 100 columns)

[nit] we will be creating two overloads, for the two variants above. The latter one only talks to one node
[nit] separate

nit: I think the comment about word wrapping the lines at 100 columns still stands; here and throughout.

---

docs/RFCS/query_sst_metrics.md line 51 at r4 (raw file):

Previously, RaduBerinde wrote…

Question for other reviewrs - do we want level as a separate column maybe? Anything else? Note that the json props will contain all this information too.

Yeah, I think elevating level to a column makes sense.

---

docs/RFCS/query_sst_metrics.md line 12 at r6 (raw file):

SELECT * FROM crdb_internal.engine_stats('start-key', 'end-key')

should we name this function sstable_metrics or engine_sstables or the like? I think something to indicate that it specifically is returning sstables would be appropriate.

---

docs/RFCS/query_sst_metrics.md line 31 at r6 (raw file):

## Pebble Side

Inside the store's server code is where Pebble will be used, specifically [DB.SSTables](https://github.com/cockroachdb/pebble/blob/25a8e9bb8d9586e5090979f24dec11712e9f4b3c/db.go#L1912). When calling `DB.SSTables` we will need to specify a `SSTableOption` which will be a function allowing us to filter SSTables for the key range specified by the user. Note that filtering can be performed based on the `FileMetadata` alone, which allows us to skip unnecessary `getTableProperties` calls (which can read metadata from storage and affect caches).

It looks like SSTableInfo doesn't contain any sublevel information for L0. I think we should also add a Sublevel field and only populate it if the file is contained within L0.

---

docs/RFCS/query_sst_metrics.md line 44 at r6 (raw file):

Audience: all participants to the RFC review.

1. How will we get a list of all the nodes we will need to send an RPC request to (in the case the user does not specify the node-id).

I think we should dig into (and document answers to) this question some more. In the node-constrained builtin we're taking the key range in terms of low-level storage keys to ensure that we can query sstables overlapping with the local key range. What does it mean if the user provides a local key range to the fan-out variant? Every node has a local key range, so does it require fanning out to all nodes? We could restrict this variant to the global keyspan, but I think it's definitely possible we'll find ourselves missing this ability in practice. I suggest we require the user provided keyspan fall entirely within the local keyspan or entirely within the global keyspan. If it falls within the local keyspan, we fan out to all nodes. If it falls within the global keyspan, we fan out only to nodes containing overlapping replicas.

When the user-provided key range is in the global keyspan, we'll need to map the user-provided key range to the set of KV ranges overlapping that range, and from there find which stores hold replicas of the resulting KV ranges. I'm not sure mechanically how to do this. I think the crdb_internal.tenant_span_stats built-in must do something similar and might be a good place to dig in further.

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @nicktrav, and @RahulAggarwal1016)

---

docs/RFCS/query_sst_metrics.md line 12 at r6 (raw file):

Previously, jbowens (Jackson Owens) wrote…

should we name this function sstable_metrics or engine_sstables or the like? I think something to indicate that it specifically is returning sstables would be appropriate.

I like sstable_metrics

---

docs/RFCS/query_sst_metrics.md line 44 at r6 (raw file):

we fan out only to nodes containing overlapping replicas.

Is that information (which nodes have overlapping replicas) easy to obtain from the relevant code paths? I think it would be ok - at least to start with - if we always fan out to all nodes. It's a manual operation, I don't think it's a big deal if it's not super efficient.

I don't see a problem with restricting the key range to be either local or global (though if we always fan out to all nodes I think it doesn't matter?)

[RaduBerinde]

I'm ok with merging this and editing it later when we figure out more details about the multi-node version.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @nicktrav, and @RahulAggarwal1016)

---

docs/RFCS/query_sst_metrics.md line 51 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Yeah, I think elevating level to a column makes sense.

Let's move these under Technical Design so it's no longer an open question. And let's move level before file_num

---

docs/RFCS/query_sst_metrics.md line 16 at r7 (raw file):

range. This will be implemented using a set-generating function (SRF) and used as follows.

``` SELECT * FROM crdb_internal.sstable_metrics('start-key', 'end-key')

These quotes aren't working as intended, we should wrap each SELECT in its own quotes, and the quotes need to be on their own lines.

BTW you can see the formatted output in the github PR, under Files, click the dots menu for the file and "View"

---

docs/RFCS/query_sst_metrics.md line 67 at r7 (raw file):

1. How will we get a list of all the nodes we will need to send an RPC request to (in the case the
user does not specify the node-id).

Add an item here around local vs global key spans for the multi-node version. One option is to only support global keys for that variant (at least at first).

Local vs global shouldn't be a problem for the single-store version I don't think.

---

docs/RFCS/query_sst_metrics.md line 69 at r7 (raw file):

2. What columns do we want to display?
- node_id 

[supernit] there are a lot of spaces before newlines in the document

[RaduBerinde]

but let's wait for Jackson and/or Nick to approve as well.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens, @nicktrav, and @RahulAggarwal1016)

---

docs/RFCS/query_sst_metrics.md line 55 at r8 (raw file):

- level
- file_num
- json_props

[nit] let's do metrics so it's consistent with the function name

[jbowens]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @nicktrav and @RahulAggarwal1016)

[raggar]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[yuzefovich]

nit: this is the only RFC that doesn't have a date prepended to the file name in the RFC folder.