From 34c13219ae62aefa24dbd5007079c2654f14ab9b Mon Sep 17 00:00:00 2001 From: Frederik Rothenberger Date: Wed, 2 Oct 2024 16:57:50 +0200 Subject: [PATCH] Remove unnecessary counters from persistent state (#2639) The `StableBTreeMap` tracks the number of elements on its own, so there is no need for an external counter. Removing the extra counters also makes the stats resetting obsolete and hence allows to remove the pre_stats II version from the integration tests. --- .github/workflows/canister-tests.yml | 1 - src/canister_tests/src/framework.rs | 14 ----- src/internet_identity/src/state.rs | 23 +------- .../src/stats/event_stats.rs | 47 ++--------------- .../src/stats/event_stats/tests.rs | 33 ------------ .../src/storage/storable_persistent_state.rs | 11 ---- .../tests/integration/aggregation_stats.rs | 52 +------------------ 7 files changed, 7 insertions(+), 174 deletions(-) diff --git a/.github/workflows/canister-tests.yml b/.github/workflows/canister-tests.yml index 3c0d11fdcc..3a31591384 100644 --- a/.github/workflows/canister-tests.yml +++ b/.github/workflows/canister-tests.yml @@ -383,7 +383,6 @@ jobs: # a relatively small price to pay to make sure PRs are always tested against the latest release. curl -sSL https://github.com/dfinity/internet-identity/releases/latest/download/internet_identity_test.wasm.gz -o internet_identity_previous.wasm.gz curl -sSL https://github.com/dfinity/internet-identity/releases/latest/download/archive.wasm.gz -o archive_previous.wasm.gz - curl -SL https://github.com/dfinity/internet-identity/releases/download/release-2024-04-26/internet_identity_test.wasm.gz -o internet_identity_pre_stats.wasm.gz # We are using --partition hash instead of count, because it makes sure that the tests partition is stable across runs # even if tests are added or removed. The tradeoff is that the balancing might be slightly worse, but we have enough diff --git a/src/canister_tests/src/framework.rs b/src/canister_tests/src/framework.rs index ecd5df18e9..0bc05dc661 100644 --- a/src/canister_tests/src/framework.rs +++ b/src/canister_tests/src/framework.rs @@ -69,20 +69,6 @@ lazy_static! { get_wasm_path("II_WASM_PREVIOUS".to_string(), &def_path).expect(&err) }; - /** The gzipped Wasm module for the II release build 2024-04-26, before the event stats were introduced. */ - pub static ref II_WASM_PRE_STATS: Vec = { - let def_path = path::PathBuf::from("..").join("..").join("internet_identity_pre_stats.wasm.gz"); - let err = format!(" - Could not find Internet Identity Wasm module for pre-stats release. - - I will look for it at {:?}, and you can specify another path with the environment variable II_WASM_PREVIOUS (note that I run from {:?}). - - In order to get the Wasm module, please run the following command: - curl -SL https://github.com/dfinity/internet-identity/releases/download/release-2024-04-26/internet_identity_test.wasm.gz -o internet_identity_pre_stats.wasm.gz - ", &def_path, &std::env::current_dir().map(|x| x.display().to_string()).unwrap_or_else(|_| "an unknown directory".to_string())); - get_wasm_path("II_WASM_PRE_STATS".to_string(), &def_path).expect(&err) - }; - /** The gzipped Wasm module for the _previous_ archive build, or latest release, which is used when testing * upgrades and downgrades */ pub static ref ARCHIVE_WASM_PREVIOUS: Vec = { diff --git a/src/internet_identity/src/state.rs b/src/internet_identity/src/state.rs index 5448038991..c56aa197a2 100644 --- a/src/internet_identity/src/state.rs +++ b/src/internet_identity/src/state.rs @@ -103,14 +103,6 @@ pub struct PersistentState { pub active_authn_method_stats: ActivityStats, // Configuration of the captcha challenge during registration flow pub captcha_config: CaptchaConfig, - // Count of entries in the event_data BTreeMap - // event_data is expected to have a lot of entries, thus counting by iterating over it is not - // an option. - pub event_data_count: u64, - // Count of entries in the event_aggregations BTreeMap - // event_aggregations is expected to have a lot of entries, thus counting by iterating over it is not - // an option. - pub event_aggregations_count: u64, // Key into the event_data BTreeMap where the 24h tracking window starts. // This key is used to remove old entries from the 24h event aggregations. // If it is `none`, then the 24h window starts from the newest entry in the event_data @@ -129,8 +121,6 @@ impl Default for PersistentState { domain_active_anchor_stats: ActivityStats::new(time), active_authn_method_stats: ActivityStats::new(time), captcha_config: DEFAULT_CAPTCHA_CONFIG, - event_data_count: 0, - event_aggregations_count: 0, event_stats_24h_start: None, } } @@ -257,17 +247,8 @@ pub fn save_persistent_state() { pub fn load_persistent_state() { STATE.with(|s| { - let loaded_state = storage_borrow(|storage| storage.read_persistent_state()); - - // Reset the event_data and event_aggregations if they are reported as empty from the persistent state - // Necessary to handle rollback across the versions where the event_data and event_aggregations were introduced - if loaded_state.event_aggregations_count == 0 { - storage_borrow_mut(|storage| storage.event_aggregations.clear_new()); - } - if loaded_state.event_data_count == 0 { - storage_borrow_mut(|storage| storage.event_data.clear_new()); - } - *s.persistent_state.borrow_mut() = loaded_state; + *s.persistent_state.borrow_mut() = + storage_borrow(|storage| storage.read_persistent_state()); }); } diff --git a/src/internet_identity/src/stats/event_stats.rs b/src/internet_identity/src/stats/event_stats.rs index 260be3f7bb..2137599c9c 100644 --- a/src/internet_identity/src/stats/event_stats.rs +++ b/src/internet_identity/src/stats/event_stats.rs @@ -207,11 +207,7 @@ fn update_events_internal(event: EventData, now: Timestamp, s: &mut S if option.is_some() { ic_cdk::println!("WARN: Event already exists for key {:?}", current_key); } - state::persistent_state_mut(|s| { - s.event_data_count = s.event_data_count.saturating_add(1); - }); - let mut aggregations_db_wrapper = CountingAggregationsWrapper(&mut s.event_aggregations); // Collect events that should no longer be reflected in the 24h aggregations. Does _not_ delete // the underlying events. let removed_24h = events_to_remove_from_24h_aggregations(now, ¤t_key, &s.event_data); @@ -221,7 +217,7 @@ fn update_events_internal(event: EventData, now: Timestamp, s: &mut S &event, &removed_24h, AggregationWindow::Day, - &mut aggregations_db_wrapper, + &mut s.event_aggregations, ); // This pruning _deletes_ the data older than 30 days. Do this after the 24h aggregation @@ -234,7 +230,7 @@ fn update_events_internal(event: EventData, now: Timestamp, s: &mut S &event, &pruned_30d, AggregationWindow::Month, - &mut aggregations_db_wrapper, + &mut s.event_aggregations, ); } @@ -244,7 +240,7 @@ fn update_aggregations( event_data: &EventData, pruned_events: &[(EventKey, EventData)], window: AggregationWindow, - aggregations_db: &mut CountingAggregationsWrapper, + aggregations_db: &mut StableBTreeMap, ) { AGGREGATIONS.iter().for_each(|aggregation| { update_aggregation( @@ -343,49 +339,14 @@ fn prune_events( let entry: &(EventKey, EventData) = entry; db.remove(&entry.0); } - state::persistent_state_mut(|s| { - s.event_data_count = s - .event_data_count - .saturating_sub(pruned_events.len() as u64); - }); pruned_events } - -/// Helper struct for updating the count of event aggregations. -struct CountingAggregationsWrapper<'a, M: Memory>(&'a mut StableBTreeMap); - -impl<'a, M: Memory> CountingAggregationsWrapper<'a, M> { - fn insert(&mut self, key: AggregationKey, value: u64) { - let prev_value = self.0.insert(key, value); - if prev_value.is_none() { - // Increase count because we added a new value - state::persistent_state_mut(|s| { - s.event_aggregations_count = s.event_aggregations_count.saturating_add(1); - }) - } - } - - fn get(&self, key: &AggregationKey) -> Option { - self.0.get(key) - } - - fn remove(&mut self, key: &AggregationKey) { - let prev_value = self.0.remove(key); - if prev_value.is_some() { - // Decrease count because we removed a value - state::persistent_state_mut(|s| { - s.event_aggregations_count = s.event_aggregations_count.saturating_sub(1); - }) - } - } -} - fn update_aggregation( aggregation_filter_map: F, now: EventKey, new_event: EventData, pruned_events: &[(EventKey, EventData)], - db: &mut CountingAggregationsWrapper, + db: &mut StableBTreeMap, ) where F: Fn(&(EventKey, EventData)) -> Option, { diff --git a/src/internet_identity/src/stats/event_stats/tests.rs b/src/internet_identity/src/stats/event_stats/tests.rs index f09f7e294f..acd864050d 100644 --- a/src/internet_identity/src/stats/event_stats/tests.rs +++ b/src/internet_identity/src/stats/event_stats/tests.rs @@ -75,8 +75,6 @@ fn should_store_event_and_add_to_aggregations() { .unwrap(), event ); - - assert_event_count_consistent(&mut storage); } #[test] @@ -98,7 +96,6 @@ fn should_store_multiple_events_with_same_timestamp() { assert_eq!(key.time, TIMESTAMP); assert_eq!(value, event.clone()); }); - assert_event_count_consistent(&mut storage); } #[test] @@ -149,7 +146,6 @@ fn should_track_ii_domains() { for key in expected_aggregations.iter() { assert!(storage.event_aggregations.contains_key(key)); } - assert_event_count_consistent(&mut storage); } #[test] @@ -191,7 +187,6 @@ fn should_track_multiple_frontends() { for key in expected_aggregations.iter() { assert!(storage.event_aggregations.contains_key(key)); } - assert_event_count_consistent(&mut storage); } #[test] @@ -258,7 +253,6 @@ fn should_store_multiple_events_and_aggregate_expected_weight_count() { .unwrap(), SESS_DURATION_SEC * 2 ); - assert_event_count_consistent(&mut storage); } #[test] @@ -273,9 +267,7 @@ fn should_remove_daily_events_after_24h() { }; update_events_internal(event.clone(), TIMESTAMP, &mut storage); - assert_event_count_consistent(&mut storage); update_events_internal(event.clone(), TIMESTAMP + DAY_NS, &mut storage); - assert_event_count_consistent(&mut storage); assert_eq!(storage.event_data.len(), 2); assert_eq!(storage.event_aggregations.len(), 4); @@ -341,9 +333,7 @@ fn should_prune_monthly_events_after_30d() { }; update_events_internal(event.clone(), TIMESTAMP, &mut storage); - assert_event_count_consistent(&mut storage); update_events_internal(event.clone(), TIMESTAMP + DAY_NS * 30, &mut storage); - assert_event_count_consistent(&mut storage); assert_eq!(storage.event_data.len(), 1); assert_eq!(storage.event_aggregations.len(), 4); @@ -419,7 +409,6 @@ fn should_remove_at_most_100_events_24h() { } update_events_internal(event.clone(), TIMESTAMP + DAY_NS, &mut storage); - assert_event_count_consistent(&mut storage); // Of the 107 initial events, 100 should be removed, 1 was added to trigger the clean-up // --> 8 expected events @@ -436,7 +425,6 @@ fn should_remove_at_most_100_events_24h() { } ); update_events_internal(event.clone(), TIMESTAMP + 2 * DAY_NS, &mut storage); - assert_event_count_consistent(&mut storage); // Clean-up again, after another 24h leaving only the event that triggered the clean-up // --> 1 expected events assert_eq!(storage.event_aggregations.get(&aggregation_key).unwrap(), 1); @@ -473,14 +461,12 @@ fn should_prune_at_most_100_events_30d() { } update_events_internal(event.clone(), TIMESTAMP + 30 * DAY_NS, &mut storage); - assert_event_count_consistent(&mut storage); // Of the 107 initial events, 100 should be pruned, 1 was added to trigger the pruning // --> 8 expected events assert_eq!(storage.event_aggregations.get(&aggregation_key).unwrap(), 8); assert_eq!(storage.event_data.len(), 8); update_events_internal(event.clone(), TIMESTAMP + 60 * DAY_NS, &mut storage); - assert_event_count_consistent(&mut storage); // Prune again, after another 30d leaving only the event that triggered the pruning // --> 1 expected events assert_eq!(storage.event_aggregations.get(&aggregation_key).unwrap(), 1); @@ -498,7 +484,6 @@ fn should_account_for_dapps_changing_session_lifetime() { }), }; update_events_internal(event1, TIMESTAMP, &mut storage); - assert_event_count_consistent(&mut storage); assert_eq!( storage @@ -521,7 +506,6 @@ fn should_account_for_dapps_changing_session_lifetime() { }), }; update_events_internal(event2, TIMESTAMP + 1, &mut storage); - assert_event_count_consistent(&mut storage); assert_eq!( storage @@ -544,7 +528,6 @@ fn should_account_for_dapps_changing_session_lifetime() { }), }; update_events_internal(event3, TIMESTAMP + 1 + DAY_NS, &mut storage); - assert_event_count_consistent(&mut storage); assert_eq!( storage @@ -572,7 +555,6 @@ fn should_remove_aggregations_without_events_when_pruning() { }; update_events_internal(event, TIMESTAMP, &mut storage); - assert_event_count_consistent(&mut storage); assert_eq!(storage.event_aggregations.len(), 4); let expected_aggregations: [AggregationKey; 4] = [ @@ -616,7 +598,6 @@ fn should_remove_aggregations_without_events_when_pruning() { // after this update, the previous aggregations should be removed as their weight is 0 // (this means that there is no event being counted for them) update_events_internal(event2, TIMESTAMP + 30 * DAY_NS, &mut storage); - assert_event_count_consistent(&mut storage); let expected_aggregations_2: [AggregationKey; 4] = [ AggregationKey::new( @@ -706,7 +687,6 @@ fn should_ignore_prune_events_for_prepare_delegation_aggregations() { for key in pd_count_aggregations.iter() { assert_eq!(storage.event_aggregations.get(key).unwrap(), 1); } - assert_event_count_consistent(&mut storage); } #[test] @@ -724,7 +704,6 @@ fn should_add_prune_events_to_prune_aggregations() { for key in sess_sec_aggregations.iter() { assert_eq!(storage.event_aggregations.get(key).unwrap(), 3); } - assert_event_count_consistent(&mut storage); } #[test] @@ -801,18 +780,6 @@ fn should_wrap_event_key_counter_correctly() { assert!(next_key > key); } -/// Make sure the cached count values are consistent with the actual data -fn assert_event_count_consistent(storage: &mut Storage>>>) { - assert_eq!( - storage.event_data.iter().count(), - persistent_state(|s| s.event_data_count) as usize - ); - assert_eq!( - storage.event_aggregations.iter().count(), - persistent_state(|s| s.event_aggregations_count) as usize - ); -} - fn to_ns(secs: u64) -> u64 { Duration::from_secs(secs).as_nanos() as u64 } diff --git a/src/internet_identity/src/storage/storable_persistent_state.rs b/src/internet_identity/src/storage/storable_persistent_state.rs index 93e8bca03b..c3f8d1fcd8 100644 --- a/src/internet_identity/src/storage/storable_persistent_state.rs +++ b/src/internet_identity/src/storage/storable_persistent_state.rs @@ -30,10 +30,7 @@ pub struct StorablePersistentState { max_inflight_captchas: u64, // opt fields because of backwards compatibility - event_data_count: Option, - event_aggregations_count: Option, event_stats_24h_start: Option, - captcha_config: Option, } @@ -70,8 +67,6 @@ impl From for StorablePersistentState { max_num_latest_delegation_origins: 0, // unused, kept for stable memory compatibility max_inflight_captchas: 0, - event_data_count: Some(s.event_data_count), - event_aggregations_count: Some(s.event_aggregations_count), event_stats_24h_start: s.event_stats_24h_start, captcha_config: Some(s.captcha_config), } @@ -88,8 +83,6 @@ impl From for PersistentState { domain_active_anchor_stats: s.domain_active_anchor_stats, active_authn_method_stats: s.active_authn_method_stats, captcha_config: s.captcha_config.unwrap_or(DEFAULT_CAPTCHA_CONFIG), - event_data_count: s.event_data_count.unwrap_or_default(), - event_aggregations_count: s.event_aggregations_count.unwrap_or_default(), event_stats_24h_start: s.event_stats_24h_start, } } @@ -129,8 +122,6 @@ mod tests { latest_delegation_origins: HashMap::new(), max_num_latest_delegation_origins: 0, max_inflight_captchas: 0, - event_data_count: Some(0), - event_aggregations_count: Some(0), event_stats_24h_start: None, captcha_config: Some(CaptchaConfig { max_unsolved_captchas: 500, @@ -154,8 +145,6 @@ mod tests { max_unsolved_captchas: 500, captcha_trigger: CaptchaTrigger::Static(StaticCaptchaTrigger::CaptchaEnabled), }, - event_data_count: 0, - event_aggregations_count: 0, event_stats_24h_start: None, }; assert_eq!(PersistentState::default(), expected_defaults); diff --git a/src/internet_identity/tests/integration/aggregation_stats.rs b/src/internet_identity/tests/integration/aggregation_stats.rs index aef41ba868..6a6d2200b1 100644 --- a/src/internet_identity/tests/integration/aggregation_stats.rs +++ b/src/internet_identity/tests/integration/aggregation_stats.rs @@ -4,7 +4,7 @@ use crate::v2_api::authn_method_test_helpers::{ use canister_tests::api::internet_identity as api; use canister_tests::framework::{ assert_metric, env, get_metrics, install_ii_canister, restore_compressed_stable_memory, - upgrade_ii_canister, EMPTY_WASM, II_WASM, II_WASM_PRE_STATS, + upgrade_ii_canister, EMPTY_WASM, II_WASM, }; use ic_cdk::api::management_canister::main::CanisterId; use internet_identity_interface::internet_identity::types::{ @@ -264,56 +264,6 @@ fn should_keep_aggregations_across_upgrades() -> Result<(), CallError> { Ok(()) } -#[test] -fn should_reset_stats_on_rollback_and_upgrade() -> Result<(), CallError> { - const II_ORIGIN: &str = "ic0.app"; - - let env = env(); - let canister_id = install_ii_canister(&env, II_WASM.clone()); - let identity_nr = create_identity(&env, canister_id, II_ORIGIN); - - delegation_for_origin(&env, canister_id, identity_nr, "https://some-dapp.com")?; - delegation_for_origin(&env, canister_id, identity_nr, "https://some-dapp.com")?; - - let aggregations = api::stats(&env, canister_id)?.event_aggregations; - assert_expected_aggregation( - &aggregations, - &aggregation_key(PD_COUNT, "24h", II_ORIGIN), - vec![("https://some-dapp.com".to_string(), 2u64)], - ); - assert_expected_aggregation( - &aggregations, - &aggregation_key(PD_COUNT, "30d", II_ORIGIN), - vec![("https://some-dapp.com".to_string(), 2u64)], - ); - assert_expected_aggregation( - &aggregations, - &aggregation_key(PD_SESS_SEC, "24h", II_ORIGIN), - vec![("https://some-dapp.com".to_string(), 2 * SESSION_LENGTH)], - ); - assert_expected_aggregation( - &aggregations, - &aggregation_key(PD_SESS_SEC, "30d", II_ORIGIN), - vec![("https://some-dapp.com".to_string(), 2 * SESSION_LENGTH)], - ); - let metrics = get_metrics(&env, canister_id); - assert_metric(&metrics, "internet_identity_event_data_count", 2f64); - assert_metric(&metrics, "internet_identity_event_aggregations_count", 4f64); - - // roll back to a version that does not know about event stats - upgrade_ii_canister(&env, canister_id, II_WASM_PRE_STATS.clone()); - // upgrade to the latest version again --> stats should be reset - upgrade_ii_canister(&env, canister_id, II_WASM.clone()); - - let aggregations = api::stats(&env, canister_id)?.event_aggregations; - assert_eq!(aggregations.len(), 0); - - let metrics = get_metrics(&env, canister_id); - assert_metric(&metrics, "internet_identity_event_data_count", 0f64); - assert_metric(&metrics, "internet_identity_event_aggregations_count", 0f64); - Ok(()) -} - #[test] fn crash_mem_backup() -> Result<(), CallError> { let env = env();