Skip to content

feat(relay): prometheus metrics registry scaffolding (#59)#62

Merged
ilmoniemi merged 4 commits into
mainfrom
feature/59
May 12, 2026
Merged

feat(relay): prometheus metrics registry scaffolding (#59)#62
ilmoniemi merged 4 commits into
mainfrom
feature/59

Conversation

@ilmoniemi
Copy link
Copy Markdown
Contributor

What

Adopt github.com/prometheus/client_golang (per ADR-0008, landed in the spec commit on this branch) and stand up the relay's private metrics registry + handler factory.

  • internal/relay/metrics.go:
    • NewMetricsRegistry() *prometheus.Registry — fresh registry per call (no singleton; mirrors NewRegistry()'s test-friendly shape).
    • NewMetricsHandler(reg) http.Handler — wraps promhttp.HandlerFor(reg, …) following the factory pattern from NewHealthzHandler (relay: /healthz returns version + connected-binary count + connected-phone count (JSON) #10). HandlerOpts.Registry: reg routes promhttp's own self-instrumentation counters onto the private registry, never DefaultRegisterer (ADR-0008 § Scope of use).
  • go.mod / go.sum — direct dep github.com/prometheus/client_golang@v1.23.2; transitives client_model, common promoted to direct because tests import them; remaining transitives (perks, xxhash, goautoneg, procfs, protobuf, …) are indirect.

Scaffolding only — no counters, no gauges, no listener wiring. Those land in #57, #58, #60.

Issue

Closes #59.

Testing

Three unit tests in internal/relay/metrics_test.go:

  • TestMetricsHandler_EmptyRegistry_ResponseShape — the explicit AC: HTTP 200, Content-Type prefix-matches text/plain; version=0.0.4; charset=utf-8, body parseable by expfmt's text decoder.
  • TestMetricsHandler_RegisterAndScrape_RoundTrip — registers a trivial counter against the registry, scrapes, asserts the counter+value appears in the body.
  • TestMetricsRegistry_NoGlobalRegistrarLeak — structural defence for ADR-0008's "never DefaultRegisterer" rule: asserts zero-delta on DefaultGatherer.Gather() across registry+handler construction.

Gates clean:

  • make vet
  • make test (-race)
  • make build

Architecture compliance

  • Follows the spec's chosen seam shape: per-call registry construction; per-concern collector struct (defined by sibling tickets in their own files, not this one).
  • HandlerOpts.Registry: reg is set deliberately — spec rationale matches the ADR-0008 § Scope of use rule "DefaultRegisterer is never touched", including for promhttp's own bookkeeping. Documented in the metrics.go docstring.
  • The handler does no content negotiation toward OpenMetrics (EnableOpenMetrics left default false), matching /healthz's no-negotiation posture.

Deviation from spec test as-written, with rationale

Two test items in the spec couldn't be implemented literally:

  1. Content-Type exact-equality assertionprometheus/common v0.62+ appends ; escaping=<scheme> (default underscores) to the Content-Type as part of Prometheus 3.0 UTF-8 name support. The suffix is additive and backward-compatible. The spec explicitly designates the test literal as a "canary" for format changes; the canary fired and the change is benign. I converted exact-equality to prefix-match on the AC literal text/plain; version=0.0.4; charset=utf-8, keeping the literal as a substring (so a non-additive change still surfaces as a failure). Rationale in the test's textFormatPrefix doc comment.
  2. Zero-MetricFamily assertion on an "empty" registryHandlerOpts.Registry: reg (a load-bearing design choice the spec keeps) means the registry isn't empty after NewMetricsHandler: promhttp registers its own self-instrumentation collectors on it. I dropped the families == 0 assertion; the "parseable body" half of the AC (decoder reaches io.EOF without an error) is preserved. Rationale in the test's inline comment.

Neither deviation weakens the AC; both make the test internally consistent with the spec's own design choices.

Out of scope

🤖 Generated with Claude Code

ilmoniemi and others added 3 commits May 11, 2026 12:27
Adopt github.com/prometheus/client_golang (per ADR-0008) and stand up the
relay's private metrics registry + handler factory. Scaffolding only — no
counters, no gauges, no listener wiring (those land in #57, #58, #60).

`NewMetricsRegistry()` returns a fresh `*prometheus.Registry`; per-call
construction matches the existing `NewRegistry()` testability shape and lets
each test build an independent registry without process-wide collisions.

`NewMetricsHandler(reg)` returns `http.Handler` wrapping
`promhttp.HandlerFor(reg, …)`. `HandlerOpts.Registry: reg` is load-bearing:
it routes promhttp's own self-instrumentation collectors
(`promhttp_metric_handler_*`) onto the relay's private registry rather than
`prometheus.DefaultRegisterer` — that's how the ADR-0008 § Scope of use
"never the default registerer" rule holds even for the library's internal
bookkeeping.

Three unit tests:

- `TestMetricsHandler_EmptyRegistry_ResponseShape` covers the AC: HTTP 200,
  `Content-Type` starts with `text/plain; version=0.0.4; charset=utf-8`,
  body parseable by `expfmt`'s text decoder to `io.EOF`. The Content-Type
  is substring-matched because prometheus/common v0.62+ appends an
  additive `; escaping=<scheme>` suffix (UTF-8 names support) which the
  AC literal predates — the substring is still the spec-author's canary
  for non-additive format changes.
- `TestMetricsHandler_RegisterAndScrape_RoundTrip` proves the
  registry→handler seam works end-to-end with a trivial counter.
- `TestMetricsRegistry_NoGlobalRegistrarLeak` structurally enforces the
  ADR-0008 § Scope of use rule by asserting a zero delta on
  `DefaultGatherer.Gather()` across registry+handler construction.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ilmoniemi
Copy link
Copy Markdown
Contributor Author

Code Review: #59

Decision: PASS

Findings

None blocking. Two well-justified deviations from the spec, both documented inline:

  • [NIT] internal/relay/metrics_test.go:40 — Content-Type assertion uses strings.HasPrefix against the AC literal instead of equality. AC pins the exact string text/plain; version=0.0.4; charset=utf-8; newer prometheus/common versions append ; escaping=<scheme> (currently underscores). The developer's comment explains the rationale and notes the literal is still the canary that catches a silent format change. Reasonable adaptation to library reality; the AC's intent (verify standard Prom text format) is preserved.
  • [NIT] internal/relay/metrics_test.go:44-61TestMetricsHandler_EmptyRegistry_ResponseShape no longer asserts families == 0 as the spec described. The developer correctly identified that HandlerOpts.Registry=reg (which the spec deliberately sets to keep promhttp's self-instrumentation off DefaultRegisterer) registers promhttp_metric_handler_* on reg, so the registry is not literally empty when scraped. The test still verifies parseability by decoding to io.EOF without error — that satisfies the AC's "parseable body" clause. Spec's internal inconsistency resolved sensibly.

Verification

  • make vet — clean
  • make test (runs with -race) — clean, all three new tests pass in 2.7s
  • make build — clean

Security review verification (security-sensitive)

  • Architect's ## Security review section is present in the spec with PASS verdict and reviewed findings — required gate satisfied.
  • Diff applies clean security goggles:
    • No tokens / secrets touched; no log lines emit headers
    • No file ops, no subprocess, no crypto primitives
    • HandlerOpts.Registry: reg is set deliberately to hold the ADR-0008 "never DefaultRegisterer" boundary even for the library's own bookkeeping — code matches the spec's design rationale
    • No new network-reachable endpoint (listener is relay: localhost-only /metrics listener with bind-address validation #60)
    • No new goroutines, locks, or shared state
  • TestMetricsRegistry_NoGlobalRegistrarLeak enforces the no-global-leak rule structurally as the security review promised.

Summary

Faithful, minimal implementation of the scaffolding slice. ADR-0008 is comprehensive and matches the dep tree in go.sum. The two test deviations are documented adaptations to library reality, not silent drift. metrics.go (~50 lines incl. doc comments) cleanly mirrors NewHealthzHandler's factory pattern. Seam is correct for siblings #57/#58/#60 to consume.

The docs/knowledge/codebase/59.md and docs/knowledge/INDEX.md updates listed in the AC are documentation-phase territory per the architect's spec § Open questions #3 — correctly absent from this PR.

…+ feature in INDEX (#59)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ilmoniemi ilmoniemi merged commit b7344ba into main May 12, 2026
2 checks passed
@ilmoniemi ilmoniemi deleted the feature/59 branch May 12, 2026 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

relay: adopt prometheus/client_golang and introduce metrics registry scaffolding

1 participant