A dependency-free Prometheus /metrics endpoint (#91)

[aaylward]

Closes #202.

What

The first work item of #91, server side. MetricsRegistry aggregates the existing Observe hooks; MetricsEndpoint serves them in the Prometheus text exposition format, which needs no client library — so this costs zero new dependencies and lives in :server directly. RecordMetrics is Observe wired to a registry, built on it rather than beside it so request timing keeps one implementation.

auto metrics = std::make_shared<smithy::server::MetricsRegistry>(
    smithy::server::MetricsOptions{.enabled = true, .service_name = "todo-service"});
transport.Start(smithy::server::Chain({MetricsEndpoint(metrics),
                                       RecordMetrics(metrics),
                                       HealthEndpoint()},
                                      server.Handler()));

Off unless asked for, and absent rather than idle when off

MetricsOptions::enabled defaults to false. Disabled, RecordMetrics and MetricsEndpoint compose to the identity — not a wrapper that checks a flag, but no wrapper at all, so a served request runs the call chain it would have run had this never been written. /metrics then reaches the router like any other unmodeled path and 404s; an empty 200 there would read to Prometheus as a live target reporting no series, which is indistinguishable from a service whose metrics have gone silent. Handles from a disabled registry are inert rather than unusable, so application code never branches on the flag; only their arguments still cost anything, so enabled() is exposed for a hot call site whose labels are expensive to build.

Registration is deliberately not conditional on the flag. An invalid metric name, a type collision, or a missing service_name aborts at startup either way (ADR-0009), so switching metrics on in production is never the first time those checks run.

The exposition is MoonBase's, and is not configurable

Five families labeled by service_name, http_method, route:

Family	Type
http_server_requests_total	counter
http_server_requests_success_total	counter (status < 400)
http_server_requests_failure_total	counter (status >= 400)
http_server_requests_active_gauge	gauge (no route)
http_server_request_duration_microseconds	histogram

plus metrics_observations_dropped_total, the registry's own health.

This is not a vocabulary invented here. It is MoonBase's shared HTTP serving contract, spoken identically by its Java (yodel), Rust (server_pal) and C++ (futility/otel, behind aura) emitters and pinned across them by //domains/platform/libs/otel_contract. Those services are who scrapes this, and prom_proxy queries exactly these names with exactly these labels.

An earlier revision made all of it configurable with a preset. That was surface with no user, and worse: a knob is a way for one service to drift off a contract the fleet shares, and the drift is silent — the panel renders empty, which looks like a quiet service rather than a misconfigured one. MetricsOptions is now three fields that are still genuine choices: whether it runs, who it says it is, and the cardinality cap.

Things the diff doesn't show:

• Status is not a label. The outcome rides on the success and failure counters, which are two views of the same status-keyed tally the total sums — so the three can never disagree, they need no drop accounting of their own, and no series is multiplied by the codes a service happens to return.
• The active gauge carries no route. It moves at request start, before dispatch, where nothing bounded is known about the path. Every rail leaves the route off it for that reason, and prom_proxy's negative route!="/health" matcher passes a series without the label through untouched — which is what makes the same filter safe on it.
• service_name is required when enabled, and empty aborts. Every dashboard query selects on it, so a service reporting the empty string is scraped, stored, and absent from all of them: success everywhere except the panel nobody is watching yet. The transport tests found this the moment it landed.
• Labels are bounded by construction. target is never a label. An unrouted request reports unmatched rather than the empty string, because route!="/health" matches the empty string and unrouted traffic would silently join the serving figures. http_method collapses to CUSTOM outside the nine RFC 9110 verbs (case-sensitively — a rejected get must not report as served GET), and a request rejected before its method parsed reports (unparsed).
• Two divergences from futility, deliberate. It puts status_code/result/error_type on its failure counter and histogram; yodel and server_pal do not, and otel_contract pins only the route sentinels cross-rail, so the leaner set stays. And futility splits success at >=200 && <400 while yodel and server_pal both use <400, which is what is implemented here.

Folded in: probes stop reporting as the unrouted sentinel

HealthEndpoint built its response without stamping HttpResponse::operation, so every probe reached Observe — and so any backend, not just this one — under the same value 404/405/400 dispatch failures use. An orchestrator polls liveness every few seconds, making it usually the highest-volume path a service has, so the 404 rate was buried under probe volume, the probe's latency contaminated the service duration histogram, and no query could separate them. MetricsEndpoint had the same hole.

Both now report their composed path — fixed at composition, never off the wire, so it costs one series per composed endpoint. It is also the prerequisite for prom_proxy's fleet-wide route!="/health" subtraction.

Folded in: RequestObservation carries what an access log needs (#202)

The observation reported six fields — enough for metrics, and not enough for the other thing every server does with a per-request hook. aura says so in its own comment: its access log is kept separate from Observe because the observation lacks the response size and the forwarded client, so the chain runs two clocks per request measuring the same interval at different depths, free to disagree.

The sharper gap is in the same file: the raw x-forwarded-for it logs instead is not the ADR-0012 derived client PerClientRateLimit keys on, so "whose bucket did that 429 come from" was unanswerable — the service computed the answer and dropped it.

Added: request_bytes, response_bytes, handler_threw, and client — the derived client with its source provenance, never the forgeable header. The provenance rides along because its distribution, not any single value, is the documented misconfiguration signal. handler_threw separates a contained crash from a deliberate 500; both report status 500 with no operation, and a log that cannot tell them apart sends whoever reads a 5xx spike looking for the wrong thing.

Observe takes the trust boundary as a fourth defaulted parameter rather than reading it off the request. The alternative — the transport deriving and stamping it at ingress, as it already mints traceparent — is architecturally nicer but puts the boundary in two independently-configured places that can disagree, which is what ADR-0012 exists to prevent. TrustedProxies::None() as the default means an unconfigured service reports the peer, which is true, rather than nothing. Costs no new dependency: middleware.h already included forwarded.h.

This unblocks #203, the structured access-log formatter, which is deliberately not in this PR.

Testing

Unit tests over the registry, the application families, and the composed middleware: exposition format, sub-millisecond latencies surviving the microsecond hook, concurrent recording, method clamping, cap accounting, label escaping and ordering, registration aborts, a handle outliving its registry, and in-flight pairing including the throwing-handler path.

The ExpositionContract suite pins the MoonBase literals — the five names with their descriptions, the label set, the outcome split at 400, the gauge keyed by method and never by route, the route and method sentinels, and the 15-bound microsecond ladder. Four of those were borrowed from MoonBase's own rails after reading their tests: no series carries the old method spelling (swept across all five families), a query string does not defeat the /health route, scanner paths collapse into one series, and invented methods collapse on the gauge too — a separate code path from the counters, and the one gap the borrowed reading actually found.

The DisabledRegistry suite pins the other half. The identity-composition test uses a plain function as the terminal so std::function::target is non-null exactly while nothing has wrapped it, and asserts the enabled case does wrap — without that second half the first proves nothing.

The observation fields get five tests, including both forwarded-client directions: behind a trusted proxy the header's client entry wins, and from an untrusted peer the forged header is ignored. Mutation-checked — forcing TrustedProxies::None() makes the trusted case report the proxy instead of the client, and the test fails.

Two over a real Beast socket. Nine in the out-of-tree consumer module, driving the generated router so the route label is the model's and not a fixture's.

Locally: make verify (127 tests under --config=werror, lockfiles, codegen, goldens, clang-format), make noexcept, gcc ASan and TSan, clang-tidy, buildifier via npx, and the 14-test consumer suite. Two sandbox gaps, both verified another way: make lint's buildifier has no system binary here (npx passes), and clang-tidy cannot parse beast_transport.cc without Boost headers, which CI installs — the files this PR touches are clean.

Deliberately not covered

The client-side registry #91 item 1 also asks for. AttemptObservation carries no duration, so the per-attempt latency histogram can't be built from ObserveAttempts today — that's the open API-shape question in the issue's own comment, and item 3 sequences hook changes separately and first. Items 2–4 remain, so this does not close #91.

Second item carried: one commit updates docs/working-agreement.md from MoonBase's fork of it (adapted, not copied). Unrelated to the metrics work, kept here by request.

Follow-ups filed: #201 (removing smithy from runtime surfaces that have nothing to do with the IDL — the metric names were the visible instance and are fixed here) and #203 (the structured access-log formatter, unblocked by the observation work above).

Checklist

• Tests added/updated for the change
• bazel test //... and (cd codegen && gradle build spotlessCheck) pass locally
• Formatting clean (clang-format, buildifier via npx, spotless)
• Architectural decisions recorded as an ADR (not applicable — new middleware on the existing composition pattern; no new architecture)

🤖 Generated with Claude Code

https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU

[cursor]

Review

The design is in good shape: identity composition when off, MoonBase names/labels/sentinels pinned by tests, Observe as the single clock, probe operation stamping, and the extra observation fields for #202. CI is green. Bundling #202 / the working-agreement commit with the metrics work is taken as intentional.

Defects below. The first two are the ones that would actually mislead a dashboard or a scraper.

Defects

1. Under max_series, request counters and the duration histogram can disagree. Record enforces the cap on counts_ and latencies_ independently, and CountKey still includes status even though status is not a label. Same method+route with a new status code: the counter insert is refused, the histogram is still updated. http_server_requests_total then undercounts (or never appears for that route) while _count / _sum keep growing — the opposite of “the three counters are views of one tally” once you look at the histogram beside them. The existing cap test only uses distinct routes at 200, so this path is unpinned.

The simplification that also fixes it: since status is no longer a label, key the built-in counters the same way as latency (method,route) and keep running success/failure tallies there. One map, one cap, no way for the families to drift.

2. Application drops split metrics_observations_dropped_total. The unlabeled series is emitted with its HELP/TYPE, then every application family is written, then extra samples of the same metric (with metric=) are appended. Prometheus text 0.0.4 requires a family to be contiguous. Related scrape-corruption hole: application help is interpolated into # HELP with no escaping (newline / backslash).

Emit every dropped sample next to the first TYPE line (and escape HELP).

3. The production-guide snippet aborts if copied. MetricsOptions{.enabled = true} with no service_name is exactly the Fatal the rest of the PR is careful about. Same paragraph still says rejections report operation="" and method unparsed; the scrape is route="unmatched" and http_method="(unparsed)".

Nits (stale contract text)

• Header + CHANGELOG say nonstandard methods collapse to "other"; code and the exposition tests emit CUSTOM.
• HistogramData still comments options_.latency_unit, which is gone.
• Guide claims a bad histogram bucket ladder aborts at registration; NewHistogram does not validate order/emptiness.

No ask to unbundle the working-agreement commit or #202.

[cursor]

Beyoncé pass (#202 access-log fields)

If you liked it, you should have put a test on it — and a consumer that actually uses it, not one that compiles against the type.

request_bytes, response_bytes, handler_threw, and client exist so an access log can share Observe's clock and answer "whose bucket did that 429 come from." Unit tests in middleware_test.cc pin the fields in isolation. Nothing a consumer would write does.

Consumer / example: no logger uses the new data

The out-of-tree Observe sink still ignores the observation:

       smithy::server::Observe(
           [&completed](const smithy::server::RequestObservation&) { ++completed; },
           [&started](const smithy::server::RequestStart&) { ++started; }),

metrics_acceptance_test.cc flexes the scrape; PerClientRateLimitKeysOnTheDerivedClientAddressNotTheSpoofableHeader flexes the limiter. Neither formats a log line, neither passes TrustedProxies into Observe, neither asserts observation.client.address is the key the limiter just used. Weather e2e still checks method/target/status only.

The production-guide chain is the same gap in the copy-paste path: PerClientRateLimit(..., trusted) then Observe with no fourth argument, so the sink gets TrustedProxies::None() and would log the peer while the limiter keyed on the forwarded client. The callback comment is still the old six fields (method/target/operation/status/duration/trace_parent); the body is still // latency o.duration. The later prose that lists the new fields is not a test and is not what a consumer copies.

The "Watch the trust boundary" paragraph still tells you to wrap a one-line middleware around DeriveClient because "Observe's sink sees only the finished observation, not the request." That was the hole this commit claims to close. o.client.source is now on the observation; the guide still teaches the pre-#202 workaround.

#203 being a follow-up does not excuse this. The contract being shipped is "these fields are what an access log needs." Beyoncé wants a consumer test that is that log: generated server, real socket (peer stamped), same TrustedProxies on limiter and Observe, sink that records bytes / handler_threw / client.address+source, and the 429 case where the two agree. exception_safety_acceptance_test already throws through Beast without Observe — handler_threw through containment is unpinned at that boundary too.

Until that exists, the new members are in-tree coincidence plus documentation.

[aaylward]

Verdict

Approve — 0 blocking, 1 non-blocking

Both fix commits correctly address every blocking finding from the thread. The RouteStats refactor is the right structural fix (not a band-aid), the new tests pin the exact failure modes reviewers identified, and the access-log acceptance test is a genuine consumer contract across the module boundary. CI is green. This is safe to merge.

Reviewed the two commits that landed after the initial review: 07c72c0 (scrape-integrity fixes) and 1082127 (access-log consumer contract).

What Was Done Well

• RouteStats unification — One {method, route} key owns counters, success/failure tallies, and histogram together. The deliberate rejection-only path (counted, not timed, no empty histogram) is documented and tested.
• Exposition integrity — Drop counter emitted whole before application families; EscapeHelp for caller-controlled strings; histogram ladder validation at registration (ADR-0009).
• Test quality — TheCapCannotSplitACountFromItsLatency reproduces the exact counter/histogram drift scenario. EveryFamilyIsContiguous validates all families, not just the one instance. access_log_acceptance_test.cc tests the composition a reader would copy: generated router, real Beast socket, same TrustedProxies on limiter and Observe, 429 logging, byte counts, and handler_threw through transport containment.
• Guide fixes — Snippet passes service_name, passes trusted to Observe, documents Observe-outside-limiter trade-off, removes the pre-#202 workaround paragraph.

Non-Blocking Issues

• access_log_acceptance_test.cc — Commit message claims mutation-checking (dropping trusted from Observe fails 3/4 tests), but there's no explicit mutation test in-file. The tests would fail with wrong wiring, which is good implicit coverage; an explicit negative case or a comment pointing to the failure mode would make the claim auditable without re-running manually.

Test Coverage

Adequate for the fixes. The two commits add 7 new tests covering cap drift, rejection-without-histogram, family contiguity, HELP escaping, histogram ladder aborts, and 4 consumer acceptance scenarios. No gaps that would block merge.