NRC: Simplification metrics fetching and additional testing

rs/dre-canisters/node-provider-rewards/canister/src/metrics.rs

@@ -64,13 +64,13 @@ where
         last_stored_ts: Option<UnixTsNanos>,
         mut subnet_update: Vec<NodeMetricsHistoryResponse>,
     ) {
-        // Extract initial total metrics for each node in the subnet.
-        let mut initial_total_metrics_per_node: HashMap<_, _> = HashMap::new();
+        let mut last_total_metrics: HashMap<_, _> = HashMap::new();
 
         subnet_update.sort_by_key(|metrics| metrics.timestamp_nanos);
+        // Extract initial total metrics for each node in the subnet.
         if let Some(first_metrics) = subnet_update.first() {
             if Some(first_metrics.timestamp_nanos) == last_stored_ts {
-                initial_total_metrics_per_node = subnet_update
+                last_total_metrics = subnet_update
                     .remove(0)
                     .node_metrics
                     .iter()
@@ -84,12 +84,8 @@ where
             }
         };
 
-        let mut running_total_metrics_per_node = initial_total_metrics_per_node;
         for one_day_update in subnet_update {
-            let key = SubnetMetricsDailyKeyStored {
-                subnet_id,
-                ts: one_day_update.timestamp_nanos,
-            };
+            let mut current_total_metrics = HashMap::new();
 
             let daily_nodes_metrics: Vec<_> = one_day_update
                 .node_metrics
@@ -98,34 +94,32 @@ where
                     let current_proposed_total = node_metrics.num_blocks_proposed_total;
                     let current_failed_total = node_metrics.num_block_failures_total;
 
-                    let (mut running_proposed_total, mut running_failed_total) = running_total_metrics_per_node
+                    let (last_proposed_total, last_failed_total) = last_total_metrics
                         .remove(&node_metrics.node_id)
                         // Default is needed if the node joined the subnet after last_stored_ts.
                         .unwrap_or_default();
 
-                    // This can happen if the node was redeployed.
-                    if running_proposed_total > current_proposed_total || running_failed_total > current_failed_total {
-                        running_proposed_total = 0;
-                        running_failed_total = 0;
-                    };
-
                     // Update the total metrics for the next iteration.
-                    running_total_metrics_per_node.insert(node_metrics.node_id, (current_proposed_total, current_failed_total));
+                    current_total_metrics.insert(node_metrics.node_id, (current_proposed_total, current_failed_total));
 
                     NodeMetricsDailyStored {
                         node_id: node_metrics.node_id.into(),
-                        num_blocks_proposed: current_proposed_total - running_proposed_total,
-                        num_blocks_failed: current_failed_total - running_failed_total,
+                        num_blocks_proposed: current_proposed_total - last_proposed_total,
+                        num_blocks_failed: current_failed_total - last_failed_total,
                     }
                 })
                 .collect();
 
             self.subnets_metrics.borrow_mut().insert(
-                key,
+                SubnetMetricsDailyKeyStored {
+                    subnet_id,
+                    ts: one_day_update.timestamp_nanos,
+                },
                 SubnetMetricsDailyValueStored {
                     nodes_metrics: daily_nodes_metrics,
                 },
             );
+            last_total_metrics = current_total_metrics;
         }
     }
 

rs/dre-canisters/node-provider-rewards/canister/src/metrics/tests.rs

@@ -1,11 +1,11 @@
 use crate::metrics::{MetricsManager, UnixTsNanos};
-use crate::metrics_types::SubnetMetricsDailyKeyStored;
+use crate::metrics_types::{NodeMetricsDailyStored, SubnetMetricsDailyKeyStored};
 use ic_base_types::{NodeId, PrincipalId, SubnetId};
 use ic_cdk::call::{CallPerformFailed, CallResult};
 use ic_management_canister_types_private::{NodeMetrics, NodeMetricsHistoryArgs, NodeMetricsHistoryResponse};
 use ic_stable_structures::memory_manager::{MemoryId, VirtualMemory};
 use ic_stable_structures::DefaultMemoryImpl;
-use rewards_calculation::types::{DayEnd, NodeMetricsDailyRaw};
+use rewards_calculation::types::DayEnd;
 use std::cell::RefCell;
 use std::collections::{BTreeMap, HashMap};
 
@@ -258,7 +258,10 @@ impl NodeMetricsHistoryResponseTracker {
 async fn _daily_metrics_correct_different_update_size(size: usize) {
     let tracker = NodeMetricsHistoryResponseTracker::new()
         .with_subnet(subnet_id(1))
-        .add_node_metrics(node_id(1), vec![(0, vec![(7, 5), (10, 6), (15, 6), (25, 50), (10, 6)])]);
+        .add_node_metrics(node_id(1), vec![(0, vec![(7, 5), (10, 6), (15, 6), (25, 50)])])
+        .add_node_metrics(node_id(2), vec![(0, vec![(19, 21), (32, 22)])])
+        // Node 2 is redeployed to subnet 1 on day 2
+        .add_node_metrics(node_id(2), vec![(3 * ONE_DAY_NANOS, vec![(10, 10)])]);
 
     let mut mock = mock::MockCanisterClient::new();
     mock.expect_node_metrics_history()
@@ -268,35 +271,42 @@ async fn _daily_metrics_correct_different_update_size(size: usize) {
     for _ in 0..MAX_TIMES {
         mm.update_subnets_metrics(vec![subnet_id(1)]).await;
     }
-    let node_1_daily_metrics: Vec<NodeMetricsDailyRaw> = mm
+    let daily_metrics: Vec<Vec<NodeMetricsDailyStored>> = mm
         .subnets_metrics
         .borrow()
         .iter()
         .collect::<BTreeMap<_, _>>()
         .into_values()
-        .map(|node_metrics| node_metrics.nodes_metrics[0].clone().into())
+        .map(|node_metrics| node_metrics.nodes_metrics.clone())
         .collect();
 
     // (7, 5)
-    assert_eq!(node_1_daily_metrics[0].num_blocks_proposed, 7);
-    assert_eq!(node_1_daily_metrics[0].num_blocks_failed, 5);
+    assert_eq!(daily_metrics[0][0].num_blocks_proposed, 7);
+    assert_eq!(daily_metrics[0][0].num_blocks_failed, 5);
+
+    assert_eq!(daily_metrics[0][1].num_blocks_proposed, 19);
+    assert_eq!(daily_metrics[0][1].num_blocks_failed, 21);
 
     // (10 - 7, 6 - 5) = (3, 1)
-    assert_eq!(node_1_daily_metrics[1].num_blocks_proposed, 3);
-    assert_eq!(node_1_daily_metrics[1].num_blocks_failed, 1);
+    // (32 - 19, 22 - 21) = (13, 1)
+    assert_eq!(daily_metrics[1][0].num_blocks_proposed, 3);
+    assert_eq!(daily_metrics[1][0].num_blocks_failed, 1);
+
+    assert_eq!(daily_metrics[1][1].num_blocks_proposed, 13);
+    assert_eq!(daily_metrics[1][1].num_blocks_failed, 1);
 
     // (15 - 10, 6 - 6) = (5, 0)
-    assert_eq!(node_1_daily_metrics[2].num_blocks_proposed, 5);
-    assert_eq!(node_1_daily_metrics[2].num_blocks_failed, 0);
+    assert_eq!(daily_metrics[2][0].num_blocks_proposed, 5);
+    assert_eq!(daily_metrics[2][0].num_blocks_failed, 0);
+
+    assert_eq!(daily_metrics[2].len(), 1);
 
     // (25 - 15, 50 - 6) = (10, 44)
-    assert_eq!(node_1_daily_metrics[3].num_blocks_proposed, 10);
-    assert_eq!(node_1_daily_metrics[3].num_blocks_failed, 44);
+    assert_eq!(daily_metrics[3][0].num_blocks_proposed, 10);
+    assert_eq!(daily_metrics[3][0].num_blocks_failed, 44);
 
-    // Node is redeployed and added to the same subnet!
-    // (10, 6)
-    assert_eq!(node_1_daily_metrics[4].num_blocks_proposed, 10);
-    assert_eq!(node_1_daily_metrics[4].num_blocks_failed, 6);
+    assert_eq!(daily_metrics[3][1].num_blocks_proposed, 10);
+    assert_eq!(daily_metrics[3][1].num_blocks_failed, 10);
 }
 
 #[tokio::test]