cockroachdb / cockroachdb/cockroach

obs/ash: add store_id to ASH samples

Open
#171,305 0 comments 0 reactions 0 assignees View on GitHub
A-cluster-observability C-enhancement O-agent T-observability
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Is your feature request related to a problem? Please describe.**

Active Session History (ASH) samples currently record `node_id` but not `store_id`. For KV-layer work events (e.g. `KVEval`, `ReplicaSend`, `LatchWait`, `LockWait`, `RaftProposalWait`), knowing which store the work occurred on would help attribute load and contention to specific stores — useful on multi-store nodes where a single `node_id` is too coarse.

**Describe the solution you'd like**

Add a `store_id` column to ASH samples, populated for KV-server instrumentation points that operate on a replica.

**Important:** this is *not* a simple parallel of `node_id`. `node_id` is a property of the process-wide sampler singleton — it's set once at init ([`sampler.go:166`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/obs/ash/sampler.go#L166)) and stamped unconditionally on every sample ([`sampler.go:307`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/obs/ash/sampler.go#L307)). A store sits *below* a node (one node can host multiple stores), so `store_id` is a property of the per-goroutine `WorkState`, not the sampler, and is only meaningful for a subset of instrumentation points.

Of the 24 instrumentation points ([`ash_instrumentation_points.txt`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/obs/ash/ash_instrumentation_points.txt)):
- **Have a store** (all `pkg/kv/kvserver/...`, run on a `Replica`): `KVEval`, `ReplicaSend`, `LatchWait`, `LockWait`, `RaftProposalWait`, `LeaseAcquisition`, `Backpressure`, `CommitWaitSleep`, `TxnPushWait`, `TxnQueryWait`, `TenantRateLimit`, `ReplicationFlowControl`.
- **No store concept** (`store_id` would be NULL): `Optimize`, `DistSenderLocal`/`DistSenderRemote`, `OutboxSend`/`InboxRecv`, `BatchFlowCoordinator`, `ColExecSync`, execinfra processors, admission `work_queue`.

So unlike `node_id` (always populated), `store_id` would be **sparsely populated**.

**Work involved:**

_Mechanical / output-side (mirrors `node_id`, ~easy):_
- [ ] Add `store_id` to the `ASHSample` proto ([`status.proto:1221`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/server/serverpb/status.proto#L1221)) and Go struct ([`types.go:67`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/obs/ash/types.go#L67))
- [ ] Map it in the RPC handler (`ListLocalActiveSessionHistory` in `pkg/server/status.go`)
- [ ] Add the column + `addRow` value to both virtual tables ([`crdb_internal.go:10000`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/sql/crdb_internal.go#L10000), [`crdb_internal.go:10061`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/sql/crdb_internal.go#L10061)) and the two `information_schema` views

_Input-side (the real work):_
- [ ] Add a `StoreID roachpb.StoreID` field to `WorkloadInfo` ([`types.go:50`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/obs/ash/types.go#L50)), defaulting to 0 (unset). This **avoids any `SetWorkState` signature change** — existing call sites are untouched; only KV call sites add `StoreID: ...`. The value flows automatically via the existing `state.WorkloadInfo = info` assignment in `SetWorkState`.
- [ ] At each KV call site, fetch the store ID from the `Replica` and set it in the `WorkloadInfo` literal (e.g. [`replica_send.go:114`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/kv/kvserver/replica_send.go#L114))
- [ ] Copy `StoreID` into the emitted `ASHSample` in `takeSample` ([`sampler.go:305`](https://github.com/cockroachdb/cockroach/blob/b4c850ad69766fc7f9acdda105e35180a7f66713/pkg/obs/ash/sampler.go#L305))
- [ ] Decide semantics for no-store sites (leave 0, surface as NULL)

**Describe alternatives you've considered**

- _Status quo:_ leaving attribution at `node_id` granularity. Adequate for single-store nodes but loses information on multi-store deployments.
- _Dedicated `WorkState.StoreID` field:_ semantically cleaner than adding to `WorkloadInfo` (which otherwise groups workload-*identity* fields), but requires a new `SetWorkState` parameter, changing all ~24 call sites. The `WorkloadInfo` route is preferred for minimal churn.

**Additional context**

Effort is dominated by populating `store_id` at the KV-server call sites, not by the proto/table changes. The output-side plumbing is a direct copy of the existing `node_id` pattern.

Jira issue: CRDB-64435

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.