From d662bfc663ad956bbb38716bd5b8022a699bfce4 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Fri, 2 Aug 2024 17:25:33 +0200 Subject: [PATCH] ref(iroh-net): Remove need for relay info in best_addr (#2579) ## Description This moves metrics for when switches between direct connections and relayed connections to the NodeState. This is after all where this decision is made. BestAddr only knows about the best UDP address, it has no business keeping track of relays. Cleaning this up enables further refactoring of BestAddr and PathState which need to improve because they depend on each other, but are hindered by the relay state sneaking into there. ## Breaking Changes None ## Notes & open questions These metrics ignore the fact that we have mixed as a possibility so over-simplify the situation. This makes the logic to increment them complex, which in turn makes it no one will really know what they mean. I need this to move on with #2546 so don't really want to get into designing our metrics though. I believe this way the keep the same behaviour. ## Change checklist - [x] Self-review. - [x] 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. - [x] Tests if relevant. - [x] All breaking changes documented. --- iroh-net/src/magicsock/node_map/best_addr.rs | 25 +--------- iroh-net/src/magicsock/node_map/node_state.rs | 50 +++++++++++++++++-- 2 files changed, 48 insertions(+), 27 deletions(-) diff --git a/iroh-net/src/magicsock/node_map/best_addr.rs b/iroh-net/src/magicsock/node_map/best_addr.rs index 95b47b361f..d5550b7c49 100644 --- a/iroh-net/src/magicsock/node_map/best_addr.rs +++ b/iroh-net/src/magicsock/node_map/best_addr.rs @@ -5,11 +5,8 @@ use std::{ time::{Duration, Instant}, }; -use iroh_metrics::inc; use tracing::{debug, info}; -use crate::magicsock::metrics::Metrics as MagicsockMetrics; - /// How long we trust a UDP address as the exclusive path (without using relay) without having heard a Pong reply. const TRUST_UDP_ADDR_DURATION: Duration = Duration::from_millis(6500); @@ -93,12 +90,6 @@ impl BestAddr { let old = self.0.take(); if let Some(old_addr) = old.as_ref().map(BestAddrInner::addr) { info!(?reason, ?has_relay, %old_addr, "clearing best_addr"); - // no longer relying on the direct connection - inc!(MagicsockMetrics, num_direct_conns_removed); - if has_relay { - // we are now relying on the relay connection, add a relay conn - inc!(MagicsockMetrics, num_relay_conns_added); - } } } @@ -126,16 +117,15 @@ impl BestAddr { latency: Duration, source: Source, confirmed_at: Instant, - has_relay: bool, ) { match self.0.as_mut() { None => { - self.insert(addr, latency, source, confirmed_at, has_relay); + self.insert(addr, latency, source, confirmed_at); } Some(state) => { let candidate = AddrLatency { addr, latency }; if !state.is_trusted(confirmed_at) || candidate.is_better_than(&state.addr) { - self.insert(addr, latency, source, confirmed_at, has_relay); + self.insert(addr, latency, source, confirmed_at); } else if state.addr.addr == addr { state.confirmed_at = confirmed_at; state.trust_until = Some(source.trust_until(confirmed_at)); @@ -160,7 +150,6 @@ impl BestAddr { latency: Duration, source: Source, confirmed_at: Instant, - has_relay: bool, ) { let trust_until = source.trust_until(confirmed_at); @@ -184,22 +173,12 @@ impl BestAddr { "selecting new direct path for node" ); } - let was_empty = self.is_empty(); let inner = BestAddrInner { addr: AddrLatency { addr, latency }, trust_until: Some(trust_until), confirmed_at, }; self.0 = Some(inner); - if was_empty && has_relay { - // we now have a direct connection, adjust direct connection count - inc!(MagicsockMetrics, num_direct_conns_added); - if has_relay { - // we no longer rely on the relay connection, decrease the relay connection - // count - inc!(MagicsockMetrics, num_relay_conns_removed); - } - } } pub fn state(&self, now: Instant) -> State { diff --git a/iroh-net/src/magicsock/node_map/node_state.rs b/iroh-net/src/magicsock/node_map/node_state.rs index 4c98d0b5bc..df3e88a82d 100644 --- a/iroh-net/src/magicsock/node_map/node_state.rs +++ b/iroh-net/src/magicsock/node_map/node_state.rs @@ -295,8 +295,8 @@ impl NodeState { (None, Some(relay_url)) => ConnectionType::Relay(relay_url), (None, None) => ConnectionType::None, }; - if self.conn_type.update(typ).is_ok() { - let typ = self.conn_type.get(); + if let Ok(prev_typ) = self.conn_type.update(typ.clone()) { + // The connection type has changed. event!( target: "events.net.conn_type.changed", Level::DEBUG, @@ -304,6 +304,50 @@ impl NodeState { conn_type = ?typ, ); info!(%typ, "new connection type"); + + // Update some metrics + match (prev_typ, typ) { + (ConnectionType::Direct(_), ConnectionType::Direct(_)) => (), + (ConnectionType::Direct(_), ConnectionType::Relay(_)) => { + inc!(MagicsockMetrics, num_direct_conns_removed); + inc!(MagicsockMetrics, num_relay_conns_added); + } + (ConnectionType::Direct(_), ConnectionType::Mixed(_, _)) => { + inc!(MagicsockMetrics, num_direct_conns_removed); + inc!(MagicsockMetrics, num_relay_conns_added); + } + (ConnectionType::Direct(_), ConnectionType::None) => { + inc!(MagicsockMetrics, num_direct_conns_removed) + } + (ConnectionType::Relay(_), ConnectionType::Direct(_)) => { + inc!(MagicsockMetrics, num_direct_conns_added); + inc!(MagicsockMetrics, num_relay_conns_removed); + } + (ConnectionType::Relay(_), ConnectionType::Relay(_)) => (), + (ConnectionType::Relay(_), ConnectionType::Mixed(_, _)) => (), + (ConnectionType::Relay(_), ConnectionType::None) => { + inc!(MagicsockMetrics, num_relay_conns_removed) + } + (ConnectionType::Mixed(_, _), ConnectionType::Direct(_)) => { + inc!(MagicsockMetrics, num_direct_conns_added); + inc!(MagicsockMetrics, num_relay_conns_removed); + } + (ConnectionType::Mixed(_, _), ConnectionType::Relay(_)) => (), + (ConnectionType::Mixed(_, _), ConnectionType::Mixed(_, _)) => (), + (ConnectionType::Mixed(_, _), ConnectionType::None) => { + inc!(MagicsockMetrics, num_relay_conns_removed) + } + (ConnectionType::None, ConnectionType::Direct(_)) => { + inc!(MagicsockMetrics, num_direct_conns_added) + } + (ConnectionType::None, ConnectionType::Relay(_)) => { + inc!(MagicsockMetrics, num_relay_conns_added) + } + (ConnectionType::None, ConnectionType::Mixed(_, _)) => { + inc!(MagicsockMetrics, num_relay_conns_added) + } + (ConnectionType::None, ConnectionType::None) => (), + } } (best_addr, relay_url) } @@ -367,7 +411,6 @@ impl NodeState { pong.latency, best_addr::Source::BestCandidate, pong.pong_at, - self.relay_url.is_some(), ) } } @@ -916,7 +959,6 @@ impl NodeState { latency, best_addr::Source::ReceivedPong, now, - self.relay_url.is_some(), ); }