Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
status: accepted
updated_at: '2026-05-07T00:00:00Z'
updated_by: mashkovd
notes: |
Operator-seeded proposal (skipping mentor digest). Phase 4 of the
incident auto-cleanup track. Phases 1/2/3 shipped in 1.8.0/1.9.0/
1.10.0; Phase 4 wires Prometheus metrics so the cleanup activity is
visible in Grafana and alertable. Mirrors the mctl-api standalone
ServiceMonitor pattern (port http, path /metrics) — does not
require base-service chart changes.

Cardinality kept low (status / source / reason / outcome only — no
tenant or service labels) so the time series count stays bounded
with cluster scale.
Original file line number Diff line number Diff line change
@@ -0,0 +1,278 @@
# Design: incident-auto-cleanup-phase4-metrics

## Current state

`mctl-agent` does not expose Prometheus metrics. `go.mod` does not
import `prometheus/client_golang`. The chi router at
`internal/api/router.go` only mounts a narrow JSON metrics endpoint
for skill introspection at `/api/v1/skills/{name}/metrics`.

`mctl-api` shows the pattern we will follow:

- `internal/api/router.go:31-32` imports `prometheus/client_golang`
and `prometheus/client_golang/promhttp`.
- Line 108 mounts `r.Handle("/metrics", promhttp.Handler())` directly
on the same router, on the same port (8080).
- Lines 256-262 exempt `/metrics` from the per-request audit-logging
middleware so scrape requests do not pollute the audit trail.
- gitops `bootstrap/templates/mctl-platform/mctl-api-monitor.yaml`
defines a standalone `ServiceMonitor` that scrapes port `http`
(8080) on path `/metrics` every 30s.

## Proposed solution

### Part 1: Wire `prometheus/client_golang` in mctl-agent

`go.mod` gains `github.com/prometheus/client_golang` as a direct
dependency. Run `go mod tidy` in the implementer step.

A new package `internal/metrics` defines the registry and the
metric handles:

```go
package metrics

import (
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
)

var (
StaleTTLResolved = promauto.NewCounterVec(
prometheus.CounterOpts{
Name: "mctl_agent_stale_ttl_resolved_total",
Help: "Tickets auto-resolved by the stale-TTL GC, by previous status.",
},
[]string{"status"},
)

OrphanPruned = promauto.NewCounter(
prometheus.CounterOpts{
Name: "mctl_agent_orphan_pruned_total",
Help: "Tickets auto-resolved by orphan pruning (service no longer in inventory).",
},
)

AMReconcileResolved = promauto.NewCounter(
prometheus.CounterOpts{
Name: "mctl_agent_am_reconcile_resolved_total",
Help: "Tickets auto-resolved by AlertManager fingerprint reconciliation.",
},
)

CleanupSkipped = promauto.NewCounterVec(
prometheus.CounterOpts{
Name: "mctl_agent_cleanup_skipped_total",
Help: "Cleanup passes short-circuited by safety guards.",
},
[]string{"reason"},
)

AMRequestDuration = promauto.NewHistogramVec(
prometheus.HistogramOpts{
Name: "mctl_agent_am_request_duration_seconds",
Help: "AlertManager /api/v2/alerts request duration.",
Buckets: prometheus.DefBuckets, // 5ms..10s; AM should land well within
},
[]string{"outcome"},
)

OpenTickets = promauto.NewGaugeVec(
prometheus.GaugeOpts{
Name: "mctl_agent_open_tickets",
Help: "Non-terminal tickets by status and source.",
},
[]string{"status", "source"},
)
)

// PreRegister forces label combinations to appear at zero on first
// scrape so that rate() / increase() queries cover the full series
// from t=0 instead of only after the first increment.
func PreRegister() {
for _, st := range []string{"open", "analyzing", "fix_proposed"} {
StaleTTLResolved.WithLabelValues(st).Add(0)
}
for _, r := range []string{"empty_inventory", "am_unknown", "am_empty_set", "am_fetch_error"} {
CleanupSkipped.WithLabelValues(r).Add(0)
}
for _, o := range []string{"success", "http_error", "decode_error", "transport_error"} {
AMRequestDuration.WithLabelValues(o).Observe(0) // emits an initial histogram bucket entry
}
}
```

Counters and label sets stay tight — high-cardinality labels (tenant,
service, alertname) are deliberately excluded to avoid Prometheus
cardinality explosions. Tenant breakdown for orphan/AM resolutions
goes to logs, not metrics.

### Part 2: Mount `/metrics` and wire callers

`internal/api/router.go`:

```go
r.Handle("/metrics", promhttp.Handler())
```

mounted at the top level. Existing handlers untouched.

Each cleanup pass increments its counter where it currently emits its
`slog.Info` log line:

- `internal/monitor/poller.go` `resolveStale`:
- On successful resolve: `metrics.StaleTTLResolved.WithLabelValues(string(t.Status)).Inc()`
- On `state.allUnknown` short-circuit (existing log absent — add one): `metrics.CleanupSkipped.WithLabelValues("am_unknown").Inc()` — wait, that's the orphan-prune path. The stale-TTL pass has no allUnknown gate today. Skip in this pass.
- `internal/monitor/poller.go` `pruneOrphans`:
- Empty inventory guard:
`metrics.CleanupSkipped.WithLabelValues("empty_inventory").Inc()`
- allUnknown short-circuit (no log today; add `slog.Warn` AND
counter): `metrics.CleanupSkipped.WithLabelValues("am_unknown").Inc()`.
Note: the existing code returns silently here; this proposal adds
the log line for symmetry with the empty-inventory guard.
- On successful resolve: `metrics.OrphanPruned.Inc()`
- `internal/monitor/poller.go` `reconcileWithAlertManager`:
- Empty active set: `metrics.CleanupSkipped.WithLabelValues("am_empty_set").Inc()`
- Fetch error: `metrics.CleanupSkipped.WithLabelValues("am_fetch_error").Inc()`
- On successful resolve: `metrics.AMReconcileResolved.Inc()`
- `internal/monitor/alertmanager_client.go` `ActiveFingerprints`:
- Wrap the request with `time.Now()` then
`metrics.AMRequestDuration.WithLabelValues(outcome).Observe(time.Since(start).Seconds())`
where outcome is determined by the error path taken.

For the open-tickets gauge: extend the poll cycle (after the three
cleanup passes complete) with a call to `store.OpenTicketBreakdown()`
returning a `map[StatusSourcePair]int`. The metric is reset and
re-set on each cycle to handle decreasing counts:

```go
metrics.OpenTickets.Reset()
for k, v := range breakdown {
metrics.OpenTickets.WithLabelValues(k.Status, k.Source).Set(float64(v))
}
```

Add a small store helper that runs a single grouped SQL query:

```go
func (s *Store) OpenTicketBreakdown() (map[StatusSourcePair]int, error) {
// SELECT status, source, COUNT(*) FROM tickets
// WHERE status NOT IN ('resolved', 'suppressed')
// GROUP BY status, source
}
```

`PreRegister()` is called in `cmd/agent/main.go` right after the
metrics package is first imported (effectively when `init()` runs
under promauto), and an explicit call is made in main to lock in the
zero-baseline labels listed in Part 1.

### Part 3: ServiceMonitor in gitops

New file
`platform-gitops/bootstrap/templates/mctl-platform/mctl-agent-monitor.yaml`,
mirroring `mctl-api-monitor.yaml`:

```yaml
apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
name: mctl-agent
namespace: monitoring
labels:
release: monitoring
spec:
selector:
matchLabels:
app.kubernetes.io/instance: admins-mctl-agent
app.kubernetes.io/name: base-service
namespaceSelector:
matchNames:
- admins
endpoints:
- port: http
path: /metrics
interval: 30s
```

The base-service Helm chart's existing `metrics.enabled` toggle is
**not** used — that template expects a separate `metrics` port
(default 9090) and would require service+container changes. Mirroring
the mctl-api standalone-ServiceMonitor pattern is simpler and matches
existing operator habit.

### Part 4: Tests

- `internal/metrics/metrics_test.go` — verify all expected metrics
are registered, label sets match the AC, and `PreRegister()` emits
zero-baseline series.
- `internal/monitor/poller_test.go` — extend each existing
Phase 1/2/3 happy-path test to assert the corresponding counter
increments by 1 after the resolution call. Use
`testutil.ToFloat64()` from `client_golang/prometheus/testutil`.
- `internal/monitor/poller_test.go` — extend the existing guard
tests (empty inventory, AM error, AM empty set) to assert the
matching `cleanup_skipped_total` counter increments.
- `internal/api/router_test.go` (or new) — `GET /metrics` returns
200 with the expected Content-Type and contains
`mctl_agent_stale_ttl_resolved_total` in the response body.

## Alternatives

### (a) Use OpenTelemetry instead of Prometheus

OTLP push to a collector is more modern but does not match the
existing cluster setup (VictoriaMetrics scrapes Prometheus
endpoints). Adding an OTLP collector hop is overkill for one
service. Rejected.

### (b) Mount `/metrics` on a separate port (9090)

Matches the base-service chart's defaults but requires Service /
Deployment / NetworkPolicy changes. The mctl-api precedent
(everything on 8080) is good enough and cluster scrape rules
already trust that port. Rejected.

### (c) Use `promauto.With(reg)` to register against a
non-default registry

Provides cleaner test isolation but introduces the burden of
plumbing the registry through every code path that emits a metric.
Default registry is fine for a single-binary agent. Rejected.

### (d) Add high-cardinality labels (tenant, service)

Useful for ad-hoc Grafana drilldowns but creates one time series per
(tenant, service, status) tuple. With ~30 deployed services, the
count is bounded today but grows linearly with cluster scale.
Logging keeps the data accessible without the cardinality cost.
Rejected for v1; revisit if log-based aggregation proves slow.

## Platform impact

- **Database:** none; the new `OpenTicketBreakdown` helper is a
read-only `SELECT ... GROUP BY` that runs once per poll cycle.
- **API:** one new route `/metrics` exposing the standard Prometheus
text format on port 8080. Does not require authentication.
- **Network:** one extra scrape per 30s from the cluster's
VictoriaMetrics scraper. Response size grows linearly with the
Phase 1/2/3 metric count (≈10 KB at steady state).
- **Memory / CPU:** Prometheus collectors are essentially free (<1
MB RSS, microseconds per increment). The poll-cycle gauge update
adds one extra grouped SQL query per cycle — sub-millisecond on
any reasonable ticket count.
- **Observability:** the agent becomes scrapable; an obvious
follow-up is a Grafana dashboard JSON in
`platform-gitops/observability-stack/dashboards/`. Not in this PR.
- **Configuration:** none; metrics are always-on. If a future need
arises to disable `/metrics`, a `METRICS_ENABLED` env var can be
added then.
- **Backwards compatibility:** existing routes, log lines, and CLI
flags are unchanged. The new `/metrics` endpoint is additive.
- **Failure modes:**
- The metrics package never panics on increment / observe —
`client_golang` is internally safe under concurrent use.
- If the metrics registry double-registers on hot reload (does
not currently happen — the binary restarts on Helm upgrade),
`promauto` will panic at init time. Acceptable: hot reload is
not a feature we support.
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# Prometheus metrics for incident auto-cleanup

## Context

The incident auto-cleanup track shipped in three phases (mctl-agent
1.8.0 / 1.9.0 / 1.10.0). Each phase adds a poll-cycle pass — Phase 1
stale-TTL GC, Phase 3 orphan pruning, Phase 2 AlertManager
fingerprint reconciliation — and emits structured `slog` lines on
resolution and on guard activation. None of those passes are
currently visible to the metrics stack.

Operationally this is fine for spot-checks (`kubectl logs | grep
poller:`) but inadequate for trend monitoring:

- Counts by reason (resolved by stale TTL vs orphan vs AM reconcile)
cannot be aggregated over days from logs without a parser.
- Empty-inventory guard activations on every cycle (visible today
in Loki) cannot be alerted on.
- AM call failure rate / latency cannot be seen at all — silent on
success, slog.Warn on error, no histogram of HTTP duration.
- Open-ticket counts by status / source — already in the DB but
exposing as a gauge gives a free single-glass dashboard.

mctl-api already publishes Prometheus metrics on `/metrics` over the
service's main port (8080); the cluster's VictoriaMetrics k8s-stack
scrapes it via a standalone `ServiceMonitor` at
`bootstrap/templates/mctl-platform/mctl-api-monitor.yaml`. mctl-agent
does **not** wire `prometheus/client_golang` and exposes only a
narrow `/api/v1/skills/{name}/metrics` JSON endpoint for skill-level
introspection. There is no `/metrics` Prometheus endpoint.

This proposal adds a standard Prometheus metrics surface to
mctl-agent and a sibling ServiceMonitor in gitops, mirroring the
mctl-api pattern.

## User stories

- AS an on-call SRE I WANT a Grafana panel showing the rate of
auto-resolved incidents per reason (stale TTL by status, orphan,
AM reconcile) SO THAT I can confirm the cleanup track is actually
doing work and detect regressions.
- AS a platform engineer I WANT to alert on `mctl_agent_open_tickets`
staying above a threshold SO THAT a wedged self-healing pipeline
(no resolutions, ticket count climbing) becomes visible without me
watching logs.
- AS a future maintainer I WANT a histogram of AlertManager call
duration SO THAT I can spot when AM becomes slow before it starts
timing out.
- AS an operator I WANT every metric to follow the established
naming conventions (`mctl_agent_*`, snake_case, `_total` suffix on
counters) SO THAT existing Grafana dashboards and alerting
templates remain consistent.

## Acceptance criteria (EARS)

- WHEN the binary starts, THE SYSTEM SHALL register a default
Prometheus collector and expose it at `GET /metrics` on the same
HTTP port as the existing API (8080).
- WHEN a ticket is auto-resolved by `resolveStale()`, THE SYSTEM
SHALL increment `mctl_agent_stale_ttl_resolved_total{status}`
exactly once with the ticket's previous status as the label
value (`open`, `analyzing`, or `fix_proposed`).
- WHEN a ticket is auto-resolved by `pruneOrphans()`, THE SYSTEM
SHALL increment `mctl_agent_orphan_pruned_total` exactly once.
- WHEN a ticket is auto-resolved by `reconcileWithAlertManager()`,
THE SYSTEM SHALL increment `mctl_agent_am_reconcile_resolved_total`
exactly once.
- WHEN any of the three passes is short-circuited by a guard
(`empty_inventory`, `am_unknown`, `am_empty_set`,
`am_fetch_error`), THE SYSTEM SHALL increment
`mctl_agent_cleanup_skipped_total{reason}` with the matching
reason label.
- WHILE the AlertManager client issues a request, THE SYSTEM SHALL
observe the request duration in
`mctl_agent_am_request_duration_seconds{outcome}` where outcome
is `success`, `http_error`, `decode_error`, or `transport_error`.
- WHILE the agent runs, THE SYSTEM SHALL update a gauge
`mctl_agent_open_tickets{status, source}` reflecting the count of
non-terminal tickets by status and source. Update cadence: on
every poll cycle (after the cleanup passes have run), so the gauge
reflects post-cleanup state.
- WHEN scraping `GET /metrics`, THE SYSTEM SHALL return a 200
response with `text/plain; version=0.0.4` Content-Type and SHALL
NOT require authentication (existing convention; Phase 4 inherits
the same router setup).
- WHEN counters are first exposed (after a fresh restart), THE
SYSTEM SHALL register them with their full label sets so the
metric appears at zero rather than only after the first
increment, allowing rate() queries from t=0.

## Out of scope

- Grafana dashboard JSON / alerting rules — separate concern, can be
authored once metrics are flowing. Mention a short follow-up note
in `design.md` but do not include in this PR.
- Skill-level metrics already exposed at
`/api/v1/skills/{name}/metrics` (JSON snapshot) — keep as-is for
now; converting to Prometheus format is a separate proposal.
- Authentication / authorization on `/metrics` — cluster network
policy already restricts ingress; matches mctl-api precedent.
- Push-mode (Pushgateway) — pull-only via the existing scrape path.
- HTTP request metrics for the API surface (request count, latency
by route) — would be useful but is an orthogonal addition; this
proposal stays focused on the cleanup-track observability.
Loading