feat(gateway): add a /metrics endpoint on the admin listener - #1037
Open
kvinwang wants to merge 12 commits into
Open
feat(gateway): add a /metrics endpoint on the admin listener#1037kvinwang wants to merge 12 commits into
kvinwang wants to merge 12 commits into
Conversation
The gateway has no metrics endpoint at all, so everything an operator needs to notice degradation — sync lag against a peer, certificate expiry, how much state a node is carrying — is reachable only by calling an admin RPC by hand and reading a `#[derive(Debug)]` dump. Issue #1029 asks for an observability minimum set; this is the exit it needs. `GET /metrics` is mounted on the **admin** listener, not the public one. The series name domains, node ids and instance counts: that is cluster topology, and the gateway's public listener is reachable by every CVM. The admin listener already requires a token and is off by default, so the endpoint inherits both. KMS puts its `/metrics` on the public listener behind a config flag, but it exports two unlabelled counters, not a description of who is routed where. A test pins the endpoint to the admin route set so a later refactor cannot quietly move it to the public one. Gauges are sampled at scrape time rather than mirrored into counters, so a scrape can never disagree with the state it describes. Two things the sampling is careful about: it does not call `AdminRpcHandler::status()`, which would make every scrape a replicated write via `refresh_state()`; and it holds the proxy lock only for three counts, since the public data path takes that same lock on every connection. Domains reach the exposition format from replicated state, so label values are escaped. Unescaped, a peer that gets a `"` and a newline into a domain writes arbitrary series into the scrape output — the test drives exactly that payload.
Three failure modes are logged and then dropped on the floor, and all three are silent in exactly the way that matters: the gateway keeps answering, keeps reporting healthy, and stops doing part of its job. `wg syncconf` rejects the *whole* config file when a single peer stanza is malformed. `reconfigure()` logs that and returns `Ok(())`, so a gateway that has stopped applying routing updates is indistinguishable from one with nothing to apply. `dstack_gateway_wg_syncconf_failures_total` is the signal; until now the only one was noticing that a CVM never became reachable. A replicated value that fails to decode is skipped per key, which is the right containment behaviour — but the CVM behind that record silently drops out of routing with one `warn!` to say so. The counter is labelled by key prefix, so `inst/` (a CVM vanished) is distinguishable from `cert/` (a domain lost its certificate) without reading logs. The label set is a fixed list of known prefixes plus `other`: the keys are peer-supplied, so an open label set would be a cardinality bomb pointed at the scraper. Failed periodic snapshots get a counter too — each one means a restart replays a longer WAL, and the WAL only stays bounded because snapshots succeed. None of this changes behaviour: every call site keeps its existing log and its existing control flow.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a Prometheus /metrics scrape endpoint to dstack-gateway and wires previously log-only failure signals into exported counters, intended to improve operational visibility without adding new public-surface exposure (admin listener only).
Changes:
- Add
/metricsroute (admin listener) that samples live gateway/KV state and renders Prometheus text exposition. - Introduce a gateway-local metrics module with counters (decode failures,
wg syncconffailures, KV snapshot persist failures) and gauges sampled at scrape time. - Wire decode-failure and operational failure call sites to increment the new counters; document scraping/alerting guidance.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dstack/gateway/src/web_routes/metrics.rs | Implements scrape sampling from Proxy + KV and builds a Snapshot for rendering. |
| dstack/gateway/src/web_routes.rs | Mounts /metrics in the admin route set and adds a regression test around route-set membership. |
| dstack/gateway/src/metrics.rs | Defines metric counters/gauges and renders Prometheus exposition text, including label escaping. |
| dstack/gateway/src/main.rs | Exposes the new metrics module at crate level. |
| dstack/gateway/src/main_service.rs | Increments metrics counters for wg syncconf outcomes and KV periodic persist failures. |
| dstack/gateway/src/kv/mod.rs | Increments per-prefix decode-failure counters at decode call sites. |
| docs/dstack-gateway.md | Documents the new /metrics endpoint and suggests scrape/alert targets. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The autofix that rewrote this doc comment replaced the `#[test]` line along with it, so `metrics_is_mounted_on_the_admin_listener_only` compiled as dead code and never ran -- the run went from 84 tests to 83 with a `dead_code` warning, and the invariant the test exists to pin was unenforced. The rewritten comment is kept: it is more accurate than the original about what the test covers (route-set membership, not the listener wiring in `main.rs`).
The exposition format defines exactly three escapes in a label value -- `\\`, `\"` and `\n`. Escaping `\r` and `\t` as well reads like defence in depth but inverts the result: `prometheus/common`'s parser, which backs `promtool check metrics` and most client tooling, rejects an unknown escape sequence outright. A tab is legal as a raw byte inside a quoted label value; written as `\t` it takes the whole scrape down. That is a cheaper attack than the one the escaping exists to stop. Domains arrive from replicated state, so the peer who cannot forge a series with `"` and a newline could instead put one tab in a domain and cost the operator every metric on the node. Keep the three defined escapes and drop remaining control characters, so the output is valid and carries no raw control bytes either.
`get_all_nodes()` and `get_active_nodes()` sit on `ProxyState` but read no proxy state -- only `self.kv_store`. Calling them from the scrape put all of their work inside the mutex the data path takes on every connection: two loads of the node table, one status load, a `GatewayNodeInfo` with five cloned strings per node, and one ephemeral read-lock acquisition per node for a `last_seen` the count never looks at. That last one is the sharp edge -- the scrape repeatedly took the ephemeral lock while holding the proxy mutex, against a sync task that writes ephemeral. `KvStore::count_nodes()` returns the two numbers directly, so the lock is held for one `BTreeMap::len()`. The active/down predicate moves to `KvStore::node_is_active()` and is shared with `get_all_nodes_filtered()`, because a gauge that counted a node the router had dropped would describe a routing table that does not exist.
…nter `sample()` reads certificates and the node table through the same decoding helpers the data path uses, and those call `record_decode_failure`. So one permanently corrupt `cert/` record incremented the counter once per scrape, forever: `rate()` over it measured the scrape interval, and halving that interval doubled the apparent failure rate. Observing was indistinguishable from failing. A thread-local guard suppresses counting for the duration of a scrape. It is scoped rather than a flag on `KvStore` because the suppression belongs to the caller's intent -- reading to report -- not to the store. Record corruption is a state, not an event, so the counter still answers a slightly wrong question on the data-path side: its rate tracks how often a bad record is read, which is traffic. Deduplicating by key would fix that, but the key set is peer-supplied and would need a bounded cache; leaving that for a follow-up. The docs now say to alert on `> 0` and not to read the magnitude.
Two gaps in what the new counters cover. `load_cert_attestations()` was the one decode site of six with no counter, and it is the one holding the TDX quote over a certificate's public key: the certificate keeps serving and routing keeps working while its attestation has quietly become unreadable. `reconfigure()` only counted the `wg syncconf` call. Rendering the config or writing it to disk can fail first, and both return early, so a full disk or a bad permission left every counter untouched -- indistinguishable from having nothing to apply, which is the silence this series exists to break. Both call sites only log the `Err`, so nothing else would have caught it. The counter becomes `wg_reconfigure_*`, since it no longer describes one command. Control flow is unchanged: a config `wg syncconf` rejects is still reported to the caller as success, as it always has been.
Five of the series describe replicated state, so every node in the cluster reports the same value for them. The other eight describe what this process did. Nothing in the names said which was which, and `sum()` is the first thing anyone writes in Grafana: on a three-node cluster `sum(dstack_gateway_ instances)` returned three times the real instance count. The replicated ones move under `dstack_gateway_cluster_*` and their HELP text carries the aggregation rule. Node-local series keep their names, because "one target reports its own numbers" is already the assumption a Prometheus user starts from -- only the series that violate it need marking. `cert_not_after_seconds` also gains a cap. Domains are created through the admin API, so in practice this is a handful of wildcard certificates, but the records are replicated and a peer with write access could otherwise turn one label into an unbounded series count -- the cardinality argument this module already makes for decode-failure prefixes, applied to the label that was exempt from it. `cluster_cert_domains` reports the untruncated count, so the cap cannot hide anything. Renaming is free today and breaking after this ships.
- `METERED_PREFIXES` now references `kv::keys` instead of repeating its string literals. A prefix renamed there and not here fails silently: the key space just starts being counted under `other`. - The longest-match rule moves into `longest_prefix_index()` and gets a test that actually exercises it. The metered set does not overlap today, so the old test named after the rule could not have caught it breaking. - `kv_peer_peer_ack` -> `kv_peer_remote_ack`, to pair with `_local_ack`. - `/metrics` states the exposition version in its Content-Type rather than leaving a scraper to infer a parser from bare `text/plain`. - A note at `status()` that wavekv 2.0 computes a whole-dataset digest there, which would put a full hash of both stores on every scrape.
Three corrections the code changes made necessary. Cluster-scoped series are replicated, so `sum()` across targets multiplies by the node count. The section now says which is which and gives the three queries an operator actually writes, including turning node disagreement about `nodes_active` into the replication-lag signal it is. `kv_decode_failures_total` counts reads of a bad record, not bad records, so its magnitude is a function of traffic. Alert on `> 0` and stop there. `insecure_no_auth` was not mentioned, which made "requires the same credentials" read as unconditional.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gateway has no metrics endpoint at all today, which is why four separate items of #1029 (P0.3 alerting, P1.7 per-prefix decode failures, P1.11 per-peer sync lag, and the "observability minimum set" design item) were all blocked on the same missing piece. This adds the exit and wires up the failures that are currently invisible.
Where it is mounted, and why
Admin listener only — not the public one. The series name domains, node ids and instance counts; that is cluster topology, and the public listener is reachable by every CVM. The admin listener already requires a token (
Authorization: Bearer/X-Admin-Token) and is off by default, so/metricsinherits both without new auth code or a new config flag.KMS mounts its
/metricson the public listener behind[core.metrics] enabled, but it exports two unlabelled counters, not a description of who is routed where — so the precedent does not transfer.metrics_is_mounted_on_the_admin_listener_onlypins this so a later refactor cannot quietly move it.Cluster-scoped and node-local series are named apart
Five of the series describe replicated state, so every node reports the same value for them; the rest describe what one process did. Nothing in a metric name usually says which is which, and
sum()is the first thing anyone writes in Grafana — on a three-node cluster it would have returned three times the real instance count.The replicated ones live under
dstack_gateway_cluster_*and carry the aggregation rule in theirHELPtext. Node-local series keep unprefixed names, because "one target reports its own numbers" is already the assumption a Prometheus user starts from; only the series that violate it need marking.What it exports
dstack_gateway_build_info{version,node_id}dstack_gateway_cluster_instances/_nodes/_nodes_activemax(), notsum()dstack_gateway_connectionsdstack_gateway_ktls_offloaded_total/_ktls_offload_failed_total/_splice_engaged_totalproxy::stats, just unexporteddstack_gateway_cluster_kv_keys{store}dstack_gateway_kv_next_seq{store}/_dirty{store}dstack_gateway_kv_peer_local_ack/_peer_remote_ack/_buffered_logs{store,peer}dstack_gateway_kv_decode_failures_total{prefix}dstack_gateway_wg_reconfigure_total/_failures_totaldstack_gateway_kv_persist_failures_totaldstack_gateway_cluster_cert_not_after_seconds{domain}- time()under the renewal windowdstack_gateway_cluster_cert_domainsThe failures that were log-only
reconfigure()fails three ways — the config fails to render, fails to write, orwg syncconfrejects the whole file over one bad peer stanza. All three leave the data plane on the routing table it already had, all three only reach a log line, and both call sites merely log theErr. A gateway that has stopped applying routing updates looked exactly like one with nothing to apply.inst/(a CVM vanished) is distinguishable fromcert/(a domain lost its certificate) without grepping logs. This includesload_cert_attestations(), where the unreadable record is the TDX quote over a certificate's public key: the certificate keeps serving while its attestation is gone.Every call site keeps its existing log and control flow; nothing behavioural changes. A config
wg syncconfrejects is still reported to the caller as success, as it always has been.Three things the sampling is careful about
AdminRpcHandler::status(), which callsrefresh_state()and would make every Prometheus scrape a cluster-replicated KV write.BTreeMap::len(). The node counts deliberately do not go throughget_all_nodes()/get_active_nodes(): those sit onProxyStatebut read no proxy state at all, so calling them under the lock would have dragged two loads of the node table, aGatewayNodeInfoper node, and one ephemeral read-lock acquisition per node — for alast_seenthe count never reads — inside the mutex the data path takes on every connection.cert/record would have incrementeddecode_failuresonce per scrape forever, andrate()over it would have measured the scrape interval.kv_decode_failures_totalstill answers a slightly wrong question on the data-path side — record corruption is a state, not an event, so its rate tracks how often a bad record is read, which is traffic. The docs say to alert on> 0and not to read the magnitude. Deduplicating by key would fix it properly but needs a bounded cache, since the keys are peer-supplied; that is a follow-up.Cardinality and injection
Label sets are bounded: no per-instance or per-key labels, and the decode-failure label set is a fixed prefix list plus
other, backed by a compile-time-sized array — a peer that invents keys can only land in theotherslot.cert_not_after_seconds{domain}is the one per-key label, because knowing which domain is about to expire is the point of the series. Domains are only created through the admin API, so in practice this is a handful of wildcard certificates, but the records are replicated and a peer with write access could otherwise turn one label into an unbounded series count. It is capped at 256 series withcluster_cert_domainsreporting the real count, so truncation is visible rather than silent.Domains reach the exposition format from replicated state, so label values are escaped. Unescaped, a peer that gets a
"and a newline into a domain writes arbitrary series into the scrape output;a_hostile_domain_cannot_forge_a_seriesdrives exactly that payload and fails if the escaping is removed. The escaping emits only the three sequences the format defines (\\,\",\n) and drops remaining control characters — escaping a tab as\tinstead would be rejected byprometheus/common's parser, handing that same peer a cheaper attack than the injection this defends against.Verification
cargo test -p dstack-gateway88 passed (11 new),cargo fmt --checkandcargo clippy -p dstack-gatewayclean. No new dependencies. Docs: a "Metrics" section indocs/dstack-gateway.mdwith a scrape config, the cluster-vs-node aggregation rule with worked queries, and the alert-worthy series.Independent of #1030/#1031/#1035/#1036 — no overlapping hunks. The wavekv-2.0-only telemetry (state digest, per-peer negotiated protocol, entries merged/rejected) is deliberately not here; it can be added to the same renderer once #1031 lands. Note that 2.0 also computes a whole-dataset digest inside
status(), which this samples on every scrape — there is a comment at the call site to revisit when the dependency moves.