Skip to content

Commit 0b89144

Browse files
antiguruclaude
andcommitted
Address review feedback and guard reference metric names
Merge each reference's Hazards and Invariants into a single section. The split produced near-verbatim duplication in every reference, between three and five entries each, because almost every hazard is an invariant with a measurement attached. Each entry now leads with the property that holds at any fleet size and gives the measurement inline. Correct the compute reference's response queue row. It claimed mz_compute_controller_response_queue_size exists directly, but no metric! invocation registers that name and Prometheus carries no such series. The response channel is an instrumented_unbounded_channel, which takes a send and a receive counter and exports no depth gauge, so the difference of the two is the only way to read depth. Record the counter-reset hazard that follows, since both counters reset when a pod is replaced. Add scripts/, holding the batched-query builder that step 5 needs and the panel-query slicer that step 4 refers to. Guard the references against metric renames from two directions. At run time, catalog-diff.sh reads doc/user/data/metrics.yml at the release tags on either side of a sign-off boundary, which separates an empty panel that is a finding from one that is a metric arriving or departing with the release. At commit time, ci/test/lint-skill-metrics.sh resolves every mz_* name in the skill against the catalog, glob patterns included, and fails on any that neither resolves nor carries a documented exemption in metrics-allowlist.txt. It also fails on an exemption that has started resolving, so entries cannot outlive their reason. Record three dead dashboard panels found while building the lint. mz_query_latency was added in #22049 and deleted in #26647 along with the stash, and environmentd-health still plots its _bucket family. mz_persist_columnar_validation_count and mz_txn_placeholder_schema_apply have never appeared in this repository, yet both are arms of the persist dashboard's `should be small` panel, so two of its sixteen series are permanent zeros that make the panel read healthier than it is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0a1b6d1 commit 0b89144

14 files changed

Lines changed: 596 additions & 71 deletions

.agents/skills/mz-release-signoff/SKILL.md

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,8 @@ The panel titles are cheap and give the row structure. The panel queries are not
124124

125125
Row panels appear in the title list but not in the query list, so the two are offset. Match them by title, not by index.
126126

127+
`scripts/panel-metrics.py` does the slicing. Take the roster from the dashboard, but resolve every name against the metrics catalog for the release under test before trusting it, as described in *When a metric has gone missing* below. A dashboard panel outlives the metric it plots, so the panel list is a superset of what the build actually exports.
128+
127129
## Step 5: Measure
128130

129131
Batch many metrics into one range query by tagging each aggregate with a synthetic label and combining with `or`. This turns twenty tool calls into one:
@@ -135,6 +137,8 @@ or label_replace(sum(avg_over_time(<gauge>[6h])), "m", "2_name", "", "")
135137

136138
Prefix the tags so the result order is stable and readable. Use `rate(x[6h])` for counters and `avg_over_time(x[6h])` for gauges, both matching the step so buckets do not overlap.
137139

140+
`scripts/build-range-query.py` writes these expressions from a metric list, because twenty of them by hand is where typos live. It also encodes the one thing that differs between stacks, namely the staging release-candidate join against the pinned production canary namespaces, so the same roster can be run against either without rewriting the selector.
141+
138142
Run each area twice. Once across all clusters, and once restricted to the system clusters, which the panel instructions call out because a system-cluster regression is easy to lose in the noise of user clusters. System clusters are `instance_id=~"s[0-9]+"` for the controller and replica metrics, and `pod=~".*cluster-s[0-9]+-replica-.*"` for the container metrics.
139143

140144
## Step 6: Judge
@@ -220,14 +224,36 @@ To characterize a new dashboard, or to refresh one, run Steps 1 through 6 agains
220224

221225
* Each metric with its type, the labels that select cluster and replica, and what it means.
222226
* Which metrics are bimodal, restart-sensitive, or absent when zero.
223-
* Invariants: relationships that hold at any fleet size, such as one counter equalling the difference of two others, a gauge whose only meaningful aggregate is a series count, or a metric that is structurally absent in one stack.
227+
* Hazards and invariants, in one section rather than two. Each entry leads with the property that holds at any fleet size, such as one counter equalling the difference of two others, a gauge whose only meaningful aggregate is a series count, or a metric that is structurally absent in one stack, and then gives the measurement it came from. Splitting these across two sections produced near-verbatim duplication in every reference, because almost every hazard is an invariant with a number attached.
224228
* Label naming inconsistencies, duplicate-series hazards, and any panel expression whose filters are not what they appear to be.
225229
* Known noise classes, meaning the environments that are unhealthy independently of any release and whose flat contribution can dominate a fleet aggregate.
226230

227231
Record invariants, not levels. A recorded level is stale the week after it is written, because environments are created, deleted, and resized continuously, and a stale reference value is worse than none: it invites a comparison the reader should not make. The comparison that matters is always derived in-run, since the before-window of your own query is the only baseline guaranteed to describe the same fleet as the after-window.
228232

229233
Coarse order-of-magnitude figures are worth keeping for one narrow purpose: catching a mis-scoped selector, for example a missing `container="clusterd"` that inflates a result tenfold. Keep them dated, keep them to one significant figure, and say plainly that they are not for comparison.
230234

235+
## When a metric has gone missing
236+
237+
A reference that names a metric the build no longer exports is worse than no reference, because the sweep reads the empty result as a healthy zero. Two things make this checkable, and both have to be applied to the right version.
238+
239+
`doc/user/data/metrics.yml` is the generated catalog of `metric!` invocations in the Rust tree. It is checked in, so it is tagged along with each release, and the working tree's copy describes `main`, which is ahead of whatever you are verifying. Reading that copy during a sign-off answers a question you did not ask. Read the catalog at the two release tags instead and diff them:
240+
241+
```
242+
scripts/catalog-diff.sh v26.38.0 v26.39.0-rc.3
243+
```
244+
245+
That turns "is this empty panel a regression or a rename?" into a lookup. A name in the removed list explains an after-window that went empty, and it is a documentation fix rather than a finding. A name in the added list explains an empty before-window, and comparing across the boundary on it is meaningless.
246+
247+
**Catalog membership is not a string match.** A `metric!` whose name is built with `format!` is catalogued with its placeholders globbed, so `mz_persist_user_bytes` and `mz_persist_compaction_goodbytes` are both covered by the single entry `mz_persist_*_bytes`, and histograms are catalogued as their expanded `_bucket`, `_count`, and `_sum` families rather than under the base name. Grepping the catalog for a literal name will therefore report a live metric as missing. Match against the patterns, as `scripts/lint_metrics.py` does.
248+
249+
**The catalog covers only this repository's Rust tree.** It holds no `v2_mz_*`, which the promsql exporter derives from SQL, no `container_*`, `kube_*`, or `kubelet_*` from cAdvisor and kube-state-metrics, and no cloud-side names such as `mz_envd_up` or `mz_external_*`. Those families are confirmed against Prometheus with `list_prometheus_metric_names`, and they are where the sweep has actually been bitten: the compute dashboard's scratch-disk panels plot `kubelet_volume_stats_used_bytes`, which resolves to five series in a single namespace in both production and staging us-east-1 and none of them a `clusterd` volume, so those panels render blank in both stacks.
250+
251+
The diff survives that blind spot, because both sides share it and a dynamically named metric is missing from both catalogs and so never appears in either list. The diff therefore never reports a rename that did not happen. It can still miss one, so an empty result that the diff does not explain is not yet cleared. Confirm it against Prometheus with `list_prometheus_metric_names` before reporting the metric as zero.
252+
253+
When a name resolves in neither the catalog nor Prometheus, the panel that plots it is dead, and the reference should record that rather than the metric. `git log -S<name> --all -- src/` settles which kind of dead it is, and the two kinds read differently in a report. `mz_query_latency` was real, added in #22049 and deleted in #26647 along with the stash, so the `environmentd-health` panel that still plots its `_bucket` family has been empty since that deletion. `mz_persist_columnar_validation_count` and `mz_txn_placeholder_schema_apply` have never appeared in this repository at all, yet both are live arms of the persist dashboard's `should be small` panel, and the working spelling of the first sits beside it on the same panel as `mz_persist_columnar_op_count` with `op="validation"` and `result="invalid"`.
254+
255+
`ci/test/lint-skill-metrics.sh` guards the other direction, so the references cannot rot silently between releases. It resolves every `mz_*` name in this skill against the working tree's catalog, patterns included, and fails on any that neither resolves nor appears in `scripts/metrics-allowlist.txt`. The allowlist is the point: it carries one line per name the catalog does not cover, with the reason, so adding to it is a deliberate act and a genuinely renamed metric still fails. It also fails on an allowlist entry that has started resolving, so the exemptions cannot outlive their reason. A failure is an instruction to update the skill, never to suppress the lint.
256+
231257
## Traps
232258

233259
**A gauge divided by a limit can fail on duplicate series.** `container_spec_memory_limit_bytes` is exported once per node-label set, and labels such as `karpenter_sh_initialized` flip during node lifecycle, which yields two series for one pod and a `many-to-many matching not allowed` error. Collapse both sides first:

.agents/skills/mz-release-signoff/references/adapter.md

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,26 +66,27 @@ These render empty and are not evidence of a healthy system. Verified against th
6666
* `Stash (CRDB) Query Latencies` reads `mz_query_latency_bucket`, which no longer exists. The catalog moved off the stash, so the panel has no replacement.
6767
* `Swap Usage (bytes)` plots `container_spec_swap_limit_bytes` as its limit series. The real name is `container_spec_memory_swap_limit_bytes`. The usage series is fine.
6868

69-
## Hazards
69+
## Hazards and invariants
70+
71+
Each entry states a property that holds at any fleet size, followed by the measurement it came from. The property is what survives a release. The measurement is dated, describes whatever fleet existed when it was taken, and is recorded only so the property is not mistaken for a guess.
7072

7173
**Two p99 panels are pinned by bucket resolution.** `p99 Slow Coordinator Messages` reported 0.0001276 s across eight days in staging with five-digit stability, and `Coordinator Table Append Latencies` p99 reported 0.12673 s in production with the same rigidity, both while the underlying counters advanced normally. The quantile is landing inside one wide bucket, so interpolation returns the bucket boundary and the panel cannot move. Read `rate(_sum) / rate(_count)` instead for these two, and treat an implausibly constant quantile anywhere as a bucket artifact rather than as stability.
7274

73-
**Snapshot latency is NaN except around restarts.** `mz_catalog_snapshot_latency_seconds` only records at boot, so an average over a steady-state window divides by zero. Non-NaN values in an upgrade bucket are the expected case, not a finding.
75+
**`v2_mz_envd_up` should equal the environment count.** It is the cheapest liveness check on the dashboard.
76+
77+
**`environmentd` does not swap.** Swap usage measured exactly zero across the whole window in production canary, so a non-zero reading is itself the finding.
78+
79+
**Catalog collection entries grow monotonically with catalog contents.** A fall needs explaining.
80+
81+
**Catalog snapshot latency is recorded at boot only.** `mz_catalog_snapshot_latency_seconds` records once at boot, so an average over a steady-state window divides by zero and reads NaN, while an upgrade bucket reads non-NaN. Neither is a finding.
7482

75-
**`environmentd` does not swap.** Swap usage measured exactly zero across the whole window in production canary. A non-zero value there is itself the finding.
83+
**Serializable and strict serializable time-to-first-row are not comparable.** In production canary the p99 ran about 1.2 s for strict serializable and about 6.5 s for serializable over the same window, roughly a factor of five. Never aggregate across the `isolation_level` label, and compare each against its own history.
7684

77-
**Serializable and strict serializable time-to-first-row are not comparable.** In production canary the p99 ran about 1.2 s for strict serializable and about 6.5 s for serializable over the same window. Compare each against its own history.
85+
**System queries outnumber user queries by an order of magnitude.** The gap is far larger in staging, where most environments are idle apart from introspection, so staging user-query numbers are not a workload signal.
7886

79-
## Invariants
87+
**Coordinator busy time is the best single coordinator-load signal.** It is the `_sum` rate of `mz_slow_message_handling`. The message rate alone hides how expensive each message was.
8088

81-
* `v2_mz_envd_up` should equal the environment count. It is the cheapest liveness check on the dashboard.
82-
* `environmentd` does not swap. A non-zero swap reading is itself the finding.
83-
* Catalog collection entries grow monotonically with catalog contents. A fall needs explaining.
84-
* Catalog snapshot latency is recorded at boot only, so it is NaN over a steady-state window and non-NaN in an upgrade bucket. Neither is a finding.
85-
* Serializable and strict serializable time-to-first-row differ by roughly a factor of five. Never aggregate across the `isolation_level` label, and compare each against its own history.
86-
* System queries outnumber user queries by an order of magnitude, and by far more in staging, where most environments are idle apart from introspection. Staging user-query numbers are not a workload signal.
87-
* Coordinator busy time, the `_sum` rate of `mz_slow_message_handling`, is the best single coordinator-load signal, because the message rate alone hides how expensive each message was.
88-
* `crdb_dedicated_*` metrics describe the whole regional CockroachDB cluster and cannot be attributed to the release under test.
89+
**`crdb_dedicated_*` metrics describe the whole regional CockroachDB cluster.** They cannot be attributed to the release under test.
8990

9091
## Order of magnitude
9192

.agents/skills/mz-release-signoff/references/compute.md

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ System clusters are `s` followed by digits in either family, and `.*cluster-s[0-
4545
|---|---|---|
4646
| `mz_compute_controller_replica_count`, `_collection_count`, `_peek_count`, `_subscribe_count` | gauge | Controller-side inventory. Collection count tracks catalog growth, so a slow rise is expected. |
4747
| `mz_compute_controller_command_queue_size` | gauge | Sustained depth means the controller is not draining. |
48-
| `mz_compute_controller_response_queue_size` | gauge | Exists directly, but the cluster-level panel instead computes `mz_compute_controller_response_send_count - mz_compute_controller_response_recv_count`. |
48+
| `mz_compute_controller_response_send_count`, `_recv_count` | counter | Response queue depth, as the difference of the two. There is no depth gauge, because the response channel is an `instrumented_unbounded_channel` (`src/ore/src/channel.rs`), which takes a send and a receive counter and exports nothing else. Contrast `command_queue_size`, a real gauge that the command path increments and decrements directly. |
4949
| `mz_compute_commands_total`, `mz_compute_responses_total` | counter | Protocol volume. Doubles for one bucket during a zero-downtime upgrade. |
5050
| `mz_compute_command_message_bytes_total`, `mz_compute_response_message_bytes_total` | counter | Protocol bytes. Worth checking when a change touches command encoding. |
5151
| `mz_compute_controller_history_command_count`, `_history_dataflow_count` | gauge | Controller-side command history, which should be reduced and not grow without bound. |
@@ -63,28 +63,25 @@ System clusters are `s` followed by digits in either family, and `.*cluster-s[0-
6363
| `mz_dataflow_replica_expiration_timestamp_seconds`, `_expiration_remaining_seconds` | gauge | Replica expiration. Panels filter `> 0` and `!= 0` because the metric is exported as zero when unset. |
6464
| `mz_subscribe_snapshots_skipped_total` | counter | Subscribe snapshot optimization hit rate. The panel appends `> 0` to hide inactive replicas. |
6565

66-
## Hazards
66+
## Hazards and invariants
6767

68-
**Arrangement gauges are bimodal.** `v2_mz_arrangement_record_count` and `v2_mz_arrangement_size_bytes` swing by a factor of three to ten as periodic dataflows rebuild. In August 2026 staging us-east-1 alternated between 0.72e9 and 2.86e9 records with no release involvement. Compare low state against low state.
68+
Each entry states a property that holds at any fleet size, followed by the measurement it came from. The property is what survives a release. The measurement is dated, describes whatever fleet existed when it was taken, and is recorded only so the property is not mistaken for a guess.
6969

70-
**Peak resident set and swap are restart-sensitive.** Both reset or decay at an upgrade, so a level drop across the boundary is the restart and not the release.
70+
**OOM kills, dataflow errors, and orphan dataflows are absent rather than zero when healthy.** Production canary returned no series at all for `v2_mz_dataflow_error_count` and `v2_mz_orphan_dataflow_count` across a full week. That is the healthy case, and it is indistinguishable from a renamed metric unless the metric is confirmed to exist elsewhere.
7171

72-
**Working set falls at every upgrade.** In prod canary us-east-1 the sum fell from about 290 GB to about 236 GB at the v26.37.0 rollout with no version change in the code that mattered, purely because arrangements were rebuilt fresh. Judge memory by the slope within a release, not the step across one.
72+
**`v2_mz_orphan_dataflow_count` above zero is always a bug, never a load effect.**
7373

74-
**Error and orphan gauges are absent when zero.** Prod canary returned no series at all for `v2_mz_dataflow_error_count` and `v2_mz_orphan_dataflow_count` across a full week. That is the healthy case, and it is indistinguishable from a renamed metric unless the metric is confirmed to exist elsewhere.
74+
**Working set falls at every upgrade.** In production canary us-east-1 the sum fell from about 290 GB to about 236 GB at the v26.37.0 rollout with no change in the code that mattered, purely because arrangements were rebuilt fresh. Judge memory by the slope within a release, not the step across one.
7575

76-
**Arrangement maintenance ramps after a restart.** Measured at 0.020 s/s one day after an upgrade and 0.030 s/s three days later on the same release, so an apparent increase across a boundary can be nothing more than a difference in age.
76+
**Arrangement gauges are bimodal.** `v2_mz_arrangement_record_count` and `v2_mz_arrangement_size_bytes` swing by a factor of three to ten as periodic dataflows rebuild. In August 2026 staging us-east-1 alternated between 0.72e9 and 2.86e9 records with no release involvement. Compare low state against low state, because spike heights are not comparable.
77+
78+
**Peak resident set and swap are restart-sensitive.** Both reset or decay at an upgrade, so a level drop across the boundary is the restart and not the release. Peak resident set therefore describes the current generation only.
7779

78-
## Invariants
80+
**Arrangement maintenance ramps after a restart.** Measured at 0.020 s/s one day after an upgrade and 0.030 s/s three days later on the same release, so an apparent increase across a boundary can be nothing more than a difference in age.
7981

80-
These hold at any fleet size, so they survive product change in a way that a recorded level does not.
82+
**Compute time and park time are complementary.** A CPU rise with a park fall localizes new work to the dataflow loop, while a CPU rise with park flat points outside it.
8183

82-
* OOM kills, dataflow errors, and orphan dataflows are absent rather than zero when healthy. Confirm the metric exists somewhere in the window before reporting zero.
83-
* `v2_mz_orphan_dataflow_count` above zero is always a bug, never a load effect.
84-
* Working set falls at every upgrade because arrangements are rebuilt, so judge memory by its slope within a release rather than by the step across the boundary.
85-
* Arrangement record and size gauges are bimodal, so only their base level is comparable.
86-
* Peak resident set resets on restart, so it describes the current generation only.
87-
* Compute time and park time are complementary. A CPU rise with a park fall localizes new work to the dataflow loop, while a CPU rise with park flat points outside it.
84+
**Response queue depth is a difference of two counters, so it only holds while neither has reset.** Both reset when a pod is replaced, which every upgrade does, and a scrape that catches one reset and not the other yields a wild value. Staging us-east-1 read -761 in one bucket and +5371 in another over a window where every other bucket sat within one of zero. Read a single implausible bucket as a reset artifact, and judge the panel by whether it returns to zero rather than by any one sample.
8885

8986
## Known noise classes
9087

0 commit comments

Comments
 (0)