Skip to content

fix(p2p): populate relay_connections, gate ping metrics to cluster peers - #576

Open
iamquang95 wants to merge 1 commit into
mainfrom
feat/fix-relay-grafana
Open

fix(p2p): populate relay_connections, gate ping metrics to cluster peers#576
iamquang95 wants to merge 1 commit into
mainfrom
feat/fix-relay-grafana

Conversation

@iamquang95

Copy link
Copy Markdown
Collaborator

Two p2p metric fixes for the Grafana dashboard:

  • Extra entry on the peer panels: p2p_ping_latency_secs / ping_success / ping_error_total were emitted for every pinged peer, so the relay server appeared as a phantom cluster peer on max(p2p_ping_success) by (peer). All three are now gated to known cluster peers (Charon's ClusterPeerIDs allowlist).
  • Blank "Connected Relays" panel: p2p_relay_connections had no writer. It is now set from RelayManager1 while a reservation is held, 0 otherwise.

@emlautarom1

Copy link
Copy Markdown
Collaborator

@claude

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Claude finished @emlautarom1's task in 4m 28s —— View job


Review — PR #576

  • Gather context (diff, Charon reference)
  • Review p2p.rs ping-metric gating
  • Review relay/manager.rs relay_connections gauge
  • Review tests
  • Post review

Both fixes are correct and match the Go source. I verified the cited Charon references directly:

  • charon/app/app.go:390 passes manifest.ClusterPeerIDs(cluster) into p2p.NewPingService, and charon/p2p/ping.go:35-65 only spawns ping loops for that slice (skipping self). NewPingService has exactly one caller — the relay command never starts one — so a relay genuinely never appears in Charon's p2p_ping_*. Gating at emission time in record_ping_metrics (crates/p2p/src/p2p.rs:708) is the right adaptation given libp2p's ping behaviour pings every connected peer.
  • charon/p2p/relay.go:42 sets relayConnGauge…Set(0) before each circuit.Reserve, :70 sets 1 after success, :101 back to 0 on refresh. So keying the gauge on RelayConnectionState::Reserved — not on transport connections — is faithful, and the doc comment's reasoning about the single peer label vs. tcp+quic is sound.

Nice catch that this also un-breaks insufficient_connected_peers in crates/app/src/health/checks.rs:100: it uses count_non_zero_labels() over p2p_ping_success, so a relay reporting 1 was silently inflating the connected-peer count and suppressing the alert. Worth mentioning in the PR description — it's a functional fix, not just cosmetic.

CI is green on this branch (7/7). I could not run cargo test/clippy locally (no permission for cargo in this session), so my review is static plus the CI signal.

Findings

All of these are non-blocking.

1. p2p_ping_success can stay stuck at 1 after an orderly disconnectcrates/p2p/src/p2p.rs:708

Pre-existing, not introduced here, but it undercuts the same health check this PR is fixing. Node::handle_event has no ConnectionClosed arm, and rust-libp2p's ping handler is simply dropped when a connection closes — no failure event is emitted. An unreachable peer usually trips the ping timeout first (so the gauge does reach 0), but a clean shutdown by the remote closes the connection without any ping::Event, leaving ping_success{peer}=1 forever. Charon can't hit this because pingPeer keeps retrying a disconnected peer and records the error. conn_logger.rs already tracks ConnectionClosed for peer_connection_types and would be the natural place to zero the gauge. Fix this →

2. Related parity gap: no series at all for a never-connected cluster peer

Charon emits p2p_ping_success{peer}=0 for a cluster peer it can't reach, because the ping loop runs regardless of connectivity. Pluto only writes on a libp2p ping event, which requires a connection — so an offline peer produces no series rather than a 0. count_non_zero_labels is unaffected, but max(p2p_ping_success) by (peer) shows a missing line instead of a flat zero on the Grafana panel. Also pre-existing; worth a follow-up issue rather than widening this PR.

3. report_relay_connection fires on every set_relay_state, including no-op transitionscrates/p2p/src/relay/manager.rs:213

set_relay_state already computes prev and only logs when prev != Some(next); the gauge write (plus a peer_name String allocation) happens unconditionally. Harmless at this call rate, but folding it into the same prev != Some(next) guard would be tidier — with the caveat that the initial Dialing write must still happen so the relay shows up as 0 before its first reservation, which the current placement gets right.

4. spawn_relay_server silently changes relay_circuit.rs's circuit budgetcrates/p2p/tests/common/mod.rs

The extracted helper switches from the old explicit relay::Config { max_circuit_bytes: 32 * 1024 * 1024, … } to relay::Config::default(), whose max_circuit_bytes is 1 << 17 (128 KiB) — a ~256× reduction. Keeping the default rate limiters is the right call and well justified in the comment, but the byte budget is a side effect. It passes today because the test only establishes a circuit; if that test ever pushes real payloads it will fail confusingly. Consider relay::Config { max_circuit_bytes: 32 << 20, ..Default::default() } so the intent survives.

5. The E2E test can't distinguish "gated" from "ping metrics entirely broken"crates/p2p/tests/relay_metrics.rs:48

The client is built with P2PContext::new(Vec::<PeerId>::new()), so every peer is gated and the three !contains(&relay_label) assertions would still pass if record_ping_metrics returned unconditionally. The unit tests in p2p.rs do cover the positive path, so coverage isn't actually missing — but adding a second, known cluster peer to the context and asserting its series is present would make this test a real discriminator.

6. Minor flakiness surface in the same test

The loop waits for both reserved and pinged under a single 30s TEST_TIMEOUT, and libp2p's default ping interval is 15s. There's headroom, but not much under a loaded CI runner. Not worth changing unless it actually flakes.

Nits

  • The doc comment on record_ping_metrics is genuinely good — it explains why Pluto needs a gate Charon doesn't, and why a relay-server node deliberately publishes nothing. Same for the report_relay_connection rationale about last-writer-wins. Please keep this style.
  • crates/relay-server/src/metrics.rs:17 (relay_p2p_ping_latency) also has no writer — the same class of bug as relay_connections, just on the relay-server side. crates/cli/src/commands/relay.rs:529 only asserts the name is exported, so it wouldn't catch it. Out of scope here; flagging it since this PR is about blank dashboard panels.

Overall: LGTM. The Charon parity is verified, the state-machine coverage in manager/tests.rs is thorough (including the redial_relay drop path that bypasses set_relay_state), and the test extraction into tests/common/mod.rs is a real reduction in duplication.
Branch: feat/fix-relay-grafana

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.

2 participants