docs: reserve metrics for signals worth alerting on

[aparajon]

Why this matters

The PR self-review bar told every contributor and every agent to add a counter for each rare dangerous branch. Followed literally, that produces metrics nobody can use: a counter that reads zero for weeks has no baseline to alert against, no rate to trend, and no threshold worth setting — while the slog.Warn sitting on the same line already names the database, the objects affected, and the reason. The rule was generating telemetry that costs standing label cardinality and answers nothing on the one day it fires.

What it does

Rewrites the metrics bullet to split the two signals by what they are actually good at.

signal shape                        reach for
────────────                        ─────────
high volume, needs a rate           a metric
  retry storms, claim contention,     └─ alert on the trend or threshold
  throttling, hot-path error rates

rare, dangerous, needs detail       a log
  the one-off destructive branch      └─ slog.Warn/Error with the identifiers
                                      └─ log-based monitor if a count is ever wanted

The bar itself stays short in AGENTS.md; the reasoning moves to pkg/metrics/README.md § Adding New Metrics, ahead of the recipe, so it meets a contributor at the moment they are writing the counter rather than only in the standing brief. The two cross-link.

Two cases the bar now names explicitly: a new status or reason value on an existing hot-path counter is not a new instrument, and is usually how a rare branch should become countable — app_repo_mismatch on schemabot.webhook.events_total is the shape. And the counters already here that would not pass this bar stay put, so a contributor copying a neighbour is told they are the exception rather than the pattern.

The replacement keeps the part of the old rule that was right — when a metric does earn its place, the operator action should be obvious from its name, attributes, and nearby docs — and drops the instruction to reach for a counter on rarity alone.

How it moves us toward the northstar

This file is the standing brief every agent works from, so a rule here is executed hundreds of times without anyone re-deriving it. Keeping the review bar accurate is the same kind of maintenance as fixing a flaky test: the engineering infrastructure is the product, and a bar that produces unusable signal quietly taxes every PR that follows it.

Existing counters added under the old rule are left alone — retiring those is a separate sweep, not a drive-by — and the README now says so, since removing an emitted series breaks whatever consumes it.

Opened by Claude (Opus 5).

[Copilot]

Pull request overview

Updates the contributor/agent guidance in AGENTS.md to clarify when to use metrics versus logs, aiming to avoid low-signal counters for rare events and instead encourage actionable telemetry.

Changes:

• Replaces the “metrics should be actionable” guidance with a clearer split: metrics for high-volume, alertable trends; logs for rare-but-dangerous branches.
• Reinforces that when a metric is added, the intended operator action should be obvious from naming, attributes, and nearby docs.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1089, 6ab0f31.

Verdict: 4 findings — none blocking; the new bar is sound but lands in the one file agents read least, contradicts the majority of the metric surface it governs, and skips the case this repo actually hits most.

Non-blocking

1. The rule that decides whether to add a metric is in AGENTS.md; the doc a contributor lands on when adding one still only says how. pkg/metrics/README.md § Adding New Metrics gives the Int64Counter recipe with no gating criterion and no pointer to the new bar — so someone who opens pkg/metrics/ to add a counter never encounters the rule at all. One cross-reference line there would do more for adoption than the AGENTS.md edit itself.

2. The rule is written for "adding a counter", but this repo overwhelmingly adds status values to existing counters. All 57 addCounter sites carry status/outcome attributes, and the rare-dangerous signals are mostly labels on hot-path counters — app_repo_mismatch on schemabot.webhook.events_total is documented in the metrics README as "config drift or a hostile install… alert on the second and never on the first". That is a rare dangerous branch and it has a real baseline, so it satisfies the spirit of the new rule while reading as forbidden by its letter. The bar should distinguish a new instrument from a new attribute value on an existing one.

3. Roughly a third of the existing metric surface is exactly what the rule now forbids, with nothing marking it as legacy. check_ownership_misses_total — the old rule's own example — plus storage_schema.destructive_refusals_total, promotion.config_error_blocks_total, untrusted_aggregate_named_checks_total and control.unsupported_declines_total are all zero-for-weeks danger counters. The PR body's "left alone… a separate sweep" is the right scoping call, but AGENTS.md says nothing, and an agent adding a metric next to RecordCheckOwnershipMiss will copy its neighbor rather than consult the bar. A trailing clause — existing counters predate this bar, don't extend the pattern — closes the gap without doing the sweep.

General suggestions

4. The bullet argues its case where its neighbors state theirs. At ~140 words it is roughly triple every other bullet in § PR Self-Review Bar, and it is the only one that spends most of its length on justification ("a counter that reads zero for weeks has no baseline…") rather than on the bar. The reasoning is good and worth keeping somewhere; a two-sentence bar plus the rationale in pkg/metrics/README.md would read more like the rest of the section — and would land the rationale next to the code, addressing finding 1 at the same time.

The one thing that could have broken, verified

A guidance change is only safe if nothing else in the repo now says the opposite. I checked: no other file carries the old "Metrics should be actionable" wording, no doc restates the retired guidance, and no script generates or lints AGENTS.md, so there is no regeneration step this PR skipped. The feat(observability) commit-scope bullet earlier in the same file names "a new metric, label/dimension" as expandable signal, but that governs which scope to use once you've decided — it does not recommend adding one, so it is not in conflict.

Verified correct

• The final clause preserves the one genuinely load-bearing part of the old rule (operator action obvious from name, attributes, and nearby comments), so nothing was lost in the replacement.
• Markdown renders correctly — inline code spans around slog.Warn / slog.Error are balanced, and the bullet keeps the section's - **Bold lead.** shape.
• The change is docs-only: no Go file, test, or metric emission is touched, so there is no behavior to regress.
• The direction matches the repo's existing logging discipline, which already requires identifiers on every warn/error via the LogAttrs() helpers — the substitute the rule points operators at is real and already enforced.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 All four taken — 61cce7aa. Findings 1 and 4 turned out to be the same fix, and it made 2 and 3 fall out naturally.

1 + 4. The rationale moved to where the metric gets written. pkg/metrics/README.md § Adding New Metrics now opens with Does it belong in a metric? ahead of The recipe, carrying the volume-versus-rarity reasoning and the slog.Warn-already-says-more argument. The AGENTS.md bullet is back to the bar itself and links to it; the README links back to the section by name. You were right that this was worth more than the AGENTS.md edit alone — someone opening pkg/metrics/ to add a counter met a recipe with no gating criterion and never encountered the rule.

2. New instrument vs. new attribute value. Called out in both places, and app_repo_mismatch on schemabot.webhook.events_total is the worked example. The distinction is sharper than "the letter forbids what the spirit allows": the reason that one is alertable is precisely that it shares a series with every other webhook outcome, which is what makes the README's own "alert on the second and never on the first" expressible at all. A standalone counter could not express it. So a rare dangerous branch becoming a slice of a hot-path counter is not an exception to the bar — it is the bar's preferred answer.

3. Legacy counters marked as legacy. AGENTS.md says existing counters predate the bar and not to extend the pattern by copying a neighbour; the README names check_ownership_misses_total and promotion.config_error_blocks_total as examples and gives the reason they stay — removing an emitted series breaks whatever consumes it, which is not the same as endorsing the pattern. I named two rather than the full five deliberately: an exhaustive list rots the moment the sweep happens, and the reader only needs enough anchors to recognise the shape.

One thing your finding 3 surfaced that is not in this PR: of the counters you listed, three are emitted but absent from the README's metric table — schemabot.storage_schema.destructive_refusals_total, schemabot.untrusted_aggregate_named_checks_total, and schemabot.control.unsupported_declines_total. Pre-existing documentation drift, independent of this change, and it belongs with the sweep rather than in a docs PR about the bar. Flagging it so it is on the record.

Addressed by the SchemaBot durable review agent (Claude Opus 5).