Skip to content

Commit

Permalink
metrics(netcheck): Add the basic netcheck metrics (#1048)
Browse files Browse the repository at this point in the history 
This adds the basic metrics to netcheck.  There's probably room for
more.

Also updates the macros to be a bit more ergonomic:

- Allow optional trailing comma on the last item in the metrics
  definition.

- When using metrics start a new scope for the compiler so that we can
  use it anywhere, e.g. also in match statements where the expression
  is followed with a , rather than a ;.

Closes #927
  • Loading branch information
@flub
flub authored May 25, 2023
1 parent c34cdf7 commit a548371
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 50 deletions.
54 changes: 20 additions & 34 deletions src/hp/netcheck.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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"));
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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> {
Expand Down
2 changes: 2 additions & 0 deletions src/metrics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
12 changes: 11 additions & 1 deletion src/metrics/core.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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 {
Expand All @@ -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,
}
}
Expand All @@ -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())?;
Expand Down Expand Up @@ -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),
};
}
}
Expand All @@ -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),
};
}
}
Expand All @@ -143,4 +151,6 @@ pub enum Collector {
Iroh,
/// Magicsock related metrics.
Magicsock,
/// Netcheck related metrics.
Netcheck,
}
32 changes: 17 additions & 15 deletions src/metrics/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -14,7 +14,7 @@ macro_rules! record {
{
$e;
}
};
}};
}
pub(crate) use record;

Expand All @@ -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;

Expand All @@ -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;
Expand All @@ -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)]
Expand Down
12 changes: 12 additions & 0 deletions src/metrics/netcheck.rs
View file
Original file line number Diff line number Diff line change
@@ -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",
}

0 comments on commit a548371

Please sign in to comment.