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

@@ -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 {

iroh-net/src/magicsock/node_map/node_state.rs

@@ -295,15 +295,59 @@ 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,
                 node = %self.node_id.fmt_short(),
                 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(),
                     );
                 }