feat(observability): enable metrics via config + operator monitoring setup

[luishsr]

What

The "turn on & unify" P1 of the Network Observability Initiative (axyl-private#429, research task #381). The research audit found the node is already heavily instrumented — it inherited Narwhal/Mysten's full Prometheus suite (consensus/primary/worker/network/executor metrics) and has reth's execution metrics — but everything is off by default, split across two registries, with no config-file route to enable it. This PR closes the turn-on & unify gaps.

Config — enable metrics from parameters.yaml

Both endpoints are now enable-able from config, not just CLI flags:

metrics_address: "0.0.0.0:9184"        # consensus / Narwhal suite (--metrics)
reth_metrics_address: "0.0.0.0:9001"   # reth execution layer (--reth-metrics)

• Both Option<SocketAddr> with #[serde(default)] — every existing parameters.yaml still parses and stays off (covered by tests).
• The --metrics / --reth-metrics CLI flags override the config values when passed (mirrors the existing --network override); startup logs announce each active endpoint.
• Removes the dead PrometheusMetricsParameters struct — defined with a Default impl but wired nowhere, and typed Multiaddr where the live path is SocketAddr.
• Fixes a stale --enable-healthcheck doc reference (the real flag is --healthcheck).

Monitoring — etc/monitoring/

A ready-to-run operator stack:

• prometheus.yml — a two-target scrape (consensus + execution + node_exporter), labeled by layer, which resolves the split-registry issue (the two endpoints live in separate registries, so they're scraped as separate jobs).
• docker-compose.yml — one-command Prometheus + Grafana + node_exporter.
• Grafana datasource auto-provisioning + a README walkthrough pointing operators at reth's official Grafana dashboard #20638.

Tests

4 config::node::tests cover backward-compat (no field → off), parsing when present, and the defaults for both endpoints. Compiles clean.

Scope / next

This is turn-on & unify only. The validator-identity metrics layer (per-validator liveness/participation, committee/epoch/reputation), a real readiness probe, and a Rayls consensus/validator Grafana dashboard are the next deliverable, tracked in #428. The full research (current-state audit + reuse-vs-build) is attached to #381.

🤖 Generated with Claude Code

[github-actions]

Claude finished @luishsr's task in 3m 20s —— View job

---

PR Review

• Gather context
• Read changed files
• Analyze code quality, bugs, security, performance
• Post review findings

---

Summary

Clean, well-scoped PR. The config changes are backward-compatible and idiomatic, the dead PrometheusMetricsParameters removal is overdue, and the monitoring stack is a genuinely useful operator addition. A few things worth addressing before merge:

---

Medium — Startup log inconsistency

Parameters::tracing() doesn't see CLI-only overrides.

Parameters::tracing() is called in Primary::new() (crates/consensus/primary/src/primary.rs:38). It logs self.metrics_address — the config-file value — not the final merged address. The CLI override in network-cli/src/node.rs:272 applies the merge into a local variable (let metrics = metrics.or(...)) rather than writing it back into parameters.metrics_address. This creates two distinct behaviours:

• Config-file-only: Parameters::tracing() fires AND node.rs fires → two identical log lines for the same endpoint.
• CLI-only: Parameters::tracing() is silent even though metrics ARE active → consensus startup logs say nothing about the metrics endpoint.

The simplest fix is to write the merged value back into parameters before handing it off to the builder:

// in network-cli/src/node.rs, after the merge
rayls_infrastructure_config.parameters.metrics_address = metrics;
rayls_infrastructure_config.parameters.reth_metrics_address = reth.reth_metrics.prometheus;

That way Parameters::tracing() always reflects the final active state, and node.rs's info! logs can be removed (or kept as a startup banner — they're at a different target:). Fix this →

---

Medium — No validation that the two addresses don't collide

metrics_address and reth_metrics_address can be set to the same SocketAddr. There is no validation — the conflict is only discovered at bind time with an OS error that doesn't name the culprit. A quick check in Parameters::tracing() or the builder would make operator errors obvious:

if self.metrics_address == self.reth_metrics_address && self.metrics_address.is_some() {
    tracing::warn!("metrics_address and reth_metrics_address are the same — one will fail to bind");
}

Fix this →

---

Low — Log message inconsistency

Parameters::tracing() (node.rs:386) emits:

"Consensus Prometheus metrics endpoint" / "Reth execution-layer Prometheus metrics endpoint"

network-cli/src/node.rs:273 emits:

"consensus Prometheus metrics enabled" / "reth execution-layer Prometheus metrics enabled"

Different capitalisation, different wording ("endpoint" vs "enabled"). If both fire (config-file path), an operator reading the structured log will see two differently-worded lines for the same event. Standardise to one message and one call site (see Medium issue above).

---

Low — Docker image tags not pinned

etc/monitoring/docker-compose.yml:18,32,49 uses :latest for all three images. A docker compose pull in six months could silently pull a breaking release. Pin to minor versions at minimum (e.g. prom/prometheus:v2.53.3). Fix this →

---

Nit — 0.0.0.0 in docs / config without a firewall callout

The README and config example both suggest 0.0.0.0:9184 / 0.0.0.0:9001. On a cloud node, this binds to the public interface. The healthcheck server has an explicit firewall warning — it would be good to add an equivalent note to the metrics_address / reth_metrics_address field docs and the README Enable the endpoints section, steering operators toward 127.0.0.1 unless Prometheus is remote.

---

What's good

• Option<SocketAddr> + #[serde(default)] is exactly the right type — zero-cost when absent, fully backward-compatible, no parsing surprises.
• Removal of PrometheusMetricsParameters is clean: wrong type (Multiaddr instead of SocketAddr), wired nowhere, leaked a TCP port allocation in its Default impl even when metrics were off.
• Option::or() merge semantics (let metrics = metrics.or(config.metrics_address)) is idiomatic and correctly prioritises the CLI flag.
• 4 tests cover all the backward-compat cases that matter (no field → None, parse-when-present, default impl).
• extra_hosts: host.docker.internal:host-gateway in docker-compose correctly handles Linux Docker Desktop's lack of native host.docker.internal resolution.
• Stale --enable-healthcheck doc fix is a good catch.

[github-actions]

Security Scan - Code

Severity: HIGH, CRITICAL

No vulnerabilities found

View scan results

Report Summary

┌─────────────────────────────────────────────┬────────────┬─────────────────┬───────────────────┐
│                   Target                    │    Type    │ Vulnerabilities │ Misconfigurations │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ Cargo.lock                                  │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/Cargo.lock      │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/fuzz/Cargo.lock │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/state-sum/Cargo.lock                    │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/tps/package-lock.json                   │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ rayls-contracts/package-lock.json           │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/chaos-network/Dockerfile                │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-network/Dockerfile               │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-replay/Dockerfile                │ dockerfile │        -        │         0         │
└─────────────────────────────────────────────┴────────────┴─────────────────┴───────────────────┘
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)

[bronxyz]

Thanks for the research done!
The clean-up part for the PrometheusMetricsParameters and the docs tidying are good.
We already expose both consensus and reth metrics in all of our scripts via --metrics 0.0.0.0:9100 and --reth-metrics 0.0.0.0:9200 and currently the entire stack is configured around that.
Let's discuss how to proceed with this.

[luishsr]

Thanks @bronxyz — and good context on the scripts.

One clarification on how this interacts with the flag-based setup, since it changes the picture: the CLI flags take precedence over config. In network-cli/src/node.rs the resolution is metrics.or(parameters.metrics_address) (same for reth), so whenever --metrics / --reth-metrics are passed they win, and the config is only consulted when the flag is absent. Both fields default to None (off), and existing parameters.yaml files parse unchanged. So your scripts and the whole 9100/9200 stack are unaffected regardless of what we decide here — there's no competing source of truth.

Given that, the only open question is whether the config path is worth keeping as an additive option:

• Keep it (additive default): lets a deployment enable metrics declaratively in the same parameters.yaml it already uses for gc_depth/gas_limit/etc., without every launch path having to remember the flags. Directly addresses the initiative's chore(deps)(deps): bump thin-vec from 0.2.14 to 0.2.18 #1 finding ("metrics are off unless a flag is passed"). Costs a little config surface area.
• Flags-only (descope the two fields): keep the dead-code cleanup + docs + the etc/monitoring operator stack you're happy with, and drop metrics_address / reth_metrics_address since every launch path already sets the flags. Less surface; we can revisit if a non-script launch path ever needs it.

I'm happy either way — which do you prefer? If flags-only, I'll strip the two config fields and keep the rest.

[bronxyz]

I'd say we can keep the params as fallback, but let's align the ports with the script defaults, so we can avoid any potential ports collision.

[procdump]

@luishsr , can you confirm that these changes won't break the local-network test environment? maybe deployment scripts as well?

[luishsr]

Checked — no break to the local-network test env or deployment scripts:

• The new config fields are purely additive. metrics_address/reth_metrics_address on Parameters are #[serde(default)], defaulting to None. I checked the actual local-network fixtures (`etc/test-network/local-validators/{,observer,validator-1..4}/parameters.yaml`) — none of them set these keys today, so they parse identically before/after this PR and metrics stay off, same as now. There's also an explicit test (`parameters_without_metrics_address_default_to_off`) covering exactly this case.
• CLI flags are unchanged in name and type. `--metrics` and `--reth-metrics` are the same flags as before (only the doc text improved); this PR only adds a config-file fallback (`metrics.or(parameters.metrics_address)`), and the CLI value always wins when passed — so any deployment script that already passes `--metrics `/`--reth-metrics ` explicitly keeps working exactly as before.
• PrometheusMetricsParameters (removed) was dead code. Grepped all of `origin/main` — it's referenced nowhere outside its own definition (not a field on Parameters, not constructed or read anywhere), so removing it doesn't change any runtime behavior for existing configs.
• The health.rs doc-comment line in this diff is the same pre-existing-stale-doc fix I dug into on feat(observability): validator health metrics + readiness probe #102 — the actual --healthcheck <PORT> flag itself is unchanged there too (see my reply on that thread for the full trace).

So: nothing here should affect the local test environment or existing deployment scripts. Let me know if there's a specific script/config outside this repo you'd like me to check against directly.

[procdump]

can you fix the fmt problem the CI reports and also rebase?

[kstoykov]

I would just suggest to pin the versions in etc/monitoring/ files to a specific version.

[luishsr]

Rebased and pushed (4bbd193, on top of latest main) with all three items addressed:

• Fixed the fmt failure CI reported (an import-list wrapping issue in crates/infrastructure/config/src/node.rs), verified against the exact pinned CI nightly (nightly-2026-06-24), not just a local generic nightly.
• @bronxyz — aligned the etc/monitoring/ example ports with what your scripts already use (--metrics 0.0.0.0:9100, --reth-metrics 0.0.0.0:9200), replacing the 9184/9001 placeholders. While doing that I caught a real collision the old placeholders had been hiding: node-exporter's own default port is also 9100, so publishing it on the host at 9100 would have collided with a node's --metrics 0.0.0.0:9100 on the same box. Moved node-exporter's host-side mapping to 9101 — Prometheus's internal scrape of it (node-exporter:9100, a docker-network address) is unaffected.
• @kstoykov — pinned the three images: prom/prometheus:v3.8.1, grafana/grafana:12.1, prom/node-exporter:v1.10.2 (checked current stable tags rather than guessing).

cargo check on the touched crates passes clean post-rebase.

[github-actions]

Claude finished @luishsr's task in 1m 50s —— View job

---

PR Review

• Gather context
• Read changed files
• Analyze code quality, bugs, security, performance
• Post review findings

---

Summary

Good rebase — the fmt failure is resolved, images are pinned, and ports are aligned with the ops scripts. Two medium findings from the prior review remain open; one is new. The monitoring stack and config changes are clean.

---

Remaining open: Medium — Startup log inconsistency (from prior review)

Parameters::tracing() doesn't see CLI-only overrides — still not addressed.

Parameters::tracing() is called at crates/consensus/primary/src/primary.rs:38 with config.parameters(), which holds the config-file values. The CLI merge in network-cli/src/node.rs:312:

let metrics = metrics.or(rayls_infrastructure_config.parameters.metrics_address);

…produces a metrics local variable but doesn't write it back into rayls_infrastructure_config.parameters.metrics_address before the config is passed to RaylsBuilder. So two distinct behaviours remain:

• Config-file-only: two log lines fire for the same endpoint — one from Parameters::tracing() ("Consensus Prometheus metrics endpoint") and one from node.rs ("consensus Prometheus metrics enabled").
• CLI-only: node.rs logs it, but Parameters::tracing() sees None → the consensus startup logs are silent about the active metrics endpoint.

Fix: write the merged value back before handing off:

// network-cli/src/node.rs — after the .or() merges
rayls_infrastructure_config.parameters.metrics_address = metrics;
rayls_infrastructure_config.parameters.reth_metrics_address = reth.reth_metrics.prometheus;

That makes Parameters::tracing() authoritative, and the node.rs info! lines can be removed (or kept as a startup banner if desired). Fix this →

---

Remaining open: Medium — No collision check between the two addresses (from prior review)

Still not addressed. If both fields are set to the same SocketAddr, the conflict is only discovered at bind-time with an OS error that doesn't name the culprit. A one-liner in Parameters::tracing() would surface it immediately:

if self.metrics_address.is_some() && self.metrics_address == self.reth_metrics_address {
    tracing::warn!("metrics_address and reth_metrics_address are the same — one will fail to bind");
}

Fix this →

---

New: Low — grafana/grafana:12.1 is a floating minor tag

etc/monitoring/docker-compose.yml:32 pins to 12.1, which is a floating tag — a future 12.1.1, 12.1.2, etc. patch will silently pull in on the next docker compose pull. prom/prometheus:v3.8.1 and prom/node-exporter:v1.10.2 are correctly pinned to the full patch version; Grafana should match that pattern (e.g. grafana/grafana:12.1.0). Fix this →

---

Remaining open: Low — Log message inconsistency (from prior review)

Still present: Parameters::tracing() emits "Consensus Prometheus metrics endpoint" while node.rs emits "consensus Prometheus metrics enabled" (different capitalisation and verb). On the config-file path, both fire — operators reading structured logs see two differently-worded events for the same endpoint. Consolidating to one call site (the write-back fix above) resolves this naturally.

---

What's fixed since the last review (good work)

• Docker images pinned to prom/prometheus:v3.8.1, prom/node-exporter:v1.10.2 ✓ (Grafana still floating — see above)
• Port alignment — etc/monitoring/prometheus.yml now targets 9100/9200, matching the ops script defaults ✓
• node-exporter host-port collision fixed — published on 9101, not 9100, with a clear comment explaining why ✓
• fmt failure resolved against pinned nightly nightly-2026-06-24 ✓
• --healthcheck doc fix in health.rs ✓
• PrometheusMetricsParameters removal — wrong type, wired nowhere, allocated a TCP port on every Default ✓
• Backward-compatible serde defaults + 4 tests covering all cases ✓
• extra_hosts: host.docker.internal:host-gateway for Linux Docker Desktop ✓