metrics(netcheck): Add the basic netcheck metrics

src/hp/netcheck.rs

@@ -27,7 +27,11 @@ use tokio_util::sync::CancellationToken;
 use tracing::{debug, debug_span, error, info, instrument, trace, warn, Instrument};
 use trust_dns_resolver::TokioAsyncResolver;
 
-use crate::net::{interfaces, ip::to_canonical};
+use crate::{
+    metrics::inc,
+    metrics::netcheck::NetcheckMetrics,
+    net::{interfaces, ip::to_canonical},
+};
 
 use self::probe::{Probe, ProbePlan, ProbeProto};
 
@@ -219,13 +223,14 @@ impl Client {
     /// There is an implicit queue here which may drop packets if the actor does not keep up
     /// consuming them.
     pub(crate) fn receive_stun_packet(&self, payload: Bytes, src: SocketAddr) {
-        self.addr
-            .try_send(ActorMessage::StunPacket {
+        if let Err(mpsc::error::TrySendError::Full(_)) =
+            self.addr.try_send(ActorMessage::StunPacket {
                 payload,
                 from_addr: src,
             })
-            .ok();
-        // TODO: metric to count number of discarded stun packets due to full inbox
+        {
+            inc!(NetcheckMetrics::StunPacketsDropped);
+        };
     }
 
     /// Runs a netcheck, returning the report.
@@ -504,7 +509,6 @@ struct ReportState {
     last: Option<Arc<Report>>,
 }
 
-// TODO: Can we not make this pub?
 #[derive(Debug)]
 pub(crate) struct Inflight {
     tx: stun::TransactionId,
@@ -917,10 +921,9 @@ async fn run_probe(
 
     match probe {
         Probe::Ipv4 { .. } => {
-            // TODO:
-            // metricSTUNSend4.Add(1)
             if let Some(ref pc4) = pc4 {
                 let n = pc4.send_to(&req, addr).await;
+                inc!(NetcheckMetrics::StunPacketsSentIpv4);
                 debug!(%addr, send_res=?n, "sending probe IPV4");
                 // TODO:  || neterror.TreatAsLostUDP(err)
                 if n.is_ok() && n.unwrap() == req.len() {
@@ -935,10 +938,9 @@ async fn run_probe(
             }
         }
         Probe::Ipv6 { .. } => {
-            // TODO:
-            // metricSTUNSend6.Add(1)
             if let Some(ref pc6) = pc6 {
                 let n = pc6.send_to(&req, addr).await;
+                inc!(NetcheckMetrics::StunPacketsSentIpv6);
                 debug!(%addr, snd_res=?n, "sending probe IPV6");
                 // TODO:  || neterror.TreatAsLostUDP(err)
                 if n.is_ok() && n.unwrap() == req.len() {
@@ -1353,12 +1355,14 @@ impl Actor {
                 }
                 Err(err) => {
                     warn!("generate report timed out: {:?}", err);
+                    inc!(NetcheckMetrics::ReportsError);
                     addr.send(ActorMessage::ReportAborted)
                         .await
                         .unwrap_or_else(|_| error!("netcheck.report_state: netcheck actor lost"));
                 }
                 Ok(Err(err)) => {
                     warn!("failed to generate report: {:?}", err);
+                    inc!(NetcheckMetrics::ReportsError);
                     addr.send(ActorMessage::ReportAborted)
                         .await
                         .unwrap_or_else(|_| error!("netcheck.report_state: netcheck actor lost"));
@@ -1409,10 +1413,9 @@ impl Actor {
             self.reports.last = None; // causes ProbePlan::new below to do a full (initial) plan
             self.reports.next_full = false;
             self.reports.last_full = now;
-
-            // TODO
-            // metricNumGetReportFull.Add(1);
+            inc!(NetcheckMetrics::ReportsFull);
         }
+        inc!(NetcheckMetrics::Reports);
 
         let last = self.reports.last.clone();
         let plan = ProbePlan::new(dm, &if_state, last.as_deref());
@@ -1457,14 +1460,10 @@ impl Actor {
         }
 
         trace!(%src, "received STUN packet");
-
-        // if src.is_ipv4() {
-        // TODO:
-        // metricSTUNRecv4.Add(1)
-        // } else if src.is_ipv6() {
-        // TODO:
-        // metricSTUNRecv6.Add(1)
-        // }
+        match &src {
+            SocketAddr::V4(_) => inc!(NetcheckMetrics::StunPacketsRecvIpv4),
+            SocketAddr::V6(_) => inc!(NetcheckMetrics::StunPacketsRecvIpv6),
+        }
 
         if self.handle_hair_stun(pkt, src) {
             return;
@@ -1727,19 +1726,6 @@ async fn os_has_ipv6() -> bool {
     udp.is_ok()
 }
 
-// TODO: Metrics
-// var (
-// 	metricNumGetReport      = clientmetric.NewCounter("netcheck_report")
-// 	metricNumGetReportFull  = clientmetric.NewCounter("netcheck_report_full")
-// 	metricNumGetReportError = clientmetric.NewCounter("netcheck_report_error")
-
-// 	metricSTUNSend4 = clientmetric.NewCounter("netcheck_stun_send_ipv4")
-// 	metricSTUNSend6 = clientmetric.NewCounter("netcheck_stun_send_ipv6")
-// 	metricSTUNRecv4 = clientmetric.NewCounter("netcheck_stun_recv_ipv4")
-// 	metricSTUNRecv6 = clientmetric.NewCounter("netcheck_stun_recv_ipv6")
-// 	metricHTTPSend  = clientmetric.NewCounter("netcheck_https_measure")
-// )
-
 /// Resolves to pending if the inner is `None`.
 #[derive(Debug)]
 struct MaybeFuture<T> {

src/metrics.rs

@@ -42,6 +42,8 @@ mod service;
 
 pub mod iroh;
 pub mod magicsock;
+pub mod netcheck;
+
 // Expose the macros in this crate.
 #[allow(unused_imports)]
 pub(crate) use macros::{inc, make_metric_recorders, observe, record};

src/metrics/core.rs

@@ -3,7 +3,7 @@ use std::sync::atomic::{AtomicBool, Ordering};
 use once_cell::sync::Lazy;
 use prometheus_client::{encoding::text::encode, registry::Registry};
 
-use crate::metrics::{iroh, magicsock};
+use crate::metrics::{iroh, magicsock, netcheck};
 
 pub(crate) static CORE: Lazy<Core> = Lazy::new(Core::default);
 
@@ -12,6 +12,7 @@ pub(crate) struct Core {
     registry: Registry,
     iroh_metrics: iroh::Metrics,
     magicsock_metrics: magicsock::Metrics,
+    netcheck_metrics: netcheck::Metrics,
 }
 
 impl Default for Core {
@@ -21,6 +22,7 @@ impl Default for Core {
             enabled: AtomicBool::new(false),
             iroh_metrics: iroh::Metrics::new(&mut reg),
             magicsock_metrics: magicsock::Metrics::new(&mut reg),
+            netcheck_metrics: netcheck::Metrics::new(&mut reg),
             registry: reg,
         }
     }
@@ -39,6 +41,10 @@ impl Core {
         &self.magicsock_metrics
     }
 
+    pub(crate) fn netcheck_metrics(&self) -> &netcheck::Metrics {
+        &self.netcheck_metrics
+    }
+
     pub(crate) fn encode(&self) -> Result<Vec<u8>, std::io::Error> {
         let mut buf = vec![];
         encode(&mut buf, self.registry())?;
@@ -114,6 +120,7 @@ where
         match c {
             Collector::Iroh => CORE.iroh_metrics().record(m, v),
             Collector::Magicsock => CORE.magicsock_metrics().record(m, v),
+            Collector::Netcheck => CORE.netcheck_metrics().record(m, v),
         };
     }
 }
@@ -131,6 +138,7 @@ where
         match c {
             Collector::Iroh => CORE.iroh_metrics().observe(m, v),
             Collector::Magicsock => CORE.magicsock_metrics().observe(m, v),
+            Collector::Netcheck => CORE.netcheck_metrics().observe(m, v),
         };
     }
 }
@@ -143,4 +151,6 @@ pub enum Collector {
     Iroh,
     /// Magicsock related metrics.
     Magicsock,
+    /// Netcheck related metrics.
+    Netcheck,
 }

src/metrics/macros.rs

@@ -3,7 +3,7 @@
 /// Recording is for single-value metrics, each recorded metric represents a metric value.
 #[macro_export]
 macro_rules! record {
-    ( $e:expr, $v:expr) => {
+    ( $e:expr, $v:expr) => {{
         #[cfg(feature = "metrics")]
         {
             use $crate::metrics::core::MRecorder;
@@ -14,7 +14,7 @@ macro_rules! record {
         {
             $e;
         }
-    };
+    }};
 }
 pub(crate) use record;
 
@@ -24,18 +24,20 @@ pub(crate) use record;
 /// counters.
 #[macro_export]
 macro_rules! inc {
-    ( $e:expr) => {
-        #[cfg(feature = "metrics")]
-        {
-            use $crate::metrics::core::MRecorder;
-            $e.record(1);
-        }
-        #[cfg(not(feature = "metrics"))]
-        #[allow(path_statements)]
+    ( $e:expr) => {{
         {
-            $e;
+            #[cfg(feature = "metrics")]
+            {
+                use $crate::metrics::core::MRecorder;
+                $e.record(1);
+            }
+            #[cfg(not(feature = "metrics"))]
+            #[allow(path_statements)]
+            {
+                $e;
+            }
         }
-    };
+    }};
 }
 pub(crate) use inc;
 
@@ -45,7 +47,7 @@ pub(crate) use inc;
 /// single metric value.
 #[macro_export]
 macro_rules! observe {
-    ( $e:expr, $v:expr) => {
+    ( $e:expr, $v:expr) => {{
         #[cfg(feature = "metrics")]
         {
             use $crate::metrics::core::MObserver;
@@ -56,14 +58,14 @@ macro_rules! observe {
         {
             $e;
         }
-    };
+    }};
 }
 pub(crate) use observe;
 
 /// Generate recorder metrics for a module.
 #[macro_export]
 macro_rules! make_metric_recorders {
-    ($module_name:ident, $($name:ident: $type:ident: $description:expr),+) => {
+    ($module_name:ident, $($name:ident: $type:ident: $description:expr $(,)?)+) => {
         paste::paste! {
             #[cfg(feature = "metrics")]
             #[allow(unused_imports)]

src/metrics/netcheck.rs

@@ -0,0 +1,12 @@
+super::make_metric_recorders! {
+    Netcheck,
+
+    StunPacketsDropped: Counter: "Incoming STUN packets dropped due to a full receiving queue.",
+    StunPacketsSentIpv4: Counter: "Number of IPv4 STUN packets sent",
+    StunPacketsSentIpv6: Counter: "Number of IPv6 STUN packets sent",
+    StunPacketsRecvIpv4: Counter: "Number of IPv4 STUN packets received",
+    StunPacketsRecvIpv6: Counter: "Number of IPv6 STUN packets received",
+    Reports: Counter: "Number of reports executed by netcheck, including full reports",
+    ReportsFull: Counter: "Number of full reports executed by netcheck",
+    ReportsError: Counter: "Number of executed reports resulting in an error",
+}