From bd59d8ea0d083b94fcda0b2ec31ec85714fb76dc Mon Sep 17 00:00:00 2001 From: Nikolay Komarevskiy Date: Fri, 17 Jan 2025 15:33:36 +0100 Subject: [PATCH 1/6] chore: add routes_stats() to RouteProvider --- ic-agent/src/agent/route_provider.rs | 22 +++++++++++++++++++ .../dynamic_routing/dynamic_route_provider.rs | 11 ++++++++++ .../snapshot/latency_based_routing.rs | 17 ++++++++++++++ .../snapshot/round_robin_routing.rs | 4 ++++ .../snapshot/routing_snapshot.rs | 2 ++ 5 files changed, 56 insertions(+) diff --git a/ic-agent/src/agent/route_provider.rs b/ic-agent/src/agent/route_provider.rs index 8720c05a..7d7d9f06 100644 --- a/ic-agent/src/agent/route_provider.rs +++ b/ic-agent/src/agent/route_provider.rs @@ -51,6 +51,15 @@ pub trait RouteProvider: std::fmt::Debug + Send + Sync { /// appearing first. The returned vector can contain fewer than `n` URLs if /// fewer are available. fn n_ordered_routes(&self, n: usize) -> Result, AgentError>; + + /// Returns the total number of routes and healthy routes as a tuple. + /// + /// - First element is the total number of routes available (both healthy and unhealthy) + /// - Second element is the number of currently healthy routes + /// + /// A healthy route is one that is available and ready to receive traffic. + /// The specific criteria for what constitutes a "healthy" route is implementation dependent. + fn routes_stats(&self) -> (usize, usize); } /// A simple implementation of the [`RouteProvider`] which produces an even distribution of the urls from the input ones. @@ -94,6 +103,10 @@ impl RouteProvider for RoundRobinRouteProvider { Ok(urls) } + + fn routes_stats(&self) -> (usize, usize) { + (self.routes.len(), self.routes.len()) + } } impl RoundRobinRouteProvider { @@ -133,6 +146,9 @@ impl RouteProvider for Url { fn n_ordered_routes(&self, _: usize) -> Result, AgentError> { Ok(vec![self.route()?]) } + fn routes_stats(&self) -> (usize, usize) { + (1, 1) + } } /// A [`RouteProvider`] that will attempt to discover new boundary nodes and cycle through them, optionally prioritizing those with low latency. @@ -215,6 +231,9 @@ impl RouteProvider for DynamicRouteProvider { fn n_ordered_routes(&self, n: usize) -> Result, AgentError> { self.inner.n_ordered_routes(n) } + fn routes_stats(&self) -> (usize, usize) { + self.inner.routes_stats() + } } /// Strategy for [`DynamicRouteProvider`]'s routing mechanism. @@ -270,6 +289,9 @@ impl RouteProvider for UrlUntilReady { self.url.route() } } + fn routes_stats(&self) -> (usize, usize) { + (1, 1) + } } #[cfg(test)] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs b/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs index ad9b7b40..4ec96476 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs @@ -186,6 +186,11 @@ where let urls = nodes.iter().map(|n| n.to_routing_url()).collect(); Ok(urls) } + + fn routes_stats(&self) -> (usize, usize) { + let snapshot = self.routing_snapshot.load(); + snapshot.nodes_stats() + } } impl DynamicRouteProvider @@ -417,6 +422,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(6, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_1.domain()], 6); + assert_eq!(route_provider.routes_stats(), (1, 1)); // Test 2: multiple route() calls return 3 different domains with equal fairness (repetition). // Two healthy nodes are added to the topology. @@ -431,6 +437,7 @@ mod tests { vec![node_1.domain(), node_2.domain(), node_3.domain()], 2, ); + assert_eq!(route_provider.routes_stats(), (3, 3)); // Test 3: multiple route() calls return 2 different domains with equal fairness (repetition). // One node is set to unhealthy. @@ -438,6 +445,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(6, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_1.domain(), node_3.domain()], 3); + assert_eq!(route_provider.routes_stats(), (3, 2)); // Test 4: multiple route() calls return 3 different domains with equal fairness (repetition). // Unhealthy node is set back to healthy. @@ -449,6 +457,7 @@ mod tests { vec![node_1.domain(), node_2.domain(), node_3.domain()], 2, ); + assert_eq!(route_provider.routes_stats(), (3, 3)); // Test 5: multiple route() calls return 3 different domains with equal fairness (repetition). // One healthy node is added, but another one goes unhealthy. @@ -467,6 +476,7 @@ mod tests { vec![node_2.domain(), node_3.domain(), node_4.domain()], 2, ); + assert_eq!(route_provider.routes_stats(), (4, 3)); // Test 6: multiple route() calls return a single domain=api1.com. // One node is set to unhealthy and one is removed from the topology. @@ -475,6 +485,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(3, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_2.domain()], 3); + assert_eq!(route_provider.routes_stats(), (3, 1)); } #[tokio::test] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs index 028bbb2c..4127d796 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs @@ -157,6 +157,7 @@ fn compute_score( pub struct LatencyRoutingSnapshot { nodes_with_metrics: Vec, existing_nodes: HashSet, + healthy_nodes: HashSet, window_weights: Vec, window_weights_sum: f64, use_availability_penalty: bool, @@ -174,6 +175,7 @@ impl LatencyRoutingSnapshot { Self { nodes_with_metrics: vec![], existing_nodes: HashSet::new(), + healthy_nodes: HashSet::new(), use_availability_penalty: true, window_weights, window_weights_sum, @@ -302,6 +304,12 @@ impl RoutingSnapshot for LatencyRoutingSnapshot { self.nodes_with_metrics.len() - 1 }); + if health.is_healthy() { + self.healthy_nodes.insert(node.clone()); + } else { + self.healthy_nodes.remove(node); + } + self.nodes_with_metrics[idx].add_latency_measurement(health.latency()); self.nodes_with_metrics[idx].score = compute_score( @@ -314,6 +322,10 @@ impl RoutingSnapshot for LatencyRoutingSnapshot { true } + + fn nodes_stats(&self) -> (usize, usize) { + (self.existing_nodes.len(), self.healthy_nodes.len()) + } } #[cfg(test)] @@ -344,6 +356,7 @@ mod tests { assert!(!snapshot.has_nodes()); assert!(snapshot.next_node().is_none()); assert!(snapshot.next_n_nodes(1).is_empty()); + assert_eq!(snapshot.nodes_stats(), (0, 0)); } #[test] @@ -359,6 +372,7 @@ mod tests { assert!(snapshot.nodes_with_metrics.is_empty()); assert!(!snapshot.has_nodes()); assert!(snapshot.next_node().is_none()); + assert_eq!(snapshot.nodes_stats(), (0, 0)); } #[test] @@ -370,6 +384,7 @@ mod tests { let node = Node::new("api1.com").unwrap(); let health = HealthCheckStatus::new(Some(Duration::from_secs(1))); snapshot.existing_nodes.insert(node.clone()); + assert_eq!(snapshot.nodes_stats(), (1, 0)); // Check first update let is_updated = snapshot.update_node(&node, health); assert!(is_updated); @@ -377,6 +392,7 @@ mod tests { let node_with_metrics = snapshot.nodes_with_metrics.first().unwrap(); assert_eq!(node_with_metrics.score, (2.0 / 1.0) / 2.0); assert_eq!(snapshot.next_node().unwrap(), node); + assert_eq!(snapshot.nodes_stats(), (1, 1)); // Check second update let health = HealthCheckStatus::new(Some(Duration::from_secs(2))); let is_updated = snapshot.update_node(&node, health); @@ -399,6 +415,7 @@ mod tests { assert_eq!(snapshot.nodes_with_metrics.len(), 1); assert_eq!(snapshot.existing_nodes.len(), 1); assert!(snapshot.next_node().is_none()); + assert_eq!(snapshot.nodes_stats(), (1, 0)); } #[test] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs index 67ce51e0..93332b7d 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs @@ -106,6 +106,10 @@ impl RoutingSnapshot for RoundRobinRoutingSnapshot { self.healthy_nodes.remove(node) } } + + fn nodes_stats(&self) -> (usize, usize) { + (self.existing_nodes.len(), self.healthy_nodes.len()) + } } #[cfg(test)] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs index ef1e10af..f5e7ef54 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs @@ -15,4 +15,6 @@ pub trait RoutingSnapshot: Send + Sync + Clone + Debug { fn sync_nodes(&mut self, nodes: &[Node]) -> bool; /// Updates the health status of a specific node, returning `true` if the node was found and updated. fn update_node(&mut self, node: &Node, health: HealthCheckStatus) -> bool; + /// Returns the total number of nodes and healthy nodes as a tuple with first and second element, respectively. + fn nodes_stats(&self) -> (usize, usize); } From a6336223671172d3ba06ac4b04ae9189b25be087 Mon Sep 17 00:00:00 2001 From: Nikolay Komarevskiy Date: Fri, 17 Jan 2025 15:57:02 +0100 Subject: [PATCH 2/6] docs: add changes to changelog.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcda350e..8b2c6237 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Added `CanisterInfo` to `MgmtMethod`. +* Extended `RouteProvider` trait with `fn routes_stats()`, returning the number of total and healthy routes as a tuple. + ## [0.39.2] - 2024-12-20 * Bumped `ic-certification` to `3.0.0`. From 0f9ff4807f55401abb5b44b25e214974c59044e1 Mon Sep 17 00:00:00 2001 From: Nikolay Komarevskiy Date: Mon, 20 Jan 2025 18:18:42 +0100 Subject: [PATCH 3/6] chore: use Option for healthy routes count --- ic-agent/src/agent/route_provider.rs | 18 +++++++++--------- .../dynamic_routing/dynamic_route_provider.rs | 14 +++++++------- .../snapshot/latency_based_routing.rs | 14 +++++++------- .../snapshot/round_robin_routing.rs | 4 ++-- .../snapshot/routing_snapshot.rs | 9 +++++++-- 5 files changed, 32 insertions(+), 27 deletions(-) diff --git a/ic-agent/src/agent/route_provider.rs b/ic-agent/src/agent/route_provider.rs index 7d7d9f06..77225a64 100644 --- a/ic-agent/src/agent/route_provider.rs +++ b/ic-agent/src/agent/route_provider.rs @@ -55,11 +55,11 @@ pub trait RouteProvider: std::fmt::Debug + Send + Sync { /// Returns the total number of routes and healthy routes as a tuple. /// /// - First element is the total number of routes available (both healthy and unhealthy) - /// - Second element is the number of currently healthy routes + /// - Second element is the number of currently healthy routes, or None if health status information is unavailable /// /// A healthy route is one that is available and ready to receive traffic. /// The specific criteria for what constitutes a "healthy" route is implementation dependent. - fn routes_stats(&self) -> (usize, usize); + fn routes_stats(&self) -> (usize, Option); } /// A simple implementation of the [`RouteProvider`] which produces an even distribution of the urls from the input ones. @@ -104,8 +104,8 @@ impl RouteProvider for RoundRobinRouteProvider { Ok(urls) } - fn routes_stats(&self) -> (usize, usize) { - (self.routes.len(), self.routes.len()) + fn routes_stats(&self) -> (usize, Option) { + (self.routes.len(), None) } } @@ -146,8 +146,8 @@ impl RouteProvider for Url { fn n_ordered_routes(&self, _: usize) -> Result, AgentError> { Ok(vec![self.route()?]) } - fn routes_stats(&self) -> (usize, usize) { - (1, 1) + fn routes_stats(&self) -> (usize, Option) { + (1, None) } } @@ -231,7 +231,7 @@ impl RouteProvider for DynamicRouteProvider { fn n_ordered_routes(&self, n: usize) -> Result, AgentError> { self.inner.n_ordered_routes(n) } - fn routes_stats(&self) -> (usize, usize) { + fn routes_stats(&self) -> (usize, Option) { self.inner.routes_stats() } } @@ -289,8 +289,8 @@ impl RouteProvider for UrlUntilReady { self.url.route() } } - fn routes_stats(&self) -> (usize, usize) { - (1, 1) + fn routes_stats(&self) -> (usize, Option) { + (1, None) } } diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs b/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs index 4ec96476..dca44562 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs @@ -187,7 +187,7 @@ where Ok(urls) } - fn routes_stats(&self) -> (usize, usize) { + fn routes_stats(&self) -> (usize, Option) { let snapshot = self.routing_snapshot.load(); snapshot.nodes_stats() } @@ -422,7 +422,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(6, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_1.domain()], 6); - assert_eq!(route_provider.routes_stats(), (1, 1)); + assert_eq!(route_provider.routes_stats(), (1, Some(1))); // Test 2: multiple route() calls return 3 different domains with equal fairness (repetition). // Two healthy nodes are added to the topology. @@ -437,7 +437,7 @@ mod tests { vec![node_1.domain(), node_2.domain(), node_3.domain()], 2, ); - assert_eq!(route_provider.routes_stats(), (3, 3)); + assert_eq!(route_provider.routes_stats(), (3, Some(3))); // Test 3: multiple route() calls return 2 different domains with equal fairness (repetition). // One node is set to unhealthy. @@ -445,7 +445,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(6, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_1.domain(), node_3.domain()], 3); - assert_eq!(route_provider.routes_stats(), (3, 2)); + assert_eq!(route_provider.routes_stats(), (3, Some(2))); // Test 4: multiple route() calls return 3 different domains with equal fairness (repetition). // Unhealthy node is set back to healthy. @@ -457,7 +457,7 @@ mod tests { vec![node_1.domain(), node_2.domain(), node_3.domain()], 2, ); - assert_eq!(route_provider.routes_stats(), (3, 3)); + assert_eq!(route_provider.routes_stats(), (3, Some(3))); // Test 5: multiple route() calls return 3 different domains with equal fairness (repetition). // One healthy node is added, but another one goes unhealthy. @@ -476,7 +476,7 @@ mod tests { vec![node_2.domain(), node_3.domain(), node_4.domain()], 2, ); - assert_eq!(route_provider.routes_stats(), (4, 3)); + assert_eq!(route_provider.routes_stats(), (4, Some(3))); // Test 6: multiple route() calls return a single domain=api1.com. // One node is set to unhealthy and one is removed from the topology. @@ -485,7 +485,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(3, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_2.domain()], 3); - assert_eq!(route_provider.routes_stats(), (3, 1)); + assert_eq!(route_provider.routes_stats(), (3, Some(1))); } #[tokio::test] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs index 4127d796..87b3b37e 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs @@ -323,8 +323,8 @@ impl RoutingSnapshot for LatencyRoutingSnapshot { true } - fn nodes_stats(&self) -> (usize, usize) { - (self.existing_nodes.len(), self.healthy_nodes.len()) + fn nodes_stats(&self) -> (usize, Option) { + (self.existing_nodes.len(), Some(self.healthy_nodes.len())) } } @@ -356,7 +356,7 @@ mod tests { assert!(!snapshot.has_nodes()); assert!(snapshot.next_node().is_none()); assert!(snapshot.next_n_nodes(1).is_empty()); - assert_eq!(snapshot.nodes_stats(), (0, 0)); + assert_eq!(snapshot.nodes_stats(), (0, Some(0))); } #[test] @@ -372,7 +372,7 @@ mod tests { assert!(snapshot.nodes_with_metrics.is_empty()); assert!(!snapshot.has_nodes()); assert!(snapshot.next_node().is_none()); - assert_eq!(snapshot.nodes_stats(), (0, 0)); + assert_eq!(snapshot.nodes_stats(), (0, Some(0))); } #[test] @@ -384,7 +384,7 @@ mod tests { let node = Node::new("api1.com").unwrap(); let health = HealthCheckStatus::new(Some(Duration::from_secs(1))); snapshot.existing_nodes.insert(node.clone()); - assert_eq!(snapshot.nodes_stats(), (1, 0)); + assert_eq!(snapshot.nodes_stats(), (1, Some(0))); // Check first update let is_updated = snapshot.update_node(&node, health); assert!(is_updated); @@ -392,7 +392,7 @@ mod tests { let node_with_metrics = snapshot.nodes_with_metrics.first().unwrap(); assert_eq!(node_with_metrics.score, (2.0 / 1.0) / 2.0); assert_eq!(snapshot.next_node().unwrap(), node); - assert_eq!(snapshot.nodes_stats(), (1, 1)); + assert_eq!(snapshot.nodes_stats(), (1, Some(1))); // Check second update let health = HealthCheckStatus::new(Some(Duration::from_secs(2))); let is_updated = snapshot.update_node(&node, health); @@ -415,7 +415,7 @@ mod tests { assert_eq!(snapshot.nodes_with_metrics.len(), 1); assert_eq!(snapshot.existing_nodes.len(), 1); assert!(snapshot.next_node().is_none()); - assert_eq!(snapshot.nodes_stats(), (1, 0)); + assert_eq!(snapshot.nodes_stats(), (1, Some(0))); } #[test] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs index 93332b7d..28c06fdf 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs @@ -107,8 +107,8 @@ impl RoutingSnapshot for RoundRobinRoutingSnapshot { } } - fn nodes_stats(&self) -> (usize, usize) { - (self.existing_nodes.len(), self.healthy_nodes.len()) + fn nodes_stats(&self) -> (usize, Option) { + (self.existing_nodes.len(), Some(self.healthy_nodes.len())) } } diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs index f5e7ef54..89191137 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs @@ -15,6 +15,11 @@ pub trait RoutingSnapshot: Send + Sync + Clone + Debug { fn sync_nodes(&mut self, nodes: &[Node]) -> bool; /// Updates the health status of a specific node, returning `true` if the node was found and updated. fn update_node(&mut self, node: &Node, health: HealthCheckStatus) -> bool; - /// Returns the total number of nodes and healthy nodes as a tuple with first and second element, respectively. - fn nodes_stats(&self) -> (usize, usize); + /// Returns the total number of nodes and healthy nodes as a tuple. + /// + /// - First element is the total number of nodes available (both healthy and unhealthy) + /// - Second element is the number of currently healthy nodes, or None if health status information is unavailable + /// + /// The specific criteria for what constitutes a "healthy" node is implementation dependent. + fn nodes_stats(&self) -> (usize, Option); } From 9ed0fe14d4236f82171e8463a29020bb5e2f5aeb Mon Sep 17 00:00:00 2001 From: Nikolay Komarevskiy Date: Wed, 22 Jan 2025 10:24:49 +0100 Subject: [PATCH 4/6] chore: refactor to RoutesStats --- ic-agent/src/agent/route_provider.rs | 42 ++++++++++++------- .../dynamic_routing/dynamic_route_provider.rs | 20 ++++----- .../snapshot/latency_based_routing.rs | 38 ++++++++++------- .../snapshot/round_robin_routing.rs | 11 +++-- .../snapshot/routing_snapshot.rs | 14 +++---- 5 files changed, 72 insertions(+), 53 deletions(-) diff --git a/ic-agent/src/agent/route_provider.rs b/ic-agent/src/agent/route_provider.rs index 77225a64..f4b1869c 100644 --- a/ic-agent/src/agent/route_provider.rs +++ b/ic-agent/src/agent/route_provider.rs @@ -36,6 +36,24 @@ const ICP0_SUB_DOMAIN: &str = ".icp0.io"; const ICP_API_SUB_DOMAIN: &str = ".icp-api.io"; const LOCALHOST_SUB_DOMAIN: &str = ".localhost"; +/// Statistical info about routing urls. +#[derive(Debug, PartialEq)] +pub struct RoutesStats { + /// Total number of existing routes (both healthy and unhealthy). + pub total: usize, + + /// Number of currently healthy routes, or None if health status information is unavailable. + /// A healthy route is one that is available and ready to receive traffic. + /// The specific criteria for what constitutes a "healthy" route is implementation dependent. + pub healthy: Option, +} + +impl RoutesStats { + pub fn new(total: usize, healthy: Option) -> Self { + Self { total, healthy } + } +} + /// A [`RouteProvider`] for dynamic generation of routing urls. pub trait RouteProvider: std::fmt::Debug + Send + Sync { /// Generates the next routing URL based on the internal routing logic. @@ -52,14 +70,8 @@ pub trait RouteProvider: std::fmt::Debug + Send + Sync { /// fewer are available. fn n_ordered_routes(&self, n: usize) -> Result, AgentError>; - /// Returns the total number of routes and healthy routes as a tuple. - /// - /// - First element is the total number of routes available (both healthy and unhealthy) - /// - Second element is the number of currently healthy routes, or None if health status information is unavailable - /// - /// A healthy route is one that is available and ready to receive traffic. - /// The specific criteria for what constitutes a "healthy" route is implementation dependent. - fn routes_stats(&self) -> (usize, Option); + /// Returns statistics about the total number of existing routes and the number of healthy routes. + fn routes_stats(&self) -> RoutesStats; } /// A simple implementation of the [`RouteProvider`] which produces an even distribution of the urls from the input ones. @@ -104,8 +116,8 @@ impl RouteProvider for RoundRobinRouteProvider { Ok(urls) } - fn routes_stats(&self) -> (usize, Option) { - (self.routes.len(), None) + fn routes_stats(&self) -> RoutesStats { + RoutesStats::new(self.routes.len(), None) } } @@ -146,8 +158,8 @@ impl RouteProvider for Url { fn n_ordered_routes(&self, _: usize) -> Result, AgentError> { Ok(vec![self.route()?]) } - fn routes_stats(&self) -> (usize, Option) { - (1, None) + fn routes_stats(&self) -> RoutesStats { + RoutesStats::new(1, None) } } @@ -231,7 +243,7 @@ impl RouteProvider for DynamicRouteProvider { fn n_ordered_routes(&self, n: usize) -> Result, AgentError> { self.inner.n_ordered_routes(n) } - fn routes_stats(&self) -> (usize, Option) { + fn routes_stats(&self) -> RoutesStats { self.inner.routes_stats() } } @@ -289,8 +301,8 @@ impl RouteProvider for UrlUntilReady { self.url.route() } } - fn routes_stats(&self) -> (usize, Option) { - (1, None) + fn routes_stats(&self) -> RoutesStats { + RoutesStats::new(1, None) } } diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs b/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs index dca44562..10bef122 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs @@ -23,7 +23,7 @@ use crate::{ snapshot::routing_snapshot::RoutingSnapshot, type_aliases::AtomicSwap, }, - RouteProvider, + RouteProvider, RoutesStats, }, HttpService, }, @@ -187,9 +187,9 @@ where Ok(urls) } - fn routes_stats(&self) -> (usize, Option) { + fn routes_stats(&self) -> RoutesStats { let snapshot = self.routing_snapshot.load(); - snapshot.nodes_stats() + snapshot.routes_stats() } } @@ -296,7 +296,7 @@ mod tests { assert_routed_domains, route_n_times, NodeHealthCheckerMock, NodesFetcherMock, }, }, - RouteProvider, + RouteProvider, RoutesStats, }, Agent, AgentError, }; @@ -422,7 +422,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(6, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_1.domain()], 6); - assert_eq!(route_provider.routes_stats(), (1, Some(1))); + assert_eq!(route_provider.routes_stats(), RoutesStats::new(1, Some(1))); // Test 2: multiple route() calls return 3 different domains with equal fairness (repetition). // Two healthy nodes are added to the topology. @@ -437,7 +437,7 @@ mod tests { vec![node_1.domain(), node_2.domain(), node_3.domain()], 2, ); - assert_eq!(route_provider.routes_stats(), (3, Some(3))); + assert_eq!(route_provider.routes_stats(), RoutesStats::new(3, Some(3))); // Test 3: multiple route() calls return 2 different domains with equal fairness (repetition). // One node is set to unhealthy. @@ -445,7 +445,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(6, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_1.domain(), node_3.domain()], 3); - assert_eq!(route_provider.routes_stats(), (3, Some(2))); + assert_eq!(route_provider.routes_stats(), RoutesStats::new(3, Some(2))); // Test 4: multiple route() calls return 3 different domains with equal fairness (repetition). // Unhealthy node is set back to healthy. @@ -457,7 +457,7 @@ mod tests { vec![node_1.domain(), node_2.domain(), node_3.domain()], 2, ); - assert_eq!(route_provider.routes_stats(), (3, Some(3))); + assert_eq!(route_provider.routes_stats(), RoutesStats::new(3, Some(3))); // Test 5: multiple route() calls return 3 different domains with equal fairness (repetition). // One healthy node is added, but another one goes unhealthy. @@ -476,7 +476,7 @@ mod tests { vec![node_2.domain(), node_3.domain(), node_4.domain()], 2, ); - assert_eq!(route_provider.routes_stats(), (4, Some(3))); + assert_eq!(route_provider.routes_stats(), RoutesStats::new(4, Some(3))); // Test 6: multiple route() calls return a single domain=api1.com. // One node is set to unhealthy and one is removed from the topology. @@ -485,7 +485,7 @@ mod tests { tokio::time::sleep(snapshot_update_duration).await; let routed_domains = route_n_times(3, Arc::clone(&route_provider)); assert_routed_domains(routed_domains, vec![node_2.domain()], 3); - assert_eq!(route_provider.routes_stats(), (3, Some(1))); + assert_eq!(route_provider.routes_stats(), RoutesStats::new(3, Some(1))); } #[tokio::test] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs index 87b3b37e..96f965ac 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs @@ -5,8 +5,11 @@ use std::{ use rand::Rng; -use crate::agent::route_provider::dynamic_routing::{ - health_check::HealthCheckStatus, node::Node, snapshot::routing_snapshot::RoutingSnapshot, +use crate::agent::route_provider::{ + dynamic_routing::{ + health_check::HealthCheckStatus, node::Node, snapshot::routing_snapshot::RoutingSnapshot, + }, + RoutesStats, }; // Determines the size of the sliding window used for storing latencies and availabilities of nodes. @@ -323,8 +326,8 @@ impl RoutingSnapshot for LatencyRoutingSnapshot { true } - fn nodes_stats(&self) -> (usize, Option) { - (self.existing_nodes.len(), Some(self.healthy_nodes.len())) + fn routes_stats(&self) -> RoutesStats { + RoutesStats::new(self.existing_nodes.len(), Some(self.healthy_nodes.len())) } } @@ -335,15 +338,18 @@ mod tests { time::Duration, }; - use crate::agent::route_provider::dynamic_routing::{ - health_check::HealthCheckStatus, - node::Node, - snapshot::{ - latency_based_routing::{ - compute_score, weighted_sample, LatencyRoutingSnapshot, NodeWithMetrics, + use crate::agent::route_provider::{ + dynamic_routing::{ + health_check::HealthCheckStatus, + node::Node, + snapshot::{ + latency_based_routing::{ + compute_score, weighted_sample, LatencyRoutingSnapshot, NodeWithMetrics, + }, + routing_snapshot::RoutingSnapshot, }, - routing_snapshot::RoutingSnapshot, }, + RoutesStats, }; #[test] @@ -356,7 +362,7 @@ mod tests { assert!(!snapshot.has_nodes()); assert!(snapshot.next_node().is_none()); assert!(snapshot.next_n_nodes(1).is_empty()); - assert_eq!(snapshot.nodes_stats(), (0, Some(0))); + assert_eq!(snapshot.routes_stats(), RoutesStats::new(0, Some(0))); } #[test] @@ -372,7 +378,7 @@ mod tests { assert!(snapshot.nodes_with_metrics.is_empty()); assert!(!snapshot.has_nodes()); assert!(snapshot.next_node().is_none()); - assert_eq!(snapshot.nodes_stats(), (0, Some(0))); + assert_eq!(snapshot.routes_stats(), RoutesStats::new(0, Some(0))); } #[test] @@ -384,7 +390,7 @@ mod tests { let node = Node::new("api1.com").unwrap(); let health = HealthCheckStatus::new(Some(Duration::from_secs(1))); snapshot.existing_nodes.insert(node.clone()); - assert_eq!(snapshot.nodes_stats(), (1, Some(0))); + assert_eq!(snapshot.routes_stats(), RoutesStats::new(1, Some(0))); // Check first update let is_updated = snapshot.update_node(&node, health); assert!(is_updated); @@ -392,7 +398,7 @@ mod tests { let node_with_metrics = snapshot.nodes_with_metrics.first().unwrap(); assert_eq!(node_with_metrics.score, (2.0 / 1.0) / 2.0); assert_eq!(snapshot.next_node().unwrap(), node); - assert_eq!(snapshot.nodes_stats(), (1, Some(1))); + assert_eq!(snapshot.routes_stats(), RoutesStats::new(1, Some(1))); // Check second update let health = HealthCheckStatus::new(Some(Duration::from_secs(2))); let is_updated = snapshot.update_node(&node, health); @@ -415,7 +421,7 @@ mod tests { assert_eq!(snapshot.nodes_with_metrics.len(), 1); assert_eq!(snapshot.existing_nodes.len(), 1); assert!(snapshot.next_node().is_none()); - assert_eq!(snapshot.nodes_stats(), (1, Some(0))); + assert_eq!(snapshot.routes_stats(), RoutesStats::new(1, Some(0))); } #[test] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs index 28c06fdf..e9418adf 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs @@ -6,8 +6,11 @@ use std::{ }, }; -use crate::agent::route_provider::dynamic_routing::{ - health_check::HealthCheckStatus, node::Node, snapshot::routing_snapshot::RoutingSnapshot, +use crate::agent::route_provider::{ + dynamic_routing::{ + health_check::HealthCheckStatus, node::Node, snapshot::routing_snapshot::RoutingSnapshot, + }, + RoutesStats, }; /// Routing snapshot, which samples nodes in a round-robin fashion. @@ -107,8 +110,8 @@ impl RoutingSnapshot for RoundRobinRoutingSnapshot { } } - fn nodes_stats(&self) -> (usize, Option) { - (self.existing_nodes.len(), Some(self.healthy_nodes.len())) + fn routes_stats(&self) -> RoutesStats { + RoutesStats::new(self.existing_nodes.len(), Some(self.healthy_nodes.len())) } } diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs index 89191137..b45fe585 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs @@ -1,6 +1,9 @@ use std::fmt::Debug; -use crate::agent::route_provider::dynamic_routing::{health_check::HealthCheckStatus, node::Node}; +use crate::agent::route_provider::{ + dynamic_routing::{health_check::HealthCheckStatus, node::Node}, + RoutesStats, +}; /// A trait for interacting with the snapshot of nodes (routing table). pub trait RoutingSnapshot: Send + Sync + Clone + Debug { @@ -15,11 +18,6 @@ pub trait RoutingSnapshot: Send + Sync + Clone + Debug { fn sync_nodes(&mut self, nodes: &[Node]) -> bool; /// Updates the health status of a specific node, returning `true` if the node was found and updated. fn update_node(&mut self, node: &Node, health: HealthCheckStatus) -> bool; - /// Returns the total number of nodes and healthy nodes as a tuple. - /// - /// - First element is the total number of nodes available (both healthy and unhealthy) - /// - Second element is the number of currently healthy nodes, or None if health status information is unavailable - /// - /// The specific criteria for what constitutes a "healthy" node is implementation dependent. - fn nodes_stats(&self) -> (usize, Option); + /// Returns statistics about the routes (nodes). + fn routes_stats(&self) -> RoutesStats; } From 5641c79804cd6a3e80f7e0411005d4f8cfbe3d72 Mon Sep 17 00:00:00 2001 From: Nikolay Komarevskiy Date: Thu, 23 Jan 2025 15:35:45 +0100 Subject: [PATCH 5/6] fix: missing docs --- ic-agent/src/agent/route_provider.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ic-agent/src/agent/route_provider.rs b/ic-agent/src/agent/route_provider.rs index f4b1869c..37caad2e 100644 --- a/ic-agent/src/agent/route_provider.rs +++ b/ic-agent/src/agent/route_provider.rs @@ -49,6 +49,7 @@ pub struct RoutesStats { } impl RoutesStats { + /// Creates an new instance of [`RoutesStats`]. pub fn new(total: usize, healthy: Option) -> Self { Self { total, healthy } } From 0e5f8031463203882562be45064f9c9436acfd5a Mon Sep 17 00:00:00 2001 From: Nikolay Komarevskiy <90605504+nikolay-komarevskiy@users.noreply.github.com> Date: Thu, 23 Jan 2025 15:36:20 +0100 Subject: [PATCH 6/6] Update CHANGELOG.md Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b2c6237..1bf867af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Added `CanisterInfo` to `MgmtMethod`. -* Extended `RouteProvider` trait with `fn routes_stats()`, returning the number of total and healthy routes as a tuple. +* Extended `RouteProvider` trait with `fn routes_stats()`, returning the number of total and healthy routes. ## [0.39.2] - 2024-12-20