feat!: metrics refactor (#3262)

## Description

Depends on n0-computer/net-tools#20

Until now, we were using a superglobal static `iroh_metrics::core::Core`
struct to collect metrics into. This allowed us to use macros to track
metrics from anywhere in the codebase. However, this also made it
impossible to collect metrics *per endpoint*, which is what you want
usually as soon as you have more than one endpoint in your app.

This PR builds on n0-computer/iroh-metrics#15,
n0-computer/iroh-metrics#22, and n0-computer/iroh-metrics#23. It removes
the global metrics collection from all crates in the iroh repository.
Instead, we now create and pass metrics collector structs to all places
where we need to collect metrics.

This PR disables the `static_core` feature from iroh-metrics, which
means the macros for superglobal metrics collection are not available
anymore. This is good, because otherwise we could easily miss metrics
not tracked onto the proper metrics collector.

This PR also updates iroh-dns-server and iroh-relay to use manual
metrics collection.

While this means that we have to pass our metrics structs to more
places, it also makes metrics collection more visible, and we can now
also split the metrics structs further easily if we want to separate
concerns more.

This PR should not change anything apart from metrics collection. Most
places are straightforward conversions from the macros to methods on the
metrics collectors. At a few places, logic was changed slightly to move
metrics collection a layer up to save a few clones.

## Breaking Changes

* All metrics structs (`iroh::metrics::{MagicsockMetrics,
PortmapMetrics, NetReportMetrics}`) now implement `MetricsGroup` from
the new version `0.34` of `iroh-metrics` and no longer implement traits
from `iroh-metrics@0.32`.
* Metrics are no longer registered onto the static superglobal `Core`.
`iroh` does not use `static_core` feature of `iroh-metrics`. Metrics are
now exposed from the subsystems that track them, see e.g.
`Endpoint::metrics`.

Several methods now take a `Metrics` argument. You can always pass
`Default::default` if you don't want to unify metrics tracking with
other sections.

#### `iroh`

* `iroh::metrics::{MagicsockMetrics, NetReportMetrics, PortmapMetrics}`
all are now marked `non_exhaustive`, and implement
`iroh_metrics::MetricsGroup` from `iroh-metrics@0.34` and no longer
implement `iroh_metrics::Metric` from `iroh-metrics@0.32`. They also no
longer implement `Clone` (put them into an `Arc` for cloning instead).

* `iroh::net_report::Client::new` now takes
`iroh::net_report::metrics::Metrics` as forth argument

#### `iroh-dns-server`
* `iroh_dns_server::server::Server::spawn` now takes `Metrics` as third
argument
* `iroh_dns_server::ZoneStore::persistent` now takes `Metrics` as third
argument
* `iroh_dns_server::ZoneStore::in_memory` now takes `Metrics` as third
argument
* `iroh_dns_server::ZoneStore::new` now takes `Metrics` as third
argument
* `iroh_dns_server::state::AppState` now has a public `metrics: Metrics`
field
* `iroh_dns_server::dns::DnsHandler::new` now takes `Metrics` as third
argument
* function `iroh_dns_server::metrics::init_metrics` is removed

#### `iroh-relay`

* `iroh_relay::metrics::{StunMetrics, Metrics}` all are now marked
`non_exhaustive`, and implement `iroh_metrics::MetricsGroup` from
`iroh-metrics@0.34` and no longer implement `iroh_metrics::Metric` from
`iroh-metrics@0.32`. They also no longer implement `Clone` (put them
into an `Arc` for cloning instead).

## Notes & open questions

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

## Change checklist
<!-- Remove any that are not relevant. -->
- [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.
- [x] List all breaking changes in the above "Breaking Changes" section.
- [x] 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.
    - [x] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip)
      - n0-computer/iroh-gossip#58
    - [x] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs)
      - n0-computer/iroh-blobs#85
    - [x] [`iroh-docs`](https://github.com/n0-computer/iroh-docs)
      - n0-computer/iroh-docs#41

---------

Co-authored-by: dignifiedquire <me@dignifiedquire.com>

Author: Frando

Cargo.lock

@@ -28,7 +28,7 @@ governor = "0.8"
 hickory-server = { version = "0.25.1", features = ["https-ring"] }
 http = "1.0.0"
 humantime-serde = "1.1.1"
-iroh-metrics = { version = "0.32.0", features = ["metrics", "service"] }
+iroh-metrics = { version = "0.34", features = ["service"] }
 lru = "0.12.3"
 n0-future = "0.1.2"
 pkarr = { version = "3.7", features = ["relays", "dht"], default-features = false }

iroh-dns-server/Cargo.toml

@@ -1,15 +1,22 @@
+use std::sync::Arc;
+
 use anyhow::Result;
 use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput};
 use iroh::{discovery::pkarr::PkarrRelayClient, node_info::NodeInfo, SecretKey};
-use iroh_dns_server::{config::Config, server::Server, ZoneStore};
+use iroh_dns_server::{config::Config, metrics::Metrics, server::Server, ZoneStore};
 use rand_chacha::rand_core::SeedableRng;
 use tokio::runtime::Runtime;
 
 const LOCALHOST_PKARR: &str = "http://localhost:8080/pkarr";
 
 async fn start_dns_server(config: Config) -> Result<Server> {
-    let store = ZoneStore::persistent(Config::signed_packet_store_path()?, Default::default())?;
-    Server::spawn(config, store).await
+    let metrics = Arc::new(Metrics::default());
+    let store = ZoneStore::persistent(
+        Config::signed_packet_store_path()?,
+        Default::default(),
+        metrics.clone(),
+    )?;
+    Server::spawn(config, store, metrics).await
 }
 
 fn benchmark_dns_server(c: &mut Criterion) {

iroh-dns-server/benches/write.rs

@@ -26,7 +26,6 @@ use hickory_server::{
     server::{Request, RequestHandler, ResponseHandler, ResponseInfo},
     store::in_memory::InMemoryAuthority,
 };
-use iroh_metrics::inc;
 use serde::{Deserialize, Serialize};
 use tokio::{
     net::{TcpListener, UdpSocket},
@@ -122,12 +121,13 @@ impl DnsServer {
 pub struct DnsHandler {
     #[debug("Catalog")]
     catalog: Arc<Catalog>,
+    metrics: Arc<Metrics>,
 }
 
 impl DnsHandler {
     /// Create a DNS server given some settings, a connection to the DB for DID-by-username lookups
     /// and the server DID to serve under `_did.<origin>`.
-    pub fn new(zone_store: ZoneStore, config: &DnsConfig) -> Result<Self> {
+    pub fn new(zone_store: ZoneStore, config: &DnsConfig, metrics: Arc<Metrics>) -> Result<Self> {
         let origins = config
             .origins
             .iter()
@@ -149,6 +149,7 @@ impl DnsHandler {
 
         Ok(Self {
             catalog: Arc::new(catalog),
+            metrics,
         })
     }
 
@@ -168,23 +169,27 @@ impl RequestHandler for DnsHandler {
         request: &Request,
         response_handle: R,
     ) -> ResponseInfo {
-        inc!(Metrics, dns_requests);
+        self.metrics.dns_requests.inc();
         match request.protocol() {
-            Protocol::Udp => inc!(Metrics, dns_requests_udp),
-            Protocol::Https => inc!(Metrics, dns_requests_https),
+            Protocol::Udp => {
+                self.metrics.dns_requests_udp.inc();
+            }
+            Protocol::Https => {
+                self.metrics.dns_requests_https.inc();
+            }
             _ => {}
         }
         debug!(protocol=%request.protocol(), queries=?request.queries(), "incoming DNS request");
 
         let res = self.catalog.handle_request(request, response_handle).await;
         match &res.response_code() {
             ResponseCode::NoError => match res.answer_count() {
-                0 => inc!(Metrics, dns_lookup_notfound),
-                _ => inc!(Metrics, dns_lookup_success),
+                0 => self.metrics.dns_lookup_notfound.inc(),
+                _ => self.metrics.dns_lookup_success.inc(),
             },
-            ResponseCode::NXDomain => inc!(Metrics, dns_lookup_notfound),
-            _ => inc!(Metrics, dns_lookup_error),
-        }
+            ResponseCode::NXDomain => self.metrics.dns_lookup_notfound.inc(),
+            _ => self.metrics.dns_lookup_error.inc(),
+        };
         res
     }
 }

iroh-dns-server/src/dns.rs

@@ -7,15 +7,14 @@ use std::{
 
 use anyhow::{bail, Context, Result};
 use axum::{
-    extract::{ConnectInfo, Request},
+    extract::{ConnectInfo, Request, State},
     handler::Handler,
     http::Method,
     middleware::{self, Next},
     response::IntoResponse,
     routing::get,
     Router,
 };
-use iroh_metrics::{inc, inc_by};
 use serde::{Deserialize, Serialize};
 use tokio::{net::TcpListener, task::JoinSet};
 use tower_http::{
@@ -31,7 +30,7 @@ mod rate_limiting;
 mod tls;
 
 pub use self::{rate_limiting::RateLimitConfig, tls::CertMode};
-use crate::{config::Config, metrics::Metrics, state::AppState};
+use crate::{config::Config, state::AppState};
 
 /// Config for the HTTP server
 #[derive(Debug, Serialize, Deserialize, Clone)]
@@ -227,13 +226,13 @@ pub(crate) fn create_app(state: AppState, rate_limit_config: &RateLimitConfig) -
         )
         .route("/healthcheck", get(|| async { "OK" }))
         .route("/", get(|| async { "Hi!" }))
-        .with_state(state);
+        .with_state(state.clone());
 
     // configure app
     router
         .layer(cors)
         .layer(trace)
-        .route_layer(middleware::from_fn(metrics_middleware))
+        .route_layer(middleware::from_fn_with_state(state, metrics_middleware))
 }
 
 /// Record request metrics.
@@ -244,17 +243,24 @@ pub(crate) fn create_app(state: AppState, rate_limit_config: &RateLimitConfig) -
 //
 // See also
 // https://github.com/tokio-rs/axum/blob/main/examples/prometheus-metrics/src/main.rs#L114
-async fn metrics_middleware(req: Request, next: Next) -> impl IntoResponse {
+async fn metrics_middleware(
+    State(state): State<AppState>,
+    req: Request,
+    next: Next,
+) -> impl IntoResponse {
     let start = Instant::now();
     let response = next.run(req).await;
     let latency = start.elapsed().as_millis();
     let status = response.status();
-    inc_by!(Metrics, http_requests_duration_ms, latency as u64);
-    inc!(Metrics, http_requests);
+    state
+        .metrics
+        .http_requests_duration_ms
+        .inc_by(latency as u64);
+    state.metrics.http_requests.inc();
     if status.is_success() {
-        inc!(Metrics, http_requests_success);
+        state.metrics.http_requests_success.inc();
     } else {
-        inc!(Metrics, http_requests_error);
+        state.metrics.http_requests_error.inc();
     }
     response
 }

iroh-dns-server/src/http.rs

@@ -194,7 +194,7 @@ mod tests {
             max_batch_time: Duration::from_millis(100),
             ..Default::default()
         };
-        let store = ZoneStore::in_memory(options)?;
+        let store = ZoneStore::in_memory(options, Default::default())?;
 
         // create a signed packet
         let signed_packet = random_signed_packet()?;

iroh-dns-server/src/lib.rs

@@ -2,9 +2,7 @@ use std::path::PathBuf;
 
 use anyhow::Result;
 use clap::Parser;
-use iroh_dns_server::{
-    config::Config, metrics::init_metrics, server::run_with_config_until_ctrl_c,
-};
+use iroh_dns_server::{config::Config, server::run_with_config_until_ctrl_c};
 use tracing::debug;
 
 #[derive(Parser, Debug)]
@@ -27,6 +25,5 @@ async fn main() -> Result<()> {
         Config::default()
     };
 
-    init_metrics();
     run_with_config_until_ctrl_c(config).await
 }

iroh-dns-server/src/main.rs

@@ -1,64 +1,41 @@
 //! Metrics support for the server
 
-use iroh_metrics::core::{Core, Counter, Metric};
-use struct_iterable::Iterable;
+use iroh_metrics::{Counter, MetricsGroup};
 
 /// Metrics for iroh-dns-server
-#[derive(Debug, Clone, Iterable)]
-#[allow(missing_docs)]
+#[derive(Debug, Default, MetricsGroup)]
+#[metrics(name = "dns_server")]
 pub struct Metrics {
+    /// Number of pkarr relay puts that updated the state
     pub pkarr_publish_update: Counter,
+    /// Number of pkarr relay puts that did not update the state
     pub pkarr_publish_noop: Counter,
+    /// DNS requests (total)
     pub dns_requests: Counter,
+    /// DNS requests via UDP
     pub dns_requests_udp: Counter,
+    /// DNS requests via HTTPS (DoH)
     pub dns_requests_https: Counter,
+    /// DNS lookup responses with at least one answer
     pub dns_lookup_success: Counter,
+    /// DNS lookup responses with no answers
     pub dns_lookup_notfound: Counter,
+    /// DNS lookup responses which failed
     pub dns_lookup_error: Counter,
+    /// Number of HTTP requests
     pub http_requests: Counter,
+    /// Number of HTTP requests with a 2xx status code
     pub http_requests_success: Counter,
+    /// Number of HTTP requests with a non-2xx status code
     pub http_requests_error: Counter,
+    /// Total duration of all HTTP requests
     pub http_requests_duration_ms: Counter,
+    /// Signed packets inserted into the store
     pub store_packets_inserted: Counter,
+    /// Signed packets removed from the store
     pub store_packets_removed: Counter,
+    /// Number of updates to existing packets
     pub store_packets_updated: Counter,
+    /// Number of expired packets
     pub store_packets_expired: Counter,
 }
-
-impl Default for Metrics {
-    fn default() -> Self {
-        Self {
-            pkarr_publish_update: Counter::new("Number of pkarr relay puts that updated the state"),
-            pkarr_publish_noop: Counter::new(
-                "Number of pkarr relay puts that did not update the state",
-            ),
-            dns_requests: Counter::new("DNS requests (total)"),
-            dns_requests_udp: Counter::new("DNS requests via UDP"),
-            dns_requests_https: Counter::new("DNS requests via HTTPS (DoH)"),
-            dns_lookup_success: Counter::new("DNS lookup responses with at least one answer"),
-            dns_lookup_notfound: Counter::new("DNS lookup responses with no answers"),
-            dns_lookup_error: Counter::new("DNS lookup responses which failed"),
-            http_requests: Counter::new("Number of HTTP requests"),
-            http_requests_success: Counter::new("Number of HTTP requests with a 2xx status code"),
-            http_requests_error: Counter::new("Number of HTTP requests with a non-2xx status code"),
-            http_requests_duration_ms: Counter::new("Total duration of all HTTP requests"),
-            store_packets_inserted: Counter::new("Signed packets inserted into the store"),
-            store_packets_removed: Counter::new("Signed packets removed from the store"),
-            store_packets_updated: Counter::new("Number of updates to existing packets"),
-            store_packets_expired: Counter::new("Number of expired packets"),
-        }
-    }
-}
-
-impl Metric for Metrics {
-    fn name() -> &'static str {
-        "dns_server"
-    }
-}
-
-/// Init the metrics collection core.
-pub fn init_metrics() {
-    Core::init(|reg, metrics| {
-        metrics.insert(Metrics::new(reg));
-    });
-}

iroh-dns-server/src/metrics.rs

@@ -1,29 +1,34 @@
 //! The main server which combines the DNS and HTTP(S) servers.
 
+use std::sync::Arc;
+
 use anyhow::Result;
-use iroh_metrics::metrics::start_metrics_server;
+use iroh_metrics::service::start_metrics_server;
 use tracing::info;
 
 use crate::{
     config::Config,
     dns::{DnsHandler, DnsServer},
     http::HttpServer,
+    metrics::Metrics,
     state::AppState,
     store::ZoneStore,
 };
 
 /// Spawn the server and run until the `Ctrl-C` signal is received, then shutdown.
 pub async fn run_with_config_until_ctrl_c(config: Config) -> Result<()> {
+    let metrics = Arc::new(Metrics::default());
     let zone_store_options = config.zone_store.clone().unwrap_or_default();
     let mut store = ZoneStore::persistent(
         Config::signed_packet_store_path()?,
         zone_store_options.into(),
+        metrics.clone(),
     )?;
     if let Some(bootstrap) = config.mainline_enabled() {
         info!("mainline fallback enabled");
         store = store.with_mainline_fallback(bootstrap);
     };
-    let server = Server::spawn(config, store).await?;
+    let server = Server::spawn(config, store, metrics).await?;
     tokio::signal::ctrl_c().await?;
     info!("shutdown");
     server.shutdown().await?;
@@ -44,15 +49,21 @@ impl Server {
     /// * A DNS server task
     /// * A HTTP server task, if `config.http` is not empty
     /// * A HTTPS server task, if `config.https` is not empty
-    pub async fn spawn(config: Config, store: ZoneStore) -> Result<Self> {
-        let dns_handler = DnsHandler::new(store.clone(), &config.dns)?;
+    pub async fn spawn(config: Config, store: ZoneStore, metrics: Arc<Metrics>) -> Result<Self> {
+        let dns_handler = DnsHandler::new(store.clone(), &config.dns, metrics.clone())?;
 
-        let state = AppState { store, dns_handler };
+        let state = AppState {
+            store,
+            dns_handler,
+            metrics: metrics.clone(),
+        };
 
         let metrics_addr = config.metrics_addr();
         let metrics_task = tokio::task::spawn(async move {
             if let Some(addr) = metrics_addr {
-                start_metrics_server(addr).await?;
+                let mut registry = iroh_metrics::Registry::default();
+                registry.register(metrics);
+                start_metrics_server(addr, Arc::new(registry)).await?;
             }
             Ok(())
         });
@@ -122,12 +133,12 @@ impl Server {
         config.https = None;
         config.metrics = Some(MetricsConfig::disabled());
 
-        let mut store = ZoneStore::in_memory(options.unwrap_or_default())?;
+        let mut store = ZoneStore::in_memory(options.unwrap_or_default(), Default::default())?;
         if let Some(bootstrap) = mainline {
             info!("mainline fallback enabled");
             store = store.with_mainline_fallback(bootstrap);
         }
-        let server = Self::spawn(config, store).await?;
+        let server = Self::spawn(config, store, Default::default()).await?;
         let dns_addr = server.dns_server.local_addr();
         let http_addr = server.http_server.http_addr().expect("http is set");
         let http_url = format!("http://{http_addr}").parse()?;

iroh-dns-server/src/server.rs

@@ -1,6 +1,8 @@
 //! Shared state and store for the iroh-dns-server
 
-use crate::{dns::DnsHandler, store::ZoneStore};
+use std::sync::Arc;
+
+use crate::{dns::DnsHandler, metrics::Metrics, store::ZoneStore};
 
 /// The shared app state.
 #[derive(Clone)]
@@ -9,4 +11,6 @@ pub struct AppState {
     pub store: ZoneStore,
     /// Handler for DNS requests
     pub dns_handler: DnsHandler,
+    /// Metrics collector.
+    pub metrics: Arc<Metrics>,
 }