From 683da76151729421a70b7314a8b73fedc9d6d40f Mon Sep 17 00:00:00 2001 From: Moritz Hoffmann Date: Wed, 26 Aug 2026 15:12:18 +0200 Subject: [PATCH 1/2] Fix the release-signoff metric lint skipping fenced files The extractor paired backticks sequentially across the whole document. A fenced block contains backticks of its own, so pairing across one flipped the parity of every span after it: prose was captured as code and the real code spans became the separators between matches. Three of the six files in the skill contain a fence, and in those the lint checked almost nothing. SKILL.md and compute.md contributed no names at all and sources-and-sinks.md contributed two of forty, so a rename in any of them passed CI silently, which is the one failure the lint exists to prevent. Pull fenced blocks out as whole tokens before pairing backticks in what remains, and test the wildcard skip per word rather than per token, since a fence arrives as a single token and one wildcard inside it must not exempt the block. Also expand the abbreviations that roster rows use. A row writes a family as `mz_foo_bar`, `_baz`, and a bare continuation carries no mz_ prefix, so it was never a candidate. Which prefix of the base it attaches to is ambiguous, because rows drop either one component or two, so accept the first expansion that resolves and report only when none does. Continuations are sought within a single line: carried across lines they attach to whatever name appeared in an earlier paragraph and manufacture failures. Bare histogram suffixes are excluded, being prose about the parts of a histogram rather than family members. Together these take the guard from 98 names to every file catching a rename, and they surface two roster entries that name metrics the product does not export. `mz_storage_upsert_state_rocksdb_autospill_in_use` went with the autospill upsert backend, and its row still explained how to read the percentage panel, so a verifier would conclude no worker is spilling rather than that the concept is gone. There is no `mz_persist_gc_skipped`: the already-done case is `mz_persist_gc_noop`, and the only skipped counter is `mz_persist_compaction_skipped`. Both rows now record the metric as gone. Co-Authored-By: Claude Opus 5 (1M context) --- .agents/skills/mz-release-signoff/SKILL.md | 2 +- .../mz-release-signoff/references/persist.md | 2 +- .../references/sources-and-sinks.md | 2 +- .../scripts/lint_metrics.py | 100 +++++++++++++++--- .../scripts/metrics-allowlist.txt | 26 ++++- 5 files changed, 112 insertions(+), 20 deletions(-) diff --git a/.agents/skills/mz-release-signoff/SKILL.md b/.agents/skills/mz-release-signoff/SKILL.md index b8aec0715ca49..cffc14974a501 100644 --- a/.agents/skills/mz-release-signoff/SKILL.md +++ b/.agents/skills/mz-release-signoff/SKILL.md @@ -252,7 +252,7 @@ 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. 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/persist.md b/.agents/skills/mz-release-signoff/references/persist.md index 3c17f59e9a984..33468f28f4326 100644 --- a/.agents/skills/mz-release-signoff/references/persist.md +++ b/.agents/skills/mz-release-signoff/references/persist.md @@ -32,7 +32,7 @@ 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. | 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..f59b9cb400095 100644 --- a/.agents/skills/mz-release-signoff/references/sources-and-sinks.md +++ b/.agents/skills/mz-release-signoff/references/sources-and-sinks.md @@ -56,7 +56,7 @@ 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 | | diff --git a/.agents/skills/mz-release-signoff/scripts/lint_metrics.py b/.agents/skills/mz-release-signoff/scripts/lint_metrics.py index 64d73fdc19728..1e6c92735146c 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,24 @@ def resolves(name, exact, globs): ) +def resolve_continuation(continuation, base, exact, globs, allowed): + """Resolve `_baz` against the family of a preceding `mz_foo_bar`. + + Which prefix of the base the continuation attaches to is ambiguous: the + roster writes both `mz_persist_gc_seconds`, `_started`, which drops one + component, and `mz_compute_controller_replica_count`, `_collection_count`, + which drops two. Accept the first prefix that resolves, and report the + continuation only when no prefix does, which is what a removed metric + looks like. + """ + parts = base.split("_") + for cut in range(len(parts) - 1, 0, -1): + candidate = "_".join(parts[:cut]) + continuation + if candidate in allowed or resolves(candidate, exact, globs): + return candidate + return None + + def allowlisted(): names = set() for line in ALLOWLIST.read_text().splitlines(): @@ -74,20 +95,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 +162,15 @@ 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)) + for label, target, path in referenced(): + if isinstance(target, tuple): + continuation, base = target + if resolve_continuation(continuation, base, exact, globs, allowed): + 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: 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 From 46b37899ece70c0fcc7dbd045a41de748fafc455 Mon Sep 17 00:00:00 2001 From: Moritz Hoffmann Date: Thu, 27 Aug 2026 11:20:48 +0200 Subject: [PATCH 2/2] Pin abbreviated metric names in the release-signoff lint An abbreviated roster entry such as `mz_foo_bar`, `_baz` resolves by trying each prefix of the base, since the row does not say how many components the abbreviation drops. Four entries resolve against more than one prefix. The longest cut is the intended metric and is the one accepted, so nothing binds wrongly today, but deleting that metric leaves a shorter cut resolving, and the lint stays green on a row that points at nothing. Report an abbreviation that resolves more than one way, and spell those four out in full. Also spell out `mz_persist_read_batch_part_count`, which the bare-suffix guard was skipping as prose about a histogram. It is a separately registered counter, so its removal was undetectable. Record the two remaining bounds on what a green run proves: a name covered by a glob catalog entry cannot be falsified, and a bare `_count`, `_sum`, or `_bucket` is read as prose. Co-Authored-By: Claude Opus 5 --- .agents/skills/mz-release-signoff/SKILL.md | 4 +- .../mz-release-signoff/references/compute.md | 2 +- .../mz-release-signoff/references/persist.md | 4 +- .../references/sources-and-sinks.md | 2 +- .../scripts/lint_metrics.py | 49 ++++++++++++++----- 5 files changed, 43 insertions(+), 18 deletions(-) diff --git a/.agents/skills/mz-release-signoff/SKILL.md b/.agents/skills/mz-release-signoff/SKILL.md index cffc14974a501..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`. 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. 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 33468f28f4326..73235a108c8b2 100644 --- a/.agents/skills/mz-release-signoff/references/persist.md +++ b/.agents/skills/mz-release-signoff/references/persist.md @@ -35,8 +35,8 @@ Worth adding to the sweep even though the panel omits them: `mz_persist_compacti | `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 f59b9cb400095..d36db4ad3fc26 100644 --- a/.agents/skills/mz-release-signoff/references/sources-and-sinks.md +++ b/.agents/skills/mz-release-signoff/references/sources-and-sinks.md @@ -59,7 +59,7 @@ Label naming is not consistent: the rate panels filter `parent_source_id` while | `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 1e6c92735146c..05ee4d147f115 100644 --- a/.agents/skills/mz-release-signoff/scripts/lint_metrics.py +++ b/.agents/skills/mz-release-signoff/scripts/lint_metrics.py @@ -71,19 +71,26 @@ def resolves(name, exact, globs): def resolve_continuation(continuation, base, exact, globs, allowed): """Resolve `_baz` against the family of a preceding `mz_foo_bar`. - Which prefix of the base the continuation attaches to is ambiguous: the - roster writes both `mz_persist_gc_seconds`, `_started`, which drops one - component, and `mz_compute_controller_replica_count`, `_collection_count`, - which drops two. Accept the first prefix that resolves, and report the - continuation only when no prefix does, which is what a removed metric - looks like. + 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("_") - for cut in range(len(parts) - 1, 0, -1): - candidate = "_".join(parts[:cut]) + continuation - if candidate in allowed or resolves(candidate, exact, globs): - return candidate - return None + 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(): @@ -162,10 +169,15 @@ def main() -> int: allowed = allowlisted() unresolved = {} + ambiguous = {} for label, target, path in referenced(): if isinstance(target, tuple): continuation, base = target - if resolve_continuation(continuation, base, exact, globs, allowed): + 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): @@ -179,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:" @@ -191,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__":