feat(prometheus): bootstrap 6 rule blocks in go-prometheus-metrics-guide - #8
Conversation
Restructures the 6 MUST/SHOULD sections in docs/go-prometheus-metrics-guide.md into canonical `### RULE` blocks consumable by the rule-base walker (scripts/build-index.py). Mirrors the bootstrap template proven across PRs #2-5 (errors, security, factory, http-handler). Rules added to rules/index.json (21 → 27 entries): - go-prometheus/counter-pre-initialization (MUST, judgment) Pre-init counters with .Add(0) for known label combos so rate() doesn't silently skip absent series. - go-prometheus/composed-metrics-interface (SHOULD, judgment) Split fat Metrics interfaces into focused sub-interfaces (ISP). - go-prometheus/no-gauge-for-monotonic (MUST, judgment) Never use GaugeVec for values that only increase — breaks rate() and increase() queries. - go-prometheus/counter-total-suffix (MUST, judgment for now) Counter Name must end with _total. Mechanical ast-grep YAML was drafted but CounterOpts struct-literal traversal in ast-grep 0.43.0 needs more debugging; deferring to a focused follow-up PR rather than blocking this bootstrap. - go-prometheus/help-string-quality (MUST, judgment) Unique, accurate Help strings per metric; reject copy-paste residue. - go-prometheus/label-naming-consistency (MUST, judgment) Same label name for same concept across all metrics in the project. All rule blocks carry: Owner (go-metrics-assistant), Applies when, Enforcement, Why, Bad/Good code snippets. Bad example for no-gauge-for-monotonic also de-trades (candleHandleTotalCounter + broker label → orderHandleTotalCounter + tenant label) — leftover trading-domain leak from PR #6's first trim pass. No personal vault paths, no trading-domain terms, no remaining MUST section without a rule id (verified by grep).
There was a problem hiding this comment.
Step 5: Consolidated Report
PR Review: feat(prometheus): bootstrap 6 rule blocks in go-prometheus-metrics-guide
Target branch: master → feat/bootstrap-prometheus
Scope: docs (2 files changed: docs/go-prometheus-metrics-guide.md, rules/index.json)
Change summary: Added 6 new Prometheus metric RULE blocks to the guide and registered them in rules/index.json (21 → 27 entries).
Must Fix (Critical)
1. docs/go-prometheus-metrics-guide.md — Internal contradiction between two rules
The Good example for go-prometheus/no-gauge-for-monotonic (line ~127) shows:
orderHandleTotalCounter=prometheus.NewCounterVec(prometheus.CounterOpts{
Name: "total", // ← missing "_total" suffix
}, []string{"tenant"})This directly contradicts go-prometheus/counter-total-suffix (also added in this PR, line ~206), which is a MUST rule requiring all Counter names to end with _total. A reader who follows the no-gauge-for-monotonic Good example will produce a metric that violates the counter-total-suffix rule in the same guide.
Fix: Change to Name: "order_handle_total" or any name ending in _total.
Should Fix (Important)
2. docs/go-prometheus-metrics-guide.md — Imprecise PromQL explanation in no-gauge-for-monotonic Why
The Why text reads: "they assume the underlying type is a counter that can reset to zero on process restart"
PromQL functions are type-agnostic — they don't inspect metric types. The actual mechanism: rate() and increase() interpret any downward movement as a counter reset. The explanation is directionally correct but misstates the cause.
3. docs/go-prometheus-metrics-guide.md — Verbose Enforcement in counter-total-suffix
The Enforcement field reads: "judgment (mechanical ast-grep YAML tracked as follow-up; CounterOpts struct-literal traversal in ast-grep 0.43.0 needs further investigation)"
Tool-execution notes belong in task trackers, not enforceable rule blocks. Simplify to "judgment" or "judgment (ast-grep follow-up)".
Nice to Have (Optional)
4. help-string-quality — Only one Bad/Good pair, but Applies when has three conditions
The rule lists (a) empty Help, (b) duplicate Help, (c) copy-paste residue — but only (c) has an example.
5. composed-metrics-interface — Arbitrary ~6 method threshold
The count-based heuristic can be gamed. Reframe around functional-domain span rather than a magic number.
6. counter-pre-initialization — Missing cardinality caveat
Pre-initializing unbounded label domains (e.g., 200 endpoints) is costly. The Why should note the rule only applies to small, bounded domains (< ~20 combinations).
Security Check ✅
Diff was scanned for residual personal vault paths and trading-domain terms (Candle, Epic, Broker, SignalStore, SignalStore, etc.). None found. All examples use generic terms (tenant, product, channel).
rules/index.json Review ✅
All 6 new entries are correct: level, owner, applies_when, enforcement, doc_path, id, and anchor fields all match the doc. No JSON syntax errors. Entries are alphabetically ordered. Coverage: 6/6 doc rules have matching index entries.
Step 6: Next Steps
Fix the Name: "total" → Name: "order_handle_total" contradiction (Must Fix #1) before merging. The other issues are non-blocking but should be addressed for polish.
Verdict JSON
{
"verdict": "request-changes",
"summary": "Six new Prometheus rule blocks are well-structured and the rules/index.json is correct, but a critical internal contradiction exists: the Good example in no-gauge-for-monotonic uses 'Name: total' which violates the counter-total-suffix MUST rule added in the same PR. Two should-fix issues (imprecise PromQL explanation, verbose enforcement field) and three nice-to-have items also need attention.",
"comments": [
{
"file": "docs/go-prometheus-metrics-guide.md",
"line": 127,
"severity": "critical",
"message": "Must Fix: The Good example for go-prometheus/no-gauge-for-monotonic shows Name: \"total\" which does not end with _total, directly contradicting the go-prometheus/counter-total-suffix MUST rule defined in the same guide. Change to Name: \"order_handle_total\" or any name ending in _total."
},
{
"file": "docs/go-prometheus-metrics-guide.md",
"line": 110,
"severity": "major",
"message": "Should Fix: The Why paragraph for no-gauge-for-monotonic states that rate()/increase() 'assume the underlying type is a counter that can reset to zero on process restart.' This is imprecise — PromQL is type-agnostic and interprets any downward movement as a reset, not just process restarts. Reword to clarify the actual mechanism."
},
{
"file": "docs/go-prometheus-metrics-guide.md",
"line": 210,
"severity": "major",
"message": "Should Fix: The Enforcement field for counter-total-suffix is verbose with implementation notes ('ast-grep YAML tracked as follow-up; CounterOpts struct-literal traversal in ast-grep 0.43.0 needs further investigation'). These belong in task trackers, not enforceable rules. Simplify to 'judgment' or 'judgment (ast-grep follow-up)'."
},
{
"file": "docs/go-prometheus-metrics-guide.md",
"line": 241,
"severity": "nit",
"message": "Nice to Have: help-string-quality's Applies when clause lists three conditions (empty, duplicate, copy-paste residue Help) but only the third is illustrated with a Bad/Good pair. Add examples for empty Help (Help: \"\") and duplicate Help."
},
{
"file": "docs/go-prometheus-metrics-guide.md",
"line": 56,
"severity": "nit",
"message": "Nice to Have: composed-metrics-interface uses an arbitrary '~6 methods' threshold. Reframe the applies_when clause around functional-domain span rather than a count-based heuristic that can be misapplied."
},
{
"file": "docs/go-prometheus-metrics-guide.md",
"line": 23,
"severity": "nit",
"message": "Nice to Have: counter-pre-initialization should note that pre-initialization is only worthwhile for small bounded label domains (typically < 20 combinations). For large domains, callers should prefer absent() checks in alerting rules."
}
],
"concerns_addressed": [
"correctness: counter-total-suffix enforcement judgment noted — ast-grep YAML deferred as documented follow-up, not flagged as an error",
"security: no residual personal vault paths or trading-domain terms found — examples use only generic tenant/product/channel",
"tests: rules/index.json grew 21 → 27 entries with correct level (5 MUST, 1 SHOULD) and owner (go-metrics-assistant) for all 6 new rules"
]
}Bot found 1 CRITICAL + 2 MAJOR + 3 NIT on 2def5dc — all valid: CRITICAL (internal contradiction): - no-gauge-for-monotonic Good example used Name: 'total', directly violating the counter-total-suffix MUST rule defined in the same file. Fixed to Name: 'order_handle_total' in both Bad and Good examples so the contrast is metric-type, not naming. MAJOR: - no-gauge-for-monotonic Why was imprecise: said rate()/increase() 'assume the underlying type is a counter that can reset to zero on process restart.' PromQL is type-agnostic — it treats any downward sample movement as a reset regardless of the underlying type signal. Reworded to clarify the actual mechanism. - counter-total-suffix Enforcement field had implementation notes ('ast-grep YAML tracked as follow-up; CounterOpts struct-literal traversal in ast-grep 0.43.0 needs further investigation') that belong in commit messages / task trackers, not in the rule itself. Simplified to 'judgment (ast-grep follow-up)'. NIT (quality-of-life, fixed since the doc was already open): - help-string-quality listed three trigger conditions (empty, duplicate, copy-paste) but only illustrated the third. Added Bad examples for the other two so the rule is self-documenting. - composed-metrics-interface used an arbitrary ~6-methods threshold; reframed around functional-domain span instead of count. - counter-pre-initialization didn't note that pre-init is only worthwhile for small bounded label domains (< 20 combos). Added the bound and the absent()-check alternative for large domains.
superseded by new automated review
There was a problem hiding this comment.
{
"verdict": "approve",
"summary": "PR #8 bootstraps 6 `### RULE` blocks in go-prometheus-metrics-guide with matching index.json entries, all correctly anchored, owned by go-metrics-assistant (confirmed to exist), and free of trading-domain terms. All four plan concerns are resolved: anchors match, enforcement distinction for counter-total-suffix is intentional and documented, and the agent is live. One minor note on the commit message accuracy and one absence worth tracking.",
"comments": [
{
"file": "docs/go-prometheus-metrics-guide.md",
"line": 20,
"severity": "minor",
"message": "counter-pre-initialization: anchor present and correct, but the commit message for PR #8 described the bootstrap template as using `## RULE <id>` level headers. The actual markdown uses `### RULE` (H3), which is consistent with other guides in the repo. No action needed — just a commit-message inaccuracy that does not affect tooling."
},
{
"file": "rules/index.json",
"line": 147,
"severity": "minor",
"message": "counter-total-suffix enforcement reads 'judgment (ast-grep follow-up)' while the other 5 rules say just 'judgment'. This is intentional per the commit message — a mechanical ast-grep pattern was deferred to a follow-up PR rather than blocking the bootstrap. The distinction is correctly documented here."
}
],
"concerns_addressed": [
"correctness: anchor values in index.json match the ### RULE anchors in markdown doc exactly — go-prometheus/counter-pre-initialization, composed-metrics-interface, no-gauge-for-monotonic, counter-total-suffix, help-string-quality, label-naming-consistency all verified at lines 20, 53, 105, 206, 231, 273",
"correctness: counter-total-suffix enforcement 'judgment (ast-grep follow-up)' is intentional — the commit message explains mechanical ast-grep was deferred to a focused follow-up PR",
"correctness: rule anchor links in markdown match id fields in rules/index.json — all 6 verified",
"correctness: all 6 rules have owner 'go-metrics-assistant' — agent confirmed to exist at agents/go-metrics-assistant.md"
]
}Uh oh!
There was an error while loading. Please reload this page.
Restructures the 8 MUST sections in docs/go-testing-guide.md into canonical `### RULE` blocks. Same template as PRs #2-5, #8. Rules added (all judgment for this PR; mechanical ast-grep follow-ups tracked separately): - go-testing/no-stdlib-table-tests (MUST) Ginkgo-suite packages must use DescribeTable/Entry, not for/t.Run. - go-testing/no-testing-t-direct (MUST) No bare testing.T in Ginkgo-suite packages — use Describe/It. - go-testing/no-bare-error-call (MUST) Wrap error-returning calls in Gomega matchers (Succeed/HaveOccurred/ MatchError) — errcheck enforces this in precommit. - go-testing/suite-test-file-required (MUST) Every test-containing package needs *_suite_test.go or Ginkgo silently discovers no specs. - go-testing/main-test-with-compiles (MUST) Every binary needs main_test.go with a Compiles It-block backed by gexec.Build. - go-testing/suite-timeout-required (MUST) suiteConfig.Timeout must be set so hung tests fail fast. - go-testing/counterfeiter-mocks-required (MUST) No hand-written mocks — they drift silently. - go-testing/libtime-injection-required (MUST) Cross-references go-time/no-time-now-direct; test-quality scope. rules/index.json: 27 → 42 entries This also picks up 7 agent-cmd/* entries from PR #9 (#9 merged the doc but didn't include the walker output regen — caught locally via make build-index). Net new in this PR: 8 go-testing rules. Pre-emptive checks (lessons from PRs #6, #8): no personal vault paths, no trading-domain terms, no internal contradictions (rule examples don't violate sibling rules).
Closes the mechanical enforcement gap from PR #8 (which deferred the YAML to a focused follow-up after the PR #4 lesson about ast-grep 0.43.0 traversal complexity). Rule shape (smoke-tested against fixture files with 4 distinct edge cases): pattern: context: 'prometheus.CounterOpts{Name: $V, $$$}' selector: 'keyed_element' inside: pattern: 'prometheus.CounterOpts{$$$}' stopBy: end constraints: V: not: regex: '_total"$' Key syntax discoveries (worth propagating to docs/ast-grep-rule-writing-guide.md): - pattern.context + selector lets you match a structural sub-node inside a parsed context. Required because bare 'Name: $X' parses as a labeled_statement, not a keyed_element. - inside with stopBy: end anchors on the enclosing literal so GaugeOpts / HistogramOpts / SummaryOpts (which share the Name field shape) don't false-flag. Confirmed on the fixture: zero FPs. - constraints at rule-top-level (sibling to 'rule:'), with 'not.regex' inside the metavariable spec. The standalone 'pattern + constraints' flat form does not support 'not' as a child of constraints.X. Edge cases verified: - NewCounterVec + NewCounter both share CounterOpts struct → both match - Name field position-agnostic (first / middle / last all flag if missing _total) - GaugeOpts / HistogramOpts / SummaryOpts with Name: 'x' do NOT flag - _test.go, main.go, vendor/, mocks/ ignored Doc updated: counter-total-suffix Enforcement field now points at the YAML instead of 'judgment (ast-grep follow-up)'. Index regenerated: the entry's enforcement field now reads 'rules/go/counter-total-suffix.yml'.
PR #11 cracked the ast-grep 0.43.0 recipe for struct-literal field matching after PR #4 and PR #8 both burned cycles iterating on zero-match patterns. Folding the discoveries into the guide so the next bootstrap PR doesn't re-learn them. New section 'Struct-literal field matching' covers the 3-piece recipe: 1. pattern.context + selector for sub-node targeting (bare 'Name: $X' parses as labeled_statement, not keyed_element — must wrap in context) 2. inside.pattern with stopBy: end for type anchoring (prevents GaugeOpts / HistogramOpts / SummaryOpts false-flags through the selector) 3. constraints at rule-top-level (NOT under rule.pattern.constraints), with not.regex inside the metavariable spec Pitfalls Learned grew by 3 entries documenting the specific traps: - 'Name: $X' parses as labeled_statement - pattern.context alone leaks to sibling types - constraints placement: top-level sibling of rule, not nested Canonical example entry added pointing to rules/go/counter-total-suffix.yml (landed in PR #11). 153 → 193 lines. No personal vault paths, no trading-domain terms — pre-emptive grep clean.
Restructures the 4 MUST sections in docs/go-licensing-guide.md into canonical `### RULE` blocks. Same template as PRs #2-5, #8, #10. Rules added: - go-licensing/license-file-required (MUST, judgment) Public Go repos MUST have a root LICENSE file. Private/internal repos exempt. Detection requires file-existence check — not ast-grep. - go-licensing/readme-license-section-required (MUST, judgment) Public README must have a ## License H2 section pointing to LICENSE. Markdown structure check — not ast-grep. - go-licensing/source-file-header-required (MUST, addlicense) All non-vendor *.go files need the 3-line BSD-2 header. Enforcement delegates to 'addlicense -check' via make precommit (canonical tool; already wired into the toolchain). - go-licensing/copyright-year-discipline (MUST, judgment) No bulk year-updates for trivial changes; no future/non-numeric years. PR-scope diff inspection — partial ast-grep detection possible for the future-year case but bulk-update trigger needs diff-scope reasoning. rules/index.json: 42 -> 46 Walker drift detected by PR #13's check-index target during first precommit run — regenerated rules/index.json and re-ran precommit clean. New guard working as designed. Pre-emptive checks (lessons from #6, #8): no personal vault paths, no trading-domain terms, no internal contradictions, all cross-refs resolve. License-assistant agent exists at agents/license-assistant.md.
Restructures the godoc style guide with canonical `### RULE` blocks for its enforceable conventions. Mirrors PRs #2-5, #8, #10, #14. Rules added (rules/index.json: 46 -> 50): - go-doc/exported-item-must-have-comment (MUST, judgment) Every exported function/type/const/var needs a doc comment. Mechanical follow-up: revive's 'exported' rule. - go-doc/comment-starts-with-name (MUST, judgment) Doc comment first word must match the identifier name. Mechanical follow-up: revive (or ast-grep via the PR #11 recipe). - go-doc/package-comment-in-doc-go (SHOULD, judgment) Package comment belongs in doc.go to survive refactors of feature files. - go-doc/third-person-no-signature-repeat (SHOULD, judgment) No first-person prose; don't restate the signature. All examples generic (Order, Customer, Discount, Add) — no trading domain. All rule IDs unique against the 46 existing entries. godoc-assistant agent exists at agents/godoc-assistant.md. Pre-emptive checks (lessons from PRs #6, #8, #14): - No personal vault paths - No trading-domain terms - All cross-refs resolve - make build-index regenerated; check-index target passes
Restructures the 6 enforceable conventions in docs/go-architecture-patterns.md into canonical `### RULE` blocks. Mirrors PRs #2-5, #8, #10, #14, #15. Rules added (rules/index.json: 50 -> 56, new go-architecture/* family): - go-architecture/counterfeiter-directive-on-interface (MUST) Every substitutable interface needs a //counterfeiter:generate directive so go generate regenerates the fake when the interface drifts. - go-architecture/new-prefix-constructor-naming (MUST) Constructors start with New (not Create/Make/etc). Partial overlap with go-factory/no-impl-in-factory-pkg for Create*. - go-architecture/constructor-returns-interface (MUST) New* returns the interface, not *concrete struct. Hides implementation, keeps dependency direction one-way. - go-architecture/private-struct-matches-interface (SHOULD) UserService -> userService (first letter lowered). godoc pairs them; IDE outlines pair them; refactors find them with one rename. - go-architecture/no-globals-or-singletons (MUST) Service deps via constructor injection, never package-level vars. Globals break parallel tests, hide the dep graph, make refactors fragile. - go-architecture/business-logic-not-in-main (MUST) main.go is wiring; domain operations live in pkg/. Mixed main.go is untestable and unreachable from other binaries. Cross-references to existing rules (already in index, not duplicated): - go-errors/no-context-background-in-business-logic — covers context discipline - go-context/cancel-check-in-loop — covers infinite-loop ctx.Done() checks - go-time/* family — covers libtime injection All examples generic (User, UserService, Worker, log/db deps). No trading-domain terms (KafkaBrokers grep hit is server infrastructure, not trading-broker terminology — pre-existing in main.go example). No personal vault paths. Pre-emptive checks (lessons from PRs #6, #8, #14, #15): clean grep, unique rule IDs (6 new vs 50 existing), go-architecture-assistant agent exists at agents/go-architecture-assistant.md, make build-index regenerated, check-index passes.
…nal-options) Second multi-doc bootstrap PR — same shape as PR #17. 3 docs (~500-770 lines), 6 rules, single bot review cycle. Rules added (rules/index.json: 62 -> 68): go-service-impl/* (owner: go-architecture-assistant) - no-context-object-injection (MUST) — never bundle deps in a *Context / *Deps struct passed through methods. Constructor injection makes the dep set visible at the type signature and minimal in scope. - provider-vs-registry-choice (SHOULD) — static switch for fixed compile-time sets (< ~10 types); map-based registry only for runtime extensible / plugin systems. go-k8s-crd/* (owner: go-architecture-assistant) - use-bborbe-k8s (SHOULD) — collapse ~300 lines of hand-written event-handler + adapter + store boilerplate into k8s.NewEventHandler[T] + k8s.NewResourceEventHandler[T] from github.com/bborbe/k8s. - generated-client-not-dynamic (MUST) — first-party CRDs use the typed clientset from hack/update-codegen.sh, not client-go/dynamic. Dynamic client is for unknown schemas (admin tools, generic operators); known-schema use throws away every type-safety guarantee. go-functional-options/* (owner: go-quality-assistant) - singular-option-type (SHOULD) — XxxOption (singular fn type) + XxxOptions (plural struct). Industry-standard pair-naming. - with-prefix-option-functions (SHOULD) — option constructors prefixed With*. Uniform prefix makes the option-list call site self-describing. CLAUDE.md doc-agent table updated with all 3 new mappings. Pre-emptive checks (lessons from PRs #6, #8, #14, #17): - No personal vault paths - One trading-domain leak introduced in my service-impl example (ProcessMarket/Limit/Stop — order-type terminology) caught by pre-push grep + genericised to ProcessImage/Video/Document. Pre-existing OrderType/MarketOrder/LimitOrder/StopOrder references in the original doc are NOT changed in this PR (out of scope; the bot may flag them as pre-existing). - All 6 rule IDs unique against 62 existing entries - All 3 owner agents exist - make build-index regenerated; check-index passes
Summary
Bootstraps `docs/go-prometheus-metrics-guide.md` — converts its 6 MUST/SHOULD sections into canonical `### RULE` blocks. Mirrors the template proven across PRs #2-5.
`rules/index.json`: 21 → 27 entries (6 new go-prometheus rules).
Rules added (all judgment for this PR)
ast-grep YAML deferred
The `counter-total-suffix` rule is mechanically enforceable in principle, but ast-grep 0.43.0's traversal of `CounterOpts` struct literals (composite_literal → literal_value → keyed_element → identifier + interpreted_string_literal) needs more debugging than this PR's scope. Drafted a YAML, smoke-tested locally, got zero matches on known-bad cases. Same trap PR #4 hit — punting to a focused follow-up rather than iterating in this PR.
The rule block itself is fully populated; only the `Enforcement:` field stays as `judgment` until the YAML lands.
Pre-emptive clean-up applied (lessons from PR #6)
Test plan