diff --git a/docs/go-prometheus-metrics-guide.md b/docs/go-prometheus-metrics-guide.md index 5f3e458..c41bd80 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 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. + +#### 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 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. -**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**: `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 -// BAD: Gauge for counter-like metric -candleHandleTotalCounter = prometheus.NewGaugeVec(prometheus.GaugeOpts{ - Name: "total_counter", -}, []string{"broker"}) - -// GOOD: Counter for values that only increase -candleHandleTotalCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ - Name: "total", -}, []string{"broker"}) +// Gauge for counter-like metric — rate() and increase() return nonsense +orderHandleTotalCounter = prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Name: "order_handle_total", +}, []string{"tenant"}) +``` + +#### Good + +```go +// Counter for values that only increase +orderHandleTotalCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "order_handle_total", +}, []string{"tenant"}) ``` ### Counter — Monotonically Increasing @@ -164,53 +203,95 @@ Guidelines: - Be descriptive but concise. - Use consistent namespace/subsystem across related metrics. -### Counter `_total` Suffix Rule +### RULE go-prometheus/counter-total-suffix (MUST) + +**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 (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. -**MUST end counter metric names with `_total`.** Prometheus naming convention enforced by newer client versions. +#### 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 +// 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 }) +``` -// 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..20e05fc 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 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", + "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 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", + "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 (ast-grep follow-up)", + "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.",