Skip to content

Commit

Permalink
chore: refactor to RoutesStats
Browse files Browse the repository at this point in the history
  • Loading branch information
nikolay-komarevskiy committed Jan 22, 2025
1 parent 0f9ff48 commit 531acf4
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 53 deletions.
42 changes: 27 additions & 15 deletions ic-agent/src/agent/route_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<usize>,
}

impl RoutesStats {
fn new(total: usize, healthy: Option<usize>) -> 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.
Expand All @@ -52,14 +70,8 @@ pub trait RouteProvider: std::fmt::Debug + Send + Sync {
/// fewer are available.
fn n_ordered_routes(&self, n: usize) -> Result<Vec<Url>, 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<usize>);
/// 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.
Expand Down Expand Up @@ -104,8 +116,8 @@ impl RouteProvider for RoundRobinRouteProvider {
Ok(urls)
}

fn routes_stats(&self) -> (usize, Option<usize>) {
(self.routes.len(), None)
fn routes_stats(&self) -> RoutesStats {
RoutesStats::new(self.routes.len(), None)
}
}

Expand Down Expand Up @@ -146,8 +158,8 @@ impl RouteProvider for Url {
fn n_ordered_routes(&self, _: usize) -> Result<Vec<Url>, AgentError> {
Ok(vec![self.route()?])
}
fn routes_stats(&self) -> (usize, Option<usize>) {
(1, None)
fn routes_stats(&self) -> RoutesStats {
RoutesStats::new(1, None)
}
}

Expand Down Expand Up @@ -231,7 +243,7 @@ impl RouteProvider for DynamicRouteProvider {
fn n_ordered_routes(&self, n: usize) -> Result<Vec<Url>, AgentError> {
self.inner.n_ordered_routes(n)
}
fn routes_stats(&self) -> (usize, Option<usize>) {
fn routes_stats(&self) -> RoutesStats {
self.inner.routes_stats()
}
}
Expand Down Expand Up @@ -289,8 +301,8 @@ impl<R: RouteProvider> RouteProvider for UrlUntilReady<R> {
self.url.route()
}
}
fn routes_stats(&self) -> (usize, Option<usize>) {
(1, None)
fn routes_stats(&self) -> RoutesStats {
RoutesStats::new(1, None)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use crate::{
snapshot::routing_snapshot::RoutingSnapshot,
type_aliases::AtomicSwap,
},
RouteProvider,
RouteProvider, RoutesStats,
},
HttpService,
},
Expand Down Expand Up @@ -187,9 +187,9 @@ where
Ok(urls)
}

fn routes_stats(&self) -> (usize, Option<usize>) {
fn routes_stats(&self) -> RoutesStats {
let snapshot = self.routing_snapshot.load();
snapshot.nodes_stats()
snapshot.routes_stats()
}
}

Expand Down Expand Up @@ -296,7 +296,7 @@ mod tests {
assert_routed_domains, route_n_times, NodeHealthCheckerMock, NodesFetcherMock,
},
},
RouteProvider,
RouteProvider, RoutesStats,
},
Agent, AgentError,
};
Expand Down Expand Up @@ -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.
Expand All @@ -437,15 +437,15 @@ 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.
checker.overwrite_healthy_nodes(vec![node_1.clone(), node_3.clone()]);
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.
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -323,8 +326,8 @@ impl RoutingSnapshot for LatencyRoutingSnapshot {
true
}

fn nodes_stats(&self) -> (usize, Option<usize>) {
(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()))
}
}

Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -384,15 +390,15 @@ 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);
assert!(snapshot.has_nodes());
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);
Expand All @@ -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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -107,8 +110,8 @@ impl RoutingSnapshot for RoundRobinRoutingSnapshot {
}
}

fn nodes_stats(&self) -> (usize, Option<usize>) {
(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()))
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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<usize>);
/// Returns statistics about the routes (nodes).
fn routes_stats(&self) -> RoutesStats;
}

0 comments on commit 531acf4

Please sign in to comment.