diff --git a/.agents/skills/mz-release-signoff/SKILL.md b/.agents/skills/mz-release-signoff/SKILL.md index b8aec0715ca49..15150ceaa3cc6 100644 --- a/.agents/skills/mz-release-signoff/SKILL.md +++ b/.agents/skills/mz-release-signoff/SKILL.md @@ -252,7 +252,9 @@ The diff survives that blind spot, because both sides share it and a dynamically 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 --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"`. -`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. +`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`. Roster rows that abbreviate a family as `` `mz_foo_bar`, `_baz` `` are expanded against the preceding name, and fenced blocks are extracted before backticks are paired, since pairing across a fence desynchronises every span after it. An abbreviation that resolves against more than one prefix of its base also fails, because the row is then not pinned to one metric: deleting the intended one leaves a shorter cut resolving and the row green while it points at nothing. Spell that member out in full. + +Two blind spots bound what a green run proves. A name covered by a glob entry cannot be falsified, since the pattern keeps matching after the metric is gone, which currently affects ten names including the whole `mz_persist_*_bytes` family. A bare `` `_count` ``, `` `_sum` ``, or `` `_bucket` `` is read as prose about a histogram rather than as an abbreviation, because that is what it usually is, so a family member has to be written out in full to be checked. 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. ## Traps diff --git a/.agents/skills/mz-release-signoff/references/compute.md b/.agents/skills/mz-release-signoff/references/compute.md index 95a3ed5997250..ce4501a1a7331 100644 --- a/.agents/skills/mz-release-signoff/references/compute.md +++ b/.agents/skills/mz-release-signoff/references/compute.md @@ -43,7 +43,7 @@ System clusters are `s` followed by digits in either family, and `.*cluster-s[0- | Metric | Type | Notes | |---|---|---| -| `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. | +| `mz_compute_controller_replica_count`, `mz_compute_controller_collection_count`, `_peek_count`, `_subscribe_count` | gauge | Controller-side inventory. Collection count tracks catalog growth, so a slow rise is expected. | | `mz_compute_controller_command_queue_size` | gauge | Sustained depth means the controller is not draining. | | `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. | | `mz_compute_commands_total`, `mz_compute_responses_total` | counter | Protocol volume. Doubles for one bucket during a zero-downtime upgrade. | diff --git a/.agents/skills/mz-release-signoff/references/persist.md b/.agents/skills/mz-release-signoff/references/persist.md index 3c17f59e9a984..73235a108c8b2 100644 --- a/.agents/skills/mz-release-signoff/references/persist.md +++ b/.agents/skills/mz-release-signoff/references/persist.md @@ -32,11 +32,11 @@ Worth adding to the sweep even though the panel omits them: `mz_persist_compacti | `mz_persist_external_seconds`, `_started_count`, `_succeeded_count`, `_failed_count` | counter | Blob and consensus calls. External time dominates every other timing series. | | `mz_persist_external_rtt_latency`, `mz_persist_external_op_latency_bucket` | gauge, histogram | Per-operation latency, split by `op`. | | `mz_persist_compaction_seconds`, `_requested`, `_applied`, `_bytes`, `_goodbytes` | counter | Compaction. Requested minus applied equals the noop and dropped counts. | -| `mz_persist_gc_seconds`, `_started`, `_finished`, `_noop`, `_skipped`, `_merged_reqs` | counter | Garbage collection. | +| `mz_persist_gc_seconds`, `_started`, `_finished`, `_noop`, `_merged_reqs` | counter | Garbage collection. The already-done case is `_noop`. There is no `mz_persist_gc_skipped`, and `skipped` exists only as `mz_persist_compaction_skipped`. | | `mz_persist_retry_retries_count`, `_started_count`, `_finished_count`, `_sleep_seconds` | counter | The panel excludes `op="next_listen_batch"`, which has its own panel because it retries by design. | | `mz_persist_shard_upper` | gauge, per shard | Counting distinct `shard` labels gives the shard count. | -| `mz_persist_read_batch_part_bytes`, `_count` | counter, by `op` | The `op="unindexed"` slice is the `unindexed reads` panel. Rises by roughly 3x during rehydration. | -| `mz_persist_pushdown_parts_filtered_count`, `_fetched_count`, `_inline_count`, `_faked_count` and their `_bytes` variants | counter | The ratio panels divide one by the sum of all four. | +| `mz_persist_read_batch_part_bytes`, `mz_persist_read_batch_part_count` | counter, by `op` | The `op="unindexed"` slice is the `unindexed reads` panel. Rises by roughly 3x during rehydration. | +| `mz_persist_pushdown_parts_filtered_count`, `_fetched_count`, `_inline_count`, `_faked_count` and their `mz_persist_pushdown_parts_filtered_bytes` variants | counter | The ratio panels divide one by the sum of all four. | | `mz_persist_blob_cache_hits_bytes`, `_hits_blobs`, `_evictions` | counter | | | `mz_txn_batch_unapplied_count`, `_unapplied_min_ts`, `_commit_count`, `_commit_bytes` | gauge, counter | Txn-shard backlog. | | `mz_txn_op_started_count`, `_succeeded_count`, `_retry_count`, `_errored_count` | counter, by `op` | | diff --git a/.agents/skills/mz-release-signoff/references/sources-and-sinks.md b/.agents/skills/mz-release-signoff/references/sources-and-sinks.md index a61d1ecbd2524..d36db4ad3fc26 100644 --- a/.agents/skills/mz-release-signoff/references/sources-and-sinks.md +++ b/.agents/skills/mz-release-signoff/references/sources-and-sinks.md @@ -56,10 +56,10 @@ Label naming is not consistent: the rate panels filter `parent_source_id` while | `mz_storage_upsert_deletes_total`, `mz_source_envelope_state_tombstones` | counter, gauge | | | `mz_storage_upsert_state_rehydration_latency` | gauge, milliseconds | Last rehydration duration. See hazards. | | `mz_storage_upsert_state_rehydration_total`, `_rehydration_updates` | counter | Records and updates replayed. Their ratio exposes retraction-heavy state. | -| `mz_storage_upsert_state_rocksdb_autospill_in_use` | gauge, one per worker | The percentage panel divides `sum` by `count`, so it reports the fraction of workers spilling. | +| `mz_storage_upsert_state_rocksdb_autospill_in_use` | removed | Deleted along with the autospill upsert backend, so the percentage panel that divides `sum` by `count` renders empty and reads as no worker spilling. The surviving members of the family are `_rehydration_latency`, `_rehydration_total`, and `_rehydration_updates`. | | `mz_storage_upsert_backpressure_emitted_bytes`, `_retired_bytes`, `_last_backpressured_bytes` | counter, gauge | Backpressure. | | `mz_storage_upsert_merge_snapshot_latency_bucket` | histogram | | -| `mz_storage_upsert_merge_snapshot_updates_total`, `_inserts_total`, `_deletes_total` | counter | | +| `mz_storage_upsert_merge_snapshot_updates_total`, `mz_storage_upsert_merge_snapshot_inserts_total`, `mz_storage_upsert_merge_snapshot_deletes_total` | counter | | | `mz_storage_rocksdb_multi_get_latency_bucket`, `_multi_put_latency_bucket` | histogram | Panels plot p95, p99, and p999. | | `mz_storage_rocksdb_multi_get_count_total`, `_size_total`, `_result_bytes_total`, `_result_count_total`, and the `multi_put` equivalents | counter | Batch sizes are the size over the count. | | `mz_persist_shard_update_count` | gauge, by `shard` and `name` | Filtered by `name="$source"`, so it joins a source to its persist shard. | diff --git a/.agents/skills/mz-release-signoff/scripts/lint_metrics.py b/.agents/skills/mz-release-signoff/scripts/lint_metrics.py index 64d73fdc19728..05ee4d147f115 100644 --- a/.agents/skills/mz-release-signoff/scripts/lint_metrics.py +++ b/.agents/skills/mz-release-signoff/scripts/lint_metrics.py @@ -24,6 +24,9 @@ CATALOG_NAME = re.compile(r"^- name: '?(.+?)'?$") BACKTICKED = re.compile(r"`([^`]+)`") CANDIDATE = re.compile(r"\b(mz_[a-z0-9_]+)\b") +FENCED = re.compile(r"^```.*?^```", re.S | re.M) +# A roster row abbreviates a family as `mz_foo_bar`, `_baz`, `_qux`. +CONTINUATION = re.compile(r"^_[a-z0-9_]+$") # Histograms and summaries are catalogued as their expanded families, so a # reference naming the base is correct and must resolve through any suffix. @@ -65,6 +68,31 @@ def resolves(name, exact, globs): ) +def resolve_continuation(continuation, base, exact, globs, allowed): + """Resolve `_baz` against the family of a preceding `mz_foo_bar`. + + Return every prefix of the base that the continuation resolves against, + longest first. How many components the continuation drops is not stated by + the row, so every cut has to be tried: the roster writes both + `mz_persist_gc_seconds`, `_started`, which drops one, and + `mz_compute_controller_replica_count`, `_peek_count`, which drops two. + + More than one hit means the row is not pinned to a single metric. That + matters on removal rather than today, because the longest cut is the + intended one and comes first: delete the intended metric and a shorter cut + still resolves, so the row stays green while pointing at nothing. The + caller rejects an ambiguous row for that reason, and the fix is to spell + the member out in full. + """ + parts = base.split("_") + return [ + candidate + for cut in range(len(parts) - 1, 0, -1) + for candidate in ["_".join(parts[:cut]) + continuation] + if candidate in allowed or resolves(candidate, exact, globs) + ] + + def allowlisted(): names = set() for line in ALLOWLIST.read_text().splitlines(): @@ -74,20 +102,62 @@ def allowlisted(): return names +def names_in(token): + """Yield every catalogued-namespace metric name inside one code token. + + A wildcard names a family whose stem is not itself catalogued, so it is + skipped. The test is per word rather than per token because a fenced block + arrives as one token, and a single wildcard inside it must not exempt the + whole block. + """ + for word in token.split(): + if "*" in word: + continue + for name in CANDIDATE.findall(word): + yield name.rstrip("_") + + def referenced(): - """Yield (name, file) for every mz_* token inside backticks in the skill.""" + """Yield (label, target, file) for every metric named in the skill. + + Fenced blocks have to be pulled out before backticks are paired. A fence + contains backticks of its own, so pairing sequentially across one flips the + parity of every span after it: prose gets captured as code and the real + code spans become the separators between matches. Left unhandled, that + silently disables the check for every file containing a fence. + """ for path in sorted(SKILL.rglob("*.md")): - for token in BACKTICKED.findall(path.read_text()): - # A prefix wildcard such as `mz_persist_*` names a family, not a - # metric, and its stem is not itself catalogued. Nothing here can - # be checked, so skip the whole token. - if "*" in token: - continue - for name in CANDIDATE.findall(token): - # Brace expansion such as `mz_foo_{sum,count}` leaves a - # trailing underscore. Resolve the base, which the catalog - # holds as an expanded family. - yield name.rstrip("_"), path + text = path.read_text() + for block in FENCED.findall(text): + for name in names_in(block): + yield name, name, path + # Continuations abbreviate within a single roster row, so the base is + # only sought on the same line. Tracking it across lines attaches a + # `_sum` to whatever full name happened to appear in an earlier + # paragraph, which manufactures failures rather than finding them. + for line in FENCED.sub("\n", text).splitlines(): + previous = None + for span in BACKTICKED.findall(line): + stripped = span.strip() + # A bare histogram suffix is prose about the parts of a + # histogram, as in "the `_sum` rate of `mz_slow_message_handling`", + # never a family member abbreviated in a roster row. + if stripped in SUFFIXES: + continue + if CONTINUATION.match(stripped): + # With no in-scope base on this line the continuation + # belongs to a family the catalog does not hold, such as + # v2_mz_* or container_*, and cannot be checked. + if previous: + yield ( + f"{stripped} (after {previous})", + (stripped, previous), + path, + ) + continue + for name in names_in(span): + yield name, name, path + previous = name def main() -> int: @@ -99,10 +169,20 @@ def main() -> int: allowed = allowlisted() unresolved = {} - for name, path in referenced(): - if name in allowed or resolves(name, exact, globs): - continue - unresolved.setdefault(name, set()).add(str(path)) + ambiguous = {} + for label, target, path in referenced(): + if isinstance(target, tuple): + continuation, base = target + hits = resolve_continuation(continuation, base, exact, globs, allowed) + if len(hits) > 1: + ambiguous.setdefault(label, (hits, set()))[1].add(str(path)) + continue + if hits: + continue + else: + if target in allowed or resolves(target, exact, globs): + continue + unresolved.setdefault(label, set()).add(str(path)) stale = sorted(n for n in allowed if resolves(n, exact, globs)) if stale: @@ -111,6 +191,17 @@ def main() -> int: print(f" {name}") print() + if ambiguous: + print("Abbreviated metric names that resolve more than one way:") + for label in sorted(ambiguous): + hits, paths = ambiguous[label] + print(f" {label} ({', '.join(sorted(paths))})") + print(f" resolves to: {', '.join(hits)}") + print() + print("The row is not pinned to one metric, so removing the intended one") + print("leaves the lint green. Spell the member out in full.") + print() + if unresolved: print( "Metric names in the skill that resolve in neither the catalog nor the allowlist:" @@ -123,7 +214,7 @@ def main() -> int: f"catalog cannot see it, in which case add it to {ALLOWLIST} with a reason." ) - return 1 if (unresolved or stale) else 0 + return 1 if (unresolved or stale or ambiguous) else 0 if __name__ == "__main__": diff --git a/.agents/skills/mz-release-signoff/scripts/metrics-allowlist.txt b/.agents/skills/mz-release-signoff/scripts/metrics-allowlist.txt index 002e5096671fc..015778bf16a5b 100644 --- a/.agents/skills/mz-release-signoff/scripts/metrics-allowlist.txt +++ b/.agents/skills/mz-release-signoff/scripts/metrics-allowlist.txt @@ -32,7 +32,6 @@ mz_external_calls_count mz_external_long_lived_connection_count mz_cloud_egress_check_reachability_count mz_dataflow_initial_output_duration_seconds -mz_storage_messages_sent_bytes_count # --- Labels and identifiers that merely look like metric names --- mz_version @@ -59,3 +58,28 @@ mz_catalog mz_persist_columnar_validation_count mz_txn_placeholder_schema_apply mz_query_latency_bucket +mz_query_latency + +# The `Container Max RSS Memory Usage` panel plots mz_metrics_libc_ru_maxrss +# scaled by 1024. The live metric is mz_metrics_libc_ru_maxrss_bytes, so the +# scaled series is empty and the panel shows one line where it should show two. +mz_metrics_libc_ru_maxrss + +# The storage controller protocol panels straddle a rename with an `or`. These +# are the dead arm, kept in the reference because the `or` is deliberate and +# worth imitating. The live names are mz_storage_commands_total, +# mz_storage_responses_total, mz_storage_command_message_bytes_total, and +# mz_storage_response_message_bytes_total. +mz_storage_messages_sent_bytes_count +mz_storage_messages_received_bytes_count +mz_storage_messages_sent_bytes_sum +mz_storage_messages_received_bytes_sum + +# Removed with the autospill upsert backend. The reference marks the row dead +# because the percentage panel still plots it and so renders empty. +mz_storage_upsert_state_rocksdb_autospill_in_use + +# Named only to say it does not exist. The GC roster once listed it, but the +# already-done case is mz_persist_gc_noop and the only `skipped` counter is +# mz_persist_compaction_skipped. +mz_persist_gc_skipped