feat(network): add connection lifecycle metrics by gustavovalverde · Pull Request #11135 · ZcashFoundation/zebra

gustavovalverde · 2026-07-28T18:08:39Z

Motivation

Existing peer metrics begin inside the Zcash handshake, merge several Mainnet and Testnet gauges, and classify transport resets wrapped by the codec as generic serialization failures. Operators therefore cannot distinguish TCP reachability failures, handshake closures or resets, Version-policy rejections, and successful admission without correlating logs or capturing traffic.

Solution

Add connection-attempt and terminal-outcome counters labeled by bounded network, direction, address_family, stage, and outcome values.
Add Version-message and cancellation-safe Version-outcome counters that classify self-reported BIP-14 user agents as zakura, zebra, legacy_zcashd, or other.
Preserve I/O error meaning when a codec wraps connection resets and related transport failures in SerializationError::Io.
Add network labels to peer-set, in-flight-handshake, and address-book gauges so network instances in the same process do not overwrite each other.
Keep peer IPs and raw user agents out of the new Prometheus labels; exact self-reported user agents remain available in debug logs.

Important

Existing gauge series gain a network label, so dashboards must select or aggregate that label after rollout. Outbound whole-connector timeouts use the tcp_or_handshake stage because that timeout boundary cannot identify which inner operation was pending; inbound and inner Tokio timeouts use handshake.

This PR does not add a second per-peer snapshot store. The existing address-book and peer-set owners remain authoritative, avoiding duplicated lifecycle state that could become stale.

Related issues

#10164 introduced the peer-health metric work that these lifecycle counters extend.
#11061 documents an operational peer-admission failure that requires this failure breakdown to diagnose from aggregate telemetry.

Tests

cargo fmt --all -- --check
cargo clippy -p zebra-network --all-targets -- -D warnings
cargo test -p zebra-network -- --skip listener_bans_zcashd_compat_peer_before_reserved_slot --skip listener_reserves_one_zcashd_compat_inbound_slot --skip listener_zcashd_compat_reconnect_bypasses_recent_ip_limit

The filtered tests require binding loopback source addresses that macOS rejects with EADDRNOTAVAIL; the remaining 209 unit tests, the acceptance test, and documentation tests pass.

AI Disclosure

AI tools were used: OpenAI Codex for implementation, tests, and the PR description

PR Checklist

The PR title follows conventional commits format: type(scope): description
The PR follows the contribution guidelines.
This change was discussed with the team beforehand.
The solution is tested.
The documentation and changelogs are up to date.

Copilot

Pull request overview

Adds bounded, network-aware connection lifecycle metrics to zebra-network, improving operational visibility without exposing peer identifiers.

Changes:

Tracks connection attempts, outcomes, and remote Version classifications.
Adds network labels to peer-set, handshake, and address-book gauges.
Documents the new metrics and dashboard impact.

Risk: Timeout paths can be misclassified or omit terminal Version outcomes. Tests should cover inner handshake timeouts and peers stalling before Verack.

Process note: No pre-discussed issue link is included.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`zebra-network/src/peer/handshake.rs`	Records Version-message lifecycle metrics.
`zebra-network/src/peer/connection_metrics.rs`	Defines bounded labels and error classification.
`zebra-network/src/peer.rs`	Registers the metrics module.
`zebra-network/src/peer_set/set.rs`	Adds network labels to peer gauges.
`zebra-network/src/peer_set/initialize.rs`	Instruments inbound and outbound connection attempts.
`zebra-network/src/address_book.rs`	Adds network labels to address-book gauges.
`zebra-network/CHANGELOG.md`	Documents library-visible metrics changes.
`CHANGELOG.md`	Documents operator-visible metrics changes.

alchemydc

Thanks — the metric design here is solid (bounded labels, attempt/outcome pairing reconciles on every path I traced, isolated connections consistently excluded). Both Copilot findings are real, and the first is worse than it looks:

1. Tokio `Elapsed` unclassified — this hits every inbound handshake timeout

classify_connection_error only checks tower::timeout::error::Elapsed, but the whole-handshake timeout at handshake.rs:1226 is tokio::time::timeout, and its Elapsed is boxed directly into BoxError at handshake.rs:1238 (the From<tokio::time::error::Elapsed> for HandshakeError impl in error.rs is not applied at that site).

The timer race makes this matter more for inbound than outbound. Both timers use the same HANDSHAKE_TIMEOUT, tower's Timeout polls the inner future before its own sleep, and the inner tokio deadline is created first (inside inner.call):

Outbound: the outer tower timer starts before TcpStream::connect, so it usually fires first → correctly labeled tcp_or_handshake/timeout.
Inbound: the handshaker is called with TCP already accepted, the deadlines are essentially equal, and the inner tokio timer wins → every inbound handshake timeout lands in stage="unknown", outcome="other".

Since handshake timeouts are one of the most common failure modes this metric exists to surface, that's the main use case going into the junk bucket. There's existing precedent for checking both types at zebrad/src/application.rs:377-378.

Suggested fix: add an error.is::<tokio::time::error::Elapsed>() arm mapping to stage="handshake", outcome="timeout" (the inner timer only covers post-TCP work, so handshake is the accurate stage), and add a unit test for it (tokio::time::timeout(Duration::ZERO, future::pending())). I'd avoid converting to HandshakeError::Timeout at handshake.rs:1238 instead — that changes the boxed error type other consumers see (e.g. connection.rs:1436).

2. Version outcome accounting is not cancellation-safe — confirmed, lower severity

record_remote_version_received fires at Version decode, but the outcome only fires on explicit returns. When the outer tokio timeout fires, it drops the negotiate_version future mid-await (e.g. a peer stalling between Version and Verack), so version.messages.total permanently drifts above version.outcomes.total — exactly for the stall case the ratio is meant to catch. Metrics skew only, no correctness impact.

Suggested fix: replace the record_version_error closure with a small Drop guard created right after record_remote_version_received (owning the network, direction/addr labels, and implementation label), defused by explicit success/error recording; an undefused drop records outcome="cancelled". That's cancellation-safe and keeps the bounded implementation label.

Smaller points

The outer_timeout_keeps_its_combined_stage test only covers the tower Elapsed path — the tokio path (the one that actually fires inbound) is untested.
stage="tcp_or_handshake" is misleading for inbound tower timeouts, where TCP is already established. Bounded and conservative, but worth a doc note or an inbound-specific value.
The new debug!(remote_user_agent...) duplicates handshake.rs:715, which already logs the whole Version message (user agent included) at debug.
connection_metrics.rs is missing a //! module doc header.
Layering nit: address_book.rs and peer_set/set.rs reach into peer::connection_metrics for network_kind_label; a more neutral home (or a method on NetworkKind) would be cleaner.
Follow-up candidate (out of scope here): the pre-existing zcash.net.peers.obsolete/.connected counters label by raw remote_ip and user_agent — the unbounded cardinality this PR deliberately avoids. Worth an issue to bound those too.
Could you add the issue link for the prior discussion to the PR body?

gustavovalverde · 2026-07-28T22:27:39Z

@alchemydc Addressed in e1fc2d517: both timeout types are classified, Version outcomes are cancellation-safe, inbound/outbound timeout stages are split, duplicate logging was removed, module docs were added, and metrics moved to the crate root. Related issues are now linked; the pre-existing raw-IP metrics remain a follow-up.

alchemydc

Verified e1fc2d517 against the review point by point — everything is addressed:

Tokio Elapsed now classifies as handshake/timeout without changing the boxed error type, with the unit test covering the real construction path.
RemoteVersionOutcomeGuard is cancellation-safe, idempotent, and keeps the bounded labels; the local-recorder test verifying one terminal outcome per decoded Version is a nice touch.
The direction-aware stage for tower timeouts (inbound → handshake) goes beyond what I asked for — thanks.
Smaller points (duplicate debug log, module doc, crate-root move, issue links) all handled.

Two non-blocking notes:

The shard-5 failure (wallet transparent balance should grow after mining) looks like the known flaky timing assertion in the integration-tests repo, unrelated to this change — worth a re-run.
Please file the follow-up issue for the pre-existing zcash.net.peers.obsolete/.connected counters that label by raw remote_ip/user_agent when you get a chance.

mergify · 2026-07-28T23:09:18Z

Link the connection lifecycle metrics entry to PR #11135 so the operator-facing release notes identify their source.

Link the connection lifecycle metrics entry to PR #11135 so crate consumers can find the implementation and review context.

Link the network-labelled gauge entry to PR #11135 so the final behavior is traceable to its source change.

Link the network-labelled gauge entry to PR #11135 so the operator-facing release notes identify its source.