From 2def5dc5ef5e370830f6c87652180a87f6f41bf9 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 09:57:02 +0200 Subject: [PATCH 1/2] feat(prometheus): bootstrap 6 rule blocks in go-prometheus-metrics-guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- docs/go-prometheus-metrics-guide.md | 134 ++++++++++++++++++++-------- rules/index.json | 54 +++++++++++ 2 files changed, 153 insertions(+), 35 deletions(-) diff --git a/docs/go-prometheus-metrics-guide.md b/docs/go-prometheus-metrics-guide.md index 5f3e458..46f459c 100644 --- a/docs/go-prometheus-metrics-guide.md +++ b/docs/go-prometheus-metrics-guide.md @@ -17,15 +17,29 @@ Key principles: - Use interfaces for testability. - Pre-initialize counters with `.Add(0)` so absent series don't silently break alerts. -## Counter Pre-Initialization Pattern +### RULE go-prometheus/counter-pre-initialization (MUST) -**MUST pre-initialize counters with `.Add(0)` for all known label combinations.** This ensures metrics exist in Prometheus even when no events have occurred, preventing `absent()` alert false negatives. +**Owner**: go-metrics-assistant +**Applies when**: a CounterVec is registered for a label set whose value domain is known at compile time (enum, fixed slice of strings, etc.). +**Enforcement**: judgment +**Why**: Without pre-initialization, `rate(metric[5m])` returns *no data* (not zero) for unseen label combos. Alert expressions like `rate(errors_total[5m]) > 0.1` silently skip absent series instead of evaluating to false — so the alert never fires when the system is fine *and never fires when the system is broken either*. `absent()` checks don't save you because the series literally doesn't exist yet. + +#### Bad ```go func init() { - prometheus.MustRegister( - requestErrorTotal, - ) + prometheus.MustRegister(requestErrorTotal) + // No pre-init — "timeout" / "validation" / "internal" series don't exist + // until the first error of each type occurs. `absent(requestErrorTotal{reason="timeout"})` + // fires forever; `rate(requestErrorTotal[5m]) > 0.1` never fires. +} +``` + +#### Good + +```go +func init() { + prometheus.MustRegister(requestErrorTotal) // Pre-initialize all known label combinations to 0 for _, reason := range []string{"timeout", "validation", "internal"} { @@ -36,11 +50,27 @@ func init() { } ``` -**Why:** Without pre-initialization, `rate(metric[5m])` returns no data (not zero) for unseen label combos. Alert expressions like `rate(errors_total[5m]) > 0.1` silently skip absent series instead of evaluating to false. +### RULE go-prometheus/composed-metrics-interface (SHOULD) -## Composed Metrics Interface Pattern +**Owner**: go-metrics-assistant +**Applies when**: a service exposes a single `Metrics` interface aggregating more than ~6 methods across distinct functional domains (handlers, senders, schedulers, etc.). +**Enforcement**: judgment +**Why**: Interface Segregation Principle. Components that only send notifications should depend on `MetricsNotificationSender`, not the full `Metrics` interface. Narrow interfaces produce smaller Counterfeiter mocks, clearer test setup, and make accidental coupling visible at the type signature. -**MUST split large Metrics interfaces into focused sub-interfaces when a service has distinct metric domains.** Compose them into a single Metrics interface for the factory. +#### Bad + +```go +// One fat interface — every consumer pulls every method +type Metrics interface { + OrderHandleTotalCounterInc(tenant core.TenantID, product core.ProductID) + OrderHandleFailureCounterInc(tenant core.TenantID, product core.ProductID) + OrderHandleSuccessCounterInc(tenant core.TenantID, product core.ProductID) + NotificationSendTotalCounterInc(tenant core.TenantID, product core.ProductID, channel core.ChannelID) + NotificationSendFailureCounterInc(tenant core.TenantID, product core.ProductID, channel core.ChannelID) +} +``` + +#### Good ```go //counterfeiter:generate -o ../mocks/api-metrics.go --fake-name ApiMetrics . Metrics @@ -61,8 +91,6 @@ type MetricsNotificationSender interface { } ``` -**Why:** Components that only send notifications should depend on `MetricsNotificationSender`, not the full `Metrics` interface. Follows Interface Segregation Principle. - ## Metric Types & Design ### Choosing the Right Type @@ -74,18 +102,29 @@ type MetricsNotificationSender interface { | Need distribution / percentiles? | Histogram | — | | Need exact quantiles, can't define buckets? | Summary | Histogram | -**MUST NOT use GaugeVec for values that only increase.** If a metric only calls `.Inc()`, it MUST be a `CounterVec`. Using Gauge for monotonically increasing values breaks `rate()` and `increase()` queries. +### RULE go-prometheus/no-gauge-for-monotonic (MUST) + +**Owner**: go-metrics-assistant +**Applies when**: a `prometheus.NewGaugeVec` / `prometheus.NewGauge` registers a metric the code only ever increments (only `.Inc()` / `.Add(positive)` call sites, never `.Set()` / `.Dec()` / `.Sub()`). +**Enforcement**: judgment +**Why**: Using Gauge for monotonically increasing values breaks `rate()` and `increase()` queries — they assume the underlying type is a counter that can reset to zero on process restart, and they treat any decrease as a counter reset rather than an actual decrease. Dashboards silently produce nonsense. + +#### Bad ```go -// BAD: Gauge for counter-like metric -candleHandleTotalCounter = prometheus.NewGaugeVec(prometheus.GaugeOpts{ +// Gauge for counter-like metric — rate() and increase() return nonsense +orderHandleTotalCounter = prometheus.NewGaugeVec(prometheus.GaugeOpts{ Name: "total_counter", -}, []string{"broker"}) +}, []string{"tenant"}) +``` + +#### Good -// GOOD: Counter for values that only increase -candleHandleTotalCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ +```go +// Counter for values that only increase +orderHandleTotalCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "total", -}, []string{"broker"}) +}, []string{"tenant"}) ``` ### Counter — Monotonically Increasing @@ -164,53 +203,78 @@ Guidelines: - Be descriptive but concise. - Use consistent namespace/subsystem across related metrics. -### Counter `_total` Suffix Rule +### RULE go-prometheus/counter-total-suffix (MUST) -**MUST end counter metric names with `_total`.** Prometheus naming convention enforced by newer client versions. +**Owner**: go-metrics-assistant +**Applies when**: a `prometheus.CounterOpts` struct literal sets a `Name:` field whose string value does not end with `_total`. +**Enforcement**: judgment (mechanical ast-grep YAML tracked as follow-up; CounterOpts struct-literal traversal in ast-grep 0.43.0 needs further investigation) +**Why**: Prometheus naming convention; newer `client_golang` versions enforce this at registration time (panic). Counters without `_total` also fail the OpenMetrics spec and confuse Grafana auto-completion. + +#### Bad ```go -// BAD -Name: "requests_processed", -Name: "errors_count", +prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "requests_processed", // missing _total + Help: "...", +}, []string{"method"}) +``` -// GOOD -Name: "requests_processed_total", -Name: "errors_total", +#### Good + +```go +prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "requests_processed_total", + Help: "...", +}, []string{"method"}) ``` -### Help String Quality Rule +### RULE go-prometheus/help-string-quality (MUST) + +**Owner**: go-metrics-assistant +**Applies when**: any `prometheus.{Counter,Gauge,Histogram,Summary}Opts` struct literal sets a `Help:` field that (a) is empty, (b) duplicates another metric's Help verbatim, or (c) describes a different metric (copy-paste residue). +**Enforcement**: judgment +**Why**: Help strings appear in `/metrics` output and the Grafana metric explorer. Wrong descriptions cause real confusion during incidents — the on-call sees a Help string that contradicts the metric name, can't tell which is wrong, and burns minutes verifying. Empty Help strings make alert ownership ambiguous. -**MUST write unique, accurate Help strings for every metric.** Never copy-paste Help from another metric. Help strings appear in `/metrics` output and Grafana metric explorer — wrong descriptions cause confusion during incidents. +#### Bad ```go -// BAD: Copy-pasted Help notificationSendSuccessCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "success_total", Help: "Order Handle Total Counter", // Wrong! This is the notification sender }) +``` -// GOOD +#### Good + +```go notificationSendSuccessCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "success_total", Help: "Total number of successfully sent notifications", }) ``` -### Label Naming Consistency +### RULE go-prometheus/label-naming-consistency (MUST) + +**Owner**: go-metrics-assistant +**Applies when**: two or more metrics in the same project reference the same conceptual entity using different label names (e.g. `product` vs `item` for product ID; `tenant` vs `customer` vs `org`). +**Enforcement**: judgment +**Why**: Inconsistent label names silently break PromQL joins (`on(product)` only joins series that share the label) and Grafana dashboards (variable interpolation can't unify across panels). The cost shows up at 3am when a dashboard is half-empty and no one knows why. -**MUST use the same label name for the same concept across all metrics in a project.** Inconsistent label names break dashboards and PromQL joins. +#### Bad ```go -// BAD orderHandleCounter.With(prometheus.Labels{"product": product.String()}) -notificationSendCounter.With(prometheus.Labels{"item": product.String()}) +notificationSendCounter.With(prometheus.Labels{"item": product.String()}) // same concept, different label +``` -// GOOD +#### Good + +```go orderHandleCounter.With(prometheus.Labels{"product": product.String()}) notificationSendCounter.With(prometheus.Labels{"product": product.String()}) ``` -Define label-name constants to enforce consistency: +Define label-name constants to enforce consistency at compile time: ```go const ( diff --git a/rules/index.json b/rules/index.json index c7abfa1..2d55174 100644 --- a/rules/index.json +++ b/rules/index.json @@ -125,6 +125,60 @@ "level": "MUST", "owner": "go-http-handler-assistant" }, + { + "anchor": "go-prometheus/composed-metrics-interface", + "applies_when": "a service exposes a single `Metrics` interface aggregating more than ~6 methods across distinct functional domains (handlers, senders, schedulers, etc.).", + "doc_path": "docs/go-prometheus-metrics-guide.md", + "enforcement": "judgment", + "id": "go-prometheus/composed-metrics-interface", + "level": "SHOULD", + "owner": "go-metrics-assistant" + }, + { + "anchor": "go-prometheus/counter-pre-initialization", + "applies_when": "a CounterVec is registered for a label set whose value domain is known at compile time (enum, fixed slice of strings, etc.).", + "doc_path": "docs/go-prometheus-metrics-guide.md", + "enforcement": "judgment", + "id": "go-prometheus/counter-pre-initialization", + "level": "MUST", + "owner": "go-metrics-assistant" + }, + { + "anchor": "go-prometheus/counter-total-suffix", + "applies_when": "a `prometheus.CounterOpts` struct literal sets a `Name:` field whose string value does not end with `_total`.", + "doc_path": "docs/go-prometheus-metrics-guide.md", + "enforcement": "judgment (mechanical ast-grep YAML tracked as follow-up; CounterOpts struct-literal traversal in ast-grep 0.43.0 needs further investigation)", + "id": "go-prometheus/counter-total-suffix", + "level": "MUST", + "owner": "go-metrics-assistant" + }, + { + "anchor": "go-prometheus/help-string-quality", + "applies_when": "any `prometheus.{Counter,Gauge,Histogram,Summary}Opts` struct literal sets a `Help:` field that (a) is empty, (b) duplicates another metric's Help verbatim, or (c) describes a different metric (copy-paste residue).", + "doc_path": "docs/go-prometheus-metrics-guide.md", + "enforcement": "judgment", + "id": "go-prometheus/help-string-quality", + "level": "MUST", + "owner": "go-metrics-assistant" + }, + { + "anchor": "go-prometheus/label-naming-consistency", + "applies_when": "two or more metrics in the same project reference the same conceptual entity using different label names (e.g. `product` vs `item` for product ID; `tenant` vs `customer` vs `org`).", + "doc_path": "docs/go-prometheus-metrics-guide.md", + "enforcement": "judgment", + "id": "go-prometheus/label-naming-consistency", + "level": "MUST", + "owner": "go-metrics-assistant" + }, + { + "anchor": "go-prometheus/no-gauge-for-monotonic", + "applies_when": "a `prometheus.NewGaugeVec` / `prometheus.NewGauge` registers a metric the code only ever increments (only `.Inc()` / `.Add(positive)` call sites, never `.Set()` / `.Dec()` / `.Sub()`).", + "doc_path": "docs/go-prometheus-metrics-guide.md", + "enforcement": "judgment", + "id": "go-prometheus/no-gauge-for-monotonic", + "level": "MUST", + "owner": "go-metrics-assistant" + }, { "anchor": "go-security/chmod-return-checked", "applies_when": "an `os.Chmod($PATH, $PERM)` call in a `*.go` file outside `*_test.go` and `vendor/` whose return value is discarded (no `if err := os.Chmod(...); err != nil` wrapper, no `_ = os.Chmod(...)` with an explanatory comment). Detecting \"return value used in error check\" requires reading the surrounding statement — pure ast-grep cannot reliably distinguish a checked `os.Chmod(...)` from an unchecked one without false positives.", From 6cf814d75fb81293e11b635e4f9f6944b1c4e809 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 10:05:53 +0200 Subject: [PATCH 2/2] fix(prometheus): address bot review on PR #8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/go-prometheus-metrics-guide.md | 29 +++++++++++++++++++++++------ rules/index.json | 6 +++--- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/docs/go-prometheus-metrics-guide.md b/docs/go-prometheus-metrics-guide.md index 46f459c..c41bd80 100644 --- a/docs/go-prometheus-metrics-guide.md +++ b/docs/go-prometheus-metrics-guide.md @@ -20,7 +20,7 @@ Key principles: ### RULE go-prometheus/counter-pre-initialization (MUST) **Owner**: go-metrics-assistant -**Applies when**: a CounterVec is registered for a label set whose value domain is known at compile time (enum, fixed slice of strings, etc.). +**Applies when**: a CounterVec is registered for a label set whose value domain is small, bounded, and known at compile time (typically < 20 combinations — enum, fixed slice of strings, etc.). For large or unbounded domains, prefer `absent()` checks in alerting rules instead. **Enforcement**: judgment **Why**: Without pre-initialization, `rate(metric[5m])` returns *no data* (not zero) for unseen label combos. Alert expressions like `rate(errors_total[5m]) > 0.1` silently skip absent series instead of evaluating to false — so the alert never fires when the system is fine *and never fires when the system is broken either*. `absent()` checks don't save you because the series literally doesn't exist yet. @@ -53,7 +53,7 @@ func init() { ### RULE go-prometheus/composed-metrics-interface (SHOULD) **Owner**: go-metrics-assistant -**Applies when**: a service exposes a single `Metrics` interface aggregating more than ~6 methods across distinct functional domains (handlers, senders, schedulers, etc.). +**Applies when**: a single `Metrics` interface aggregates methods spanning two or more distinct functional domains (handlers + senders + schedulers + …), forcing consumers to depend on methods they don't use. **Enforcement**: judgment **Why**: Interface Segregation Principle. Components that only send notifications should depend on `MetricsNotificationSender`, not the full `Metrics` interface. Narrow interfaces produce smaller Counterfeiter mocks, clearer test setup, and make accidental coupling visible at the type signature. @@ -107,14 +107,14 @@ type MetricsNotificationSender interface { **Owner**: go-metrics-assistant **Applies when**: a `prometheus.NewGaugeVec` / `prometheus.NewGauge` registers a metric the code only ever increments (only `.Inc()` / `.Add(positive)` call sites, never `.Set()` / `.Dec()` / `.Sub()`). **Enforcement**: judgment -**Why**: Using Gauge for monotonically increasing values breaks `rate()` and `increase()` queries — they assume the underlying type is a counter that can reset to zero on process restart, and they treat any decrease as a counter reset rather than an actual decrease. Dashboards silently produce nonsense. +**Why**: `rate()` and `increase()` are type-agnostic — they interpret *any* downward movement in the sample series as a counter reset and adjust accordingly. With a Gauge, a legitimate decrease (e.g. queue drains) is treated as a reset, producing nonsense rates. With a Counter, the type signals that the value can only increase, so PromQL's reset detection is sound. Dashboards built on a Gauge-used-as-counter silently produce wrong numbers. #### Bad ```go // Gauge for counter-like metric — rate() and increase() return nonsense orderHandleTotalCounter = prometheus.NewGaugeVec(prometheus.GaugeOpts{ - Name: "total_counter", + Name: "order_handle_total", }, []string{"tenant"}) ``` @@ -123,7 +123,7 @@ orderHandleTotalCounter = prometheus.NewGaugeVec(prometheus.GaugeOpts{ ```go // Counter for values that only increase orderHandleTotalCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ - Name: "total", + Name: "order_handle_total", }, []string{"tenant"}) ``` @@ -207,7 +207,7 @@ Guidelines: **Owner**: go-metrics-assistant **Applies when**: a `prometheus.CounterOpts` struct literal sets a `Name:` field whose string value does not end with `_total`. -**Enforcement**: judgment (mechanical ast-grep YAML tracked as follow-up; CounterOpts struct-literal traversal in ast-grep 0.43.0 needs further investigation) +**Enforcement**: judgment (ast-grep follow-up) **Why**: Prometheus naming convention; newer `client_golang` versions enforce this at registration time (panic). Counters without `_total` also fail the OpenMetrics spec and confuse Grafana auto-completion. #### Bad @@ -238,6 +238,23 @@ prometheus.NewCounterVec(prometheus.CounterOpts{ #### Bad ```go +// Empty Help — useless in /metrics and Grafana +emptyHelpCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "requests_total", + Help: "", +}, []string{"method"}) + +// Duplicate Help across two distinct metrics — collapses to one entry in the explorer +orderHandleTotalCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "order_handle_total", + Help: "Total number of operations", +}, []string{"tenant"}) +notificationSendTotalCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "notification_send_total", + Help: "Total number of operations", // identical Help, different metric +}, []string{"tenant"}) + +// Copy-paste residue — Help describes the wrong metric notificationSendSuccessCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "success_total", Help: "Order Handle Total Counter", // Wrong! This is the notification sender diff --git a/rules/index.json b/rules/index.json index 2d55174..20e05fc 100644 --- a/rules/index.json +++ b/rules/index.json @@ -127,7 +127,7 @@ }, { "anchor": "go-prometheus/composed-metrics-interface", - "applies_when": "a service exposes a single `Metrics` interface aggregating more than ~6 methods across distinct functional domains (handlers, senders, schedulers, etc.).", + "applies_when": "a single `Metrics` interface aggregates methods spanning two or more distinct functional domains (handlers + senders + schedulers + …), forcing consumers to depend on methods they don't use.", "doc_path": "docs/go-prometheus-metrics-guide.md", "enforcement": "judgment", "id": "go-prometheus/composed-metrics-interface", @@ -136,7 +136,7 @@ }, { "anchor": "go-prometheus/counter-pre-initialization", - "applies_when": "a CounterVec is registered for a label set whose value domain is known at compile time (enum, fixed slice of strings, etc.).", + "applies_when": "a CounterVec is registered for a label set whose value domain is small, bounded, and known at compile time (typically < 20 combinations — enum, fixed slice of strings, etc.). For large or unbounded domains, prefer `absent()` checks in alerting rules instead.", "doc_path": "docs/go-prometheus-metrics-guide.md", "enforcement": "judgment", "id": "go-prometheus/counter-pre-initialization", @@ -147,7 +147,7 @@ "anchor": "go-prometheus/counter-total-suffix", "applies_when": "a `prometheus.CounterOpts` struct literal sets a `Name:` field whose string value does not end with `_total`.", "doc_path": "docs/go-prometheus-metrics-guide.md", - "enforcement": "judgment (mechanical ast-grep YAML tracked as follow-up; CounterOpts struct-literal traversal in ast-grep 0.43.0 needs further investigation)", + "enforcement": "judgment (ast-grep follow-up)", "id": "go-prometheus/counter-total-suffix", "level": "MUST", "owner": "go-metrics-assistant"