Export metrics from the standalone symphony-server - #33
Conversation
symphony's counters were only reachable through the napi `metrics()` call, so a deployment running `symphony-server` as its own process — which is how host-manager supervises it — had no way to read them at all. The per-listener counters were worse off: they were incremented on every hot path and then never read by anything. Adds an out-of-process export path, and broadens what there is to export. Counters (`src/metrics.rs`): - Per-listener counters are now surfaced through `metrics()`, one entry per configured listener with its address and mode. - `total_errors` was a single bucket for TLS handshake failure, upstream connect failure, idle timeout and copy error — four different alerts collapsed into one number. Errors and blocks are now broken out by reason, using a fixed-size array indexed by enum discriminant so the hot path stays allocation-free. A new `protection::BlockReason` variant fails to compile until it is mapped to a `BlockKind`, so a new check can't land in an unlabeled bucket. - Byte counters, via a `CountingStream` wrapper on the client side. Counting `copy_bidirectional`'s return value instead would lose every byte of any session that ends by idle timeout or reset — i.e. most long-lived ones. - Suspended connections now record how they ended (resolved vs timed out), and the route table exposes its live and cert-failing route counts. - `forward()` reports an idle timeout as its own kind rather than synthesising an `io::ErrorKind::TimedOut` for the caller to infer from — a peer's kernel-level ETIMEDOUT produces the same kind and would have been misfiled. - A listener-level `maxConnections` rejection now increments the proxy-wide blocked total as well as the per-listener one, so the proxy total is once again the sum across its listeners. Export (`ts/admin.ts`): - An optional `admin` block in the config file exposes `GET /metrics` (Prometheus text), `/metrics.json` and `/health` over a Unix socket, a loopback TCP port, or both. Omitted by default — nothing is exposed unless configured. - Blocked/error counts are emitted only under their `reason` label; the labelled series sum to the total by construction, so a separate unlabelled metric would be a second representation of the same number. - The endpoint never affects proxying: a bind failure is logged and retried on a timer rather than aborting a reconcile. This is load-bearing during a version upgrade, where the incumbent still holds the socket while the successor is already serving traffic through SO_REUSEPORT. - A socket file left by a SIGKILLed process is reclaimed, but only after a connect probe proves nobody is listening — unlinking unconditionally would let a starting process silently steal the endpoint from the running one, the same failure mode the status.json ownership guard exists to prevent. Tests cover the per-listener breakdown and exact byte counts, each error classification, the Prometheus output shape, the endpoint over both transports, and stale-socket reclaim after a SIGKILL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ples Three findings from the Gemini review leg. Byte counting did a `fetch_add` on a listener-shared atomic for every chunk read or written — putting one cache line in the path of every 8 KiB of proxied traffic and ping-ponging it across every core, which is the cross-core contention SO_REUSEPORT per worker exists to avoid. Counts now accumulate per connection and publish every 256 KiB and on drop, so a saturated connection touches the shared line ~32x less often while a scrape still sees a busy connection's traffic promptly. Drop covers the aborted-at-shutdown path. The Prometheus renderer emitted samples in proxy -> listener call order, so with more than one proxy configured a metric name's samples were split across the output under a single HELP/TYPE pair. Strict parsers reject or drop the split group. Exposition now accumulates by name and renders grouped, leaving callers in the natural iteration order. AdminServer.update() left a pending bind retry armed, so a config reload during a retry backoff would tear down the listeners it had just bound. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Consumers bind this endpoint on loopback, which any local process can reach. That is only acceptable while the labels stay free of tenant identifiers — a per-SNI or per-route label would turn aggregate proxy health into a list of which customers a host fronts. Asserting the allowed set makes adding one a deliberate, visible decision rather than an incidental one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e accounting Two blockers and four significant findings from the Codex review leg. Stale-socket reclamation could delete a live endpoint. The probe treated every connect error as "nobody is listening", so a live socket that refuses the probe with EACCES — restrictive permissions, or another uid — would be unlinked. It now reclaims only on ECONNREFUSED, and only when the inode is actually a socket: a socketPath misconfigured onto a regular file (status.json, say) would otherwise have been deleted and replaced. Reclamation also had a check/unlink race: two processes could both find a path stale, and the second one's unlink would remove the socket the first had already bound. The bind now happens on a pid-unique temp path that is renamed into place, which is atomic and leaves no window. The precheck stays, so an upgrade overlap still loses and retries rather than stealing a live incumbent. Since Node unlinks the path it bound — the temp one — shutdown now removes the published path itself, guarded by the inode it published there. The exported totals couldn't satisfy their own sum invariant under traffic: total_blocked and its reason counter were two non-atomic writes, so a scrape between them saw a total that disagreed with its breakdown. The invariant held only while the proxy was idle, which is exactly when nobody is looking. Totals are now derived from the same values reported alongside them, which also removes an atomic from the block/error paths and deletes the proxy-wide blocked counter outright — a better fix than the second increment added earlier in this branch. The HTTP-mode listener consumed the request head before any wrapper could see it and wrote its redirects directly, so bytesReceived never moved for an ACME request and no redirect response was counted at all. It now accounts for both explicitly. keepAliveTimeout = 0 disables the timeout rather than keep-alive, so idle scrape connections were never reaped — they could accumulate against the same fd budget the proxy listeners draw from. Set to 5s, with a connection cap. Corrects the byte-counting docs: passthrough routes do count handshake records, because to a passthrough route they are simply part of the forwarded stream. Codex independently flagged the per-chunk atomic on the byte counters, already fixed in 479070d from the Gemini leg. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive metrics and monitoring system for the symphony proxy, including detailed in-process metrics, a Prometheus text renderer, and an optional out-of-process admin HTTP endpoint over Unix sockets or TCP. While the implementation is thorough and well-tested, several critical issues were identified: the idle timeout implementation in src/proxy_conn.rs acts as a maximum connection duration limit that will prematurely terminate active long-lived connections; a subtle race condition in ts/admin.ts could lead to the incorrect deletion of a successor's live socket on shutdown; the socket probe lacks a timeout and could hang indefinitely; and test teardown blocks in __test__/metrics.spec.ts cause unnecessary 3-second delays by failing to check for process signal termination.
…he unlink race Two low-severity residuals from the final review pass. The HTTP listener counted response bytes before writing them, so a partial or failed write reported bytes the client never received. Counted after a successful write instead. Documents that unpublishSocket()'s inode guard narrows the shutdown-vs-rename race rather than closing it — the check and the unlink are two syscalls. Closing it would need file locking, and the exposure is one observability endpoint that self-heals on the next reconcile, unlike the probe->unlink->bind window it replaced, which could strand a live endpoint indefinitely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…admin ops Three findings from the draft review plus three from the CI review bot. Shutdown no longer unlinks the published socket path. The inode guard narrowed the check-then-unlink race but could not close it, and the loss it risked is not self-repairing as the previous comment claimed: update() returns early on an unchanged signature, so a successor whose socket got unlinked would keep serving an unreachable path until its config changed or it restarted. Leaving the pathname costs one inode in a 0o700 directory, and the next binder replaces it atomically after proving it stale. The review bot separately suggested reading the inode before the rename to make the guard exact; that fixes a different variant of the same hazard, and removing the unlink subsumes both. update(), stop(), and the retry timer all mutate the same listeners across awaits, and the retry ran fire-and-forget. A retry that was mid-bind could publish its server after a later update() or stop() had finished, and with a UDS target could leave two paths live. They now run through one operation chain. renderPrometheus and MetricsSnapshot are no longer exported from the package root. A snapshot carries the standalone server's pid, timestamps, and port-set grouping, which an embedded consumer would have to synthesise; exporting it made that an API commitment with no caller asking for one. The reclaimability probe now has a timeout — a frozen owner or a full accept backlog would otherwise hang it and stall the reconcile that called it. Test teardowns checked only exitCode, which stays null for a signal-terminated child, so every cleanup block waited out its 3s timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review bot flagged that forward()'s tokio::time::timeout is a hard deadline that does not reset on I/O, so idleTimeoutMs terminates busy connections at their total duration rather than after silence. That is pre-existing on main and out of scope here, but this branch is what names the metric, so the label would mislead without saying so. Filed as #34; noted where the reason is defined and in the README's reason list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sleekmountaincat
left a comment
There was a problem hiding this comment.
.____ ________________________
| | / _____/\__ ___/ \
| | / \ ___ | | / \ / \
| |__\ \_\ \ | |/ Y \
|_______ \______ / |____|\____|__ /
\/ \/ \/
Why
symphony's counters are only reachable through the napi
metrics()call, so a deployment runningsymphony-serveras its own OS process — which is how host-manager supervises it — has no way to read them at all. The per-listener counters were worse off: they were incremented on every hot path and then never read by anything.Companion PR: HarperFast/host-manager#166 turns the endpoint on.
What
Export path —
ts/admin.tsAn optional
adminblock in the server config exposesGET /metrics(Prometheus text),/metrics.json, and/healthover a Unix socket, a loopback TCP port, or both. Omitted by default; nothing is exposed unless configured.{ "admin": { "socketPath": "/run/symphony/admin.sock", "port": 9095, "host": "127.0.0.1" } }Blocked and error counts are emitted only under their
reasonlabel — the labelled series sum to the total by construction, so a separate unlabelled metric would be a second representation of the same number (sum without(reason)).Counters —
src/metrics.rsmetrics(), one entry per listener with its address and mode.total_errorswas a single bucket for TLS handshake failure, upstream connect failure, idle timeout and copy error — four different alerts collapsed into one number. Errors and blocks are now broken out by reason, using a fixed-size array indexed by enum discriminant so the hot path stays allocation-free.CountingStreamwrapper on the client side. Readingcopy_bidirectional's return value instead would lose every byte of any session that ends by idle timeout or reset — most long-lived ones.What gets exported
12 metric names. Every reason is emitted even at zero, so a dashboard series exists before the first incident rather than appearing mid-outage.
symphony_build_infoversionsymphony_start_time_secondssymphony_config_reload_time_secondssymphony_routesproxysymphony_routes_failingproxysymphony_suspended_pendingproxyresolveConnection()symphony_suspended_totalproxy,outcomeoutcomeisresolvedorunresolvedsymphony_listener_active_connectionsproxy,listener,modesymphony_listener_accepted_totalproxy,listener,modesymphony_listener_bytes_received_totalproxy,listener,modesymphony_listener_bytes_sent_totalproxy,listener,modesymphony_listener_blocked_totalreasonsymphony_listener_errors_totalreasonBlock reasons —
max_connections,cidr_blocked,ja3_blocked,ja4_blocked,incomplete_handshake,no_sni,rate_limited,too_many_connections,penalty_boxedError reasons —
no_route,route_rate_limited,suspend_unresolved,tls_handshake,tls_missing_cert,upstream_connect,idle_timeout,stream,http_headerSample scrape
Trimmed from real renderer output — a
:443TLS proxy and a:80HTTP one:/metrics.jsonThe same snapshot, unrendered — for a collector that would rather not parse text:
{ "pid": 1234, "version": "1.1.0", "startedAt": "2026-07-27T09:00:00.000Z", "reloadedAt": "2026-07-27T14:12:03.000Z", "proxies": [ { "ports": "443", "metrics": { "activeConnections": 412, "blockedConnections": 27, "pendingSuspended": 0, "suspendedResolved": 0, "suspendedUnresolved": 0, "routes": 138, "failingRoutes": 1, "listeners": [ { "address": "0.0.0.0:443", "mode": "tls", "activeConnections": 412, "accepted": 1482137, "blocked": 27, "errors": 348, "bytesReceived": 91238412734, "bytesSent": 812384123991, "blockedByReason": [{ "reason": "rate_limited", "count": 24 }], "errorsByReason": [{ "reason": "upstream_connect", "count": 4 }] } ] } } ] }Cardinality
The
proxylabel is the port-set of the config entry a listener belongs to; each entry keeps its own route table. On a Fabric host that is 5 port-sets × 1 listener × (4 + 9 + 9 series) ≈ 110 series — flat, regardless of how many tenants the host fronts, because no label carries a route or SNI.Invariants worth reviewing
Three places where the obvious implementation is subtly wrong, and the fix is structural rather than a guard:
Totals are derived, never maintained alongside their parts. A
total_blockedincremented next to its reason counter is two non-atomic writes, so a scrape landing between them sees a total that disagrees with its own breakdown — the invariant would hold only while the proxy is idle, precisely when nobody is reading it. Totals are summed from the same values reported alongside them.The idle timeout is its own error kind, not an inferred one. Classifying on
io::ErrorKind::TimedOutwould misfile a peer's kernel-levelETIMEDOUTas an idle timeout.A live Unix socket is never deleted. Reclaiming a stale socket requires
ECONNREFUSEDspecifically (anEACCESfrom a restrictively-permissioned live socket is not evidence nobody is listening) and an inode that is actually a socket (asocketPathmisconfigured onto a regular file would otherwise be deleted). The bind happens on a pid-unique temp path that isrenamed into place, so there is no probe→unlink→bind window for a second process to delete a socket the first has already bound. On shutdown the published path is unlinked only while its inode is still ours. Same family as thestatus.jsonownership guard.Behaviour changes worth flagging
SO_REUSEPORT.blockedConnectionsnow includesmaxConnectionsrejections. It didn't before, which meant the proxy-wide total wasn't the sum of its listeners. Small semantic change to a shipped field.Security posture
Metric labels are
proxy,listener,mode,reason,outcome,version— no SNI, hostname, or other tenant identifier. That is what makes a loopback binding acceptable for consumers; a per-route label would turn aggregate proxy health into a list of which customers a host fronts. There is a test pinning the allowed label set so adding one is a deliberate, visible decision.Testing
101 JS tests + 101 Rust tests, clippy clean. New coverage: per-listener breakdown and exact byte counts, each error classification (
no_route,upstream_connect,idle_timeoutvsstream), Prometheus output shape and sample grouping, the endpoint over both transports, stale-socket reclaim after a realSIGKILL, and — the inverse, which is the one that costs something to get wrong — a live socket surviving a would-be reclaimer whose probe fails withEACCES.Generated with Claude Opus 5.
🤖 Generated with Claude Code