feat(multipath): add back basic metrics (#3672)

## Description

This adds back basic metrics to `feat-multipath`.


* Refactor the `conn_` metrics (they were all currently unused on
`feat-multipath`) into something meaningful: Track the total number of
opened and closed connections - their diff at any time is the number of
currently-active conne
* Adds `transport_ip_paths_added`, `transport_ip_paths_removed`,
`transport_relay_paths_added`, `transport_relay_paths_removed` counters
* Removes metrics that are unused and - IMO  - not going to come back
* Comments out metrics that are unused and likely to come back 

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist
<!-- Remove any that are not relevant. -->
- [ ] Self-review.
- [ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [ ] All breaking changes documented.
- [ ] List all breaking changes in the above "Breaking Changes" section.
- [ ] Open an issue or PR on any number0 repos that are affected by this
breaking change. Give guidance on how the updates should be handled or
do the actual updates themselves. The major ones are:
    - [ ] [`quic-rpc`](https://github.com/n0-computer/quic-rpc)
    - [ ] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip)
    - [ ] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs)
    - [ ] [`dumbpipe`](https://github.com/n0-computer/dumbpipe)
    - [ ] [`sendme`](https://github.com/n0-computer/sendme)

Author: Frando

iroh/src/magicsock/metrics.rs

@@ -1,4 +1,4 @@
-use iroh_metrics::{Counter, Histogram, MetricsGroup};
+use iroh_metrics::{Counter, MetricsGroup};
 use serde::{Deserialize, Serialize};
 
 /// Enum of metrics for the module
@@ -14,11 +14,8 @@ pub struct Metrics {
     pub send_ipv4: Counter,
     pub send_ipv6: Counter,
     pub send_relay: Counter,
-    pub send_relay_error: Counter,
 
     // Data packets (non-disco)
-    pub send_data: Counter,
-    pub send_data_network_down: Counter,
     pub recv_data_relay: Counter,
     pub recv_data_ipv4: Counter,
     pub recv_data_ipv6: Counter,
@@ -38,15 +35,23 @@ pub struct Metrics {
 
     /*
      * Connection Metrics
+     *
+     * These all only count connections that completed the TLS handshake successfully. This means
+     * that short lived 0RTT connections are potentially not included in these counts.
      */
-    /// The number of direct connections we have made to peers.
-    pub num_direct_conns_added: Counter,
-    /// The number of direct connections we have lost to peers.
-    pub num_direct_conns_removed: Counter,
-    /// The number of connections to peers we have added over relay.
-    pub num_relay_conns_added: Counter,
-    /// The number of connections to peers we have removed over relay.
-    pub num_relay_conns_removed: Counter,
+    /// Number of connections opened (only handshaked connections are counted).
+    pub num_conns_opened: Counter,
+    /// Number of connections closed (only handshaked connections are counted).
+    pub num_conns_closed: Counter,
+
+    /// Number of IP transport paths opened.
+    pub transport_ip_paths_added: Counter,
+    /// Number of IP transport paths closed.
+    pub transport_ip_paths_removed: Counter,
+    /// Number of relay transport paths opened.
+    pub transport_relay_paths_added: Counter,
+    /// Number of relay transport paths closed.
+    pub transport_relay_paths_removed: Counter,
 
     pub actor_tick_main: Counter,
     pub actor_tick_msg: Counter,
@@ -55,36 +60,4 @@ pub struct Metrics {
     pub actor_tick_direct_addr_heartbeat: Counter,
     pub actor_link_change: Counter,
     pub actor_tick_other: Counter,
-
-    /// Number of endpoints we have attempted to contact.
-    pub endpoints_contacted: Counter,
-    /// Number of endpoints we have managed to contact directly.
-    pub endpoints_contacted_directly: Counter,
-
-    /// Number of connections with a successful handshake.
-    pub connection_handshake_success: Counter,
-    /// Number of connections with a successful handshake that became direct.
-    pub connection_became_direct: Counter,
-    /// Histogram of connection latency in milliseconds across all endpoint connections.
-    #[default(Histogram::new(vec![1.0, 5.0, 10.0, 25.0, 50.0, 100.0, 250.0, 500.0, 1000.0, f64::INFINITY]))]
-    pub connection_latency_ms: Histogram,
-
-    /*
-     * Path Congestion Metrics
-     */
-    /// Number of times a path was marked as outdated due to consecutive ping failures.
-    pub path_marked_outdated: Counter,
-    /// Number of ping failures recorded across all paths.
-    pub path_ping_failures: Counter,
-    /// Number of consecutive failure resets (path recovered).
-    pub path_failure_resets: Counter,
-    /// Histogram of packet loss rates (0.0-1.0) observed on UDP paths.
-    #[default(Histogram::new(vec![0.0, 0.01, 0.05, 0.1, 0.2, 0.5, 1.0]))]
-    pub path_packet_loss_rate: Histogram,
-    /// Histogram of RTT variance (in milliseconds) as a congestion indicator.
-    #[default(Histogram::new(vec![0.0, 1.0, 5.0, 10.0, 20.0, 50.0, 100.0, 200.0]))]
-    pub path_rtt_variance_ms: Histogram,
-    /// Histogram of path quality scores (0.0-1.0).
-    #[default(Histogram::new(vec![0.0, 0.3, 0.5, 0.7, 0.85, 0.95, 1.0]))]
-    pub path_quality_score: Histogram,
 }

iroh/src/magicsock/remote_map/remote_state.rs

@@ -192,15 +192,15 @@ impl RemoteStateActor {
         Self {
             endpoint_id,
             local_endpoint_id,
-            metrics,
+            metrics: metrics.clone(),
             local_direct_addrs,
             relay_mapped_addrs,
             discovery,
             connections: FxHashMap::default(),
             connections_close: Default::default(),
             path_events: Default::default(),
             addr_events: Default::default(),
-            paths: Default::default(),
+            paths: RemotePathState::new(metrics),
             last_holepunch: None,
             selected_path: Default::default(),
             scheduled_holepunch: None,
@@ -273,11 +273,7 @@ impl RemoteStateActor {
                     self.trigger_holepunching().await;
                 }
                 Some(conn_id) = self.connections_close.next(), if !self.connections_close.is_empty() => {
-                    self.connections.remove(&conn_id);
-                    if self.connections.is_empty() {
-                        trace!("last connection closed - clearing selected_path");
-                        self.selected_path.set(None).ok();
-                    }
+                    self.handle_connection_close(conn_id);
                 }
                 res = self.local_direct_addrs.updated() => {
                     if let Err(n0_watcher::Disconnected) = res {
@@ -397,6 +393,7 @@ impl RemoteStateActor {
     ) {
         let pub_open_paths = Watchable::default();
         if let Some(conn) = handle.upgrade() {
+            self.metrics.num_conns_opened.inc();
             // Remove any conflicting stable_ids from the local state.
             let conn_id = ConnId(conn.stable_id());
             self.connections.remove(&conn_id);
@@ -487,6 +484,16 @@ impl RemoteStateActor {
         self.trigger_discovery();
     }
 
+    fn handle_connection_close(&mut self, conn_id: ConnId) {
+        if self.connections.remove(&conn_id).is_some() {
+            self.metrics.num_conns_closed.inc();
+        }
+        if self.connections.is_empty() {
+            trace!("last connection closed - clearing selected_path");
+            self.selected_path.set(None).ok();
+        }
+    }
+
     fn handle_discovery_item(&mut self, item: Option<Result<DiscoveryItem, DiscoveryError>>) {
         match item {
             None => {
@@ -1052,7 +1059,6 @@ impl ConnectionState {
         self.paths.insert(path_id, remote.clone());
         self.open_paths.insert(path_id, remote.clone());
         self.path_ids.insert(remote, path_id);
-
         self.update_pub_path_info();
     }
 

iroh/src/magicsock/remote_map/remote_state/path_state.rs

@@ -1,6 +1,9 @@
 //! The state kept for each network path to a remote endpoint.
 
-use std::collections::{HashMap, HashSet, VecDeque};
+use std::{
+    collections::{HashMap, HashSet, VecDeque},
+    sync::Arc,
+};
 
 use n0_error::e;
 use n0_future::time::Instant;
@@ -9,7 +12,7 @@ use tokio::sync::oneshot;
 use tracing::trace;
 
 use super::Source;
-use crate::{discovery::DiscoveryError, magicsock::transports};
+use crate::{discovery::DiscoveryError, magicsock::transports, metrics::MagicsockMetrics};
 
 /// Maximum number of IP paths we keep around per endpoint.
 pub(super) const MAX_IP_PATHS: usize = 30;
@@ -23,7 +26,7 @@ pub(super) const MAX_INACTIVE_IP_PATHS: usize = 10;
 ///
 /// Also stores a list of resolve requests which are triggered once at least one path is known,
 /// or once this struct is notified of a failed discovery run.
-#[derive(Debug, Default)]
+#[derive(Debug)]
 pub(super) struct RemotePathState {
     /// All possible paths we are aware of.
     ///
@@ -32,6 +35,7 @@ pub(super) struct RemotePathState {
     paths: FxHashMap<transports::Addr, PathState>,
     /// Pending resolve requests from [`Self::resolve_remote`].
     pending_resolve_requests: VecDeque<oneshot::Sender<Result<(), DiscoveryError>>>,
+    metrics: Arc<MagicsockMetrics>,
 }
 
 /// Describes the usability of this path, i.e. whether it has ever been opened,
@@ -51,10 +55,21 @@ pub(super) enum PathStatus {
 }
 
 impl RemotePathState {
+    pub(super) fn new(metrics: Arc<MagicsockMetrics>) -> Self {
+        Self {
+            paths: Default::default(),
+            pending_resolve_requests: Default::default(),
+            metrics,
+        }
+    }
     /// Insert a new address of an open path into our list of paths.
     ///
     /// This will emit pending resolve requests and trigger pruning paths.
     pub(super) fn insert_open_path(&mut self, addr: transports::Addr, source: Source) {
+        match addr {
+            transports::Addr::Ip(_) => self.metrics.transport_ip_paths_added.inc(),
+            transports::Addr::Relay(_, _) => self.metrics.transport_relay_paths_added.inc(),
+        };
         let state = self.paths.entry(addr).or_default();
         state.status = PathStatus::Open;
         state.sources.insert(source.clone(), Instant::now());
@@ -68,6 +83,14 @@ impl RemotePathState {
     /// `RemotePathState`
     pub(super) fn abandoned_path(&mut self, addr: &transports::Addr) {
         if let Some(state) = self.paths.get_mut(addr) {
+            if matches!(state.status, PathStatus::Open) {
+                match addr {
+                    transports::Addr::Ip(_) => self.metrics.transport_ip_paths_removed.inc(),
+                    transports::Addr::Relay(_, _) => {
+                        self.metrics.transport_relay_paths_removed.inc()
+                    }
+                };
+            }
             match state.status {
                 PathStatus::Open | PathStatus::Inactive(_) => {
                     state.status = PathStatus::Inactive(Instant::now());
@@ -491,7 +514,7 @@ mod tests {
 
     #[test]
     fn test_insert_open_path() {
-        let mut state = RemotePathState::default();
+        let mut state = RemotePathState::new(Default::default());
         let addr = ip_addr(1000);
         let source = Source::Udp;
 
@@ -509,25 +532,31 @@ mod tests {
 
     #[test]
     fn test_abandoned_path() {
-        let mut state = RemotePathState::default();
+        let metrics = Arc::new(MagicsockMetrics::default());
+        let mut state = RemotePathState::new(metrics.clone());
 
         // Test: Open goes to Inactive
         let addr_open = ip_addr(1000);
         state.insert_open_path(addr_open.clone(), Source::Udp);
         assert!(matches!(state.paths[&addr_open].status, PathStatus::Open));
+        assert_eq!(metrics.transport_ip_paths_added.get(), 1);
 
         state.abandoned_path(&addr_open);
         assert!(matches!(
             state.paths[&addr_open].status,
             PathStatus::Inactive(_)
         ));
+        assert_eq!(metrics.transport_ip_paths_added.get(), 1);
+        assert_eq!(metrics.transport_ip_paths_removed.get(), 1);
 
         // Test: Inactive stays Inactive
         state.abandoned_path(&addr_open);
         assert!(matches!(
             state.paths[&addr_open].status,
             PathStatus::Inactive(_)
         ));
+        assert_eq!(metrics.transport_ip_paths_added.get(), 1);
+        assert_eq!(metrics.transport_ip_paths_removed.get(), 1);
 
         // Test: Unknown goes to Unusable
         let addr_unknown = ip_addr(2000);
@@ -536,18 +565,33 @@ mod tests {
             state.paths[&addr_unknown].status,
             PathStatus::Unknown
         ));
+        assert_eq!(metrics.transport_ip_paths_added.get(), 1);
+        assert_eq!(metrics.transport_ip_paths_removed.get(), 1);
 
         state.abandoned_path(&addr_unknown);
         assert!(matches!(
             state.paths[&addr_unknown].status,
             PathStatus::Unusable
         ));
+        assert_eq!(metrics.transport_ip_paths_added.get(), 1);
+        assert_eq!(metrics.transport_ip_paths_removed.get(), 1);
 
         // Test: Unusable stays Unusable
         state.abandoned_path(&addr_unknown);
         assert!(matches!(
             state.paths[&addr_unknown].status,
             PathStatus::Unusable
         ));
+        assert_eq!(metrics.transport_ip_paths_added.get(), 1);
+        assert_eq!(metrics.transport_ip_paths_removed.get(), 1);
+
+        // Test: Unusable can go to open
+        state.insert_open_path(addr_unknown.clone(), Source::Udp);
+        assert!(matches!(
+            state.paths[&addr_unknown].status,
+            PathStatus::Open
+        ));
+        assert_eq!(metrics.transport_ip_paths_added.get(), 2);
+        assert_eq!(metrics.transport_ip_paths_removed.get(), 1);
     }
 }