From e317c0cc662ddf0c704626f0c7d204600ef96d49 Mon Sep 17 00:00:00 2001 From: Beth Rennie Date: Tue, 3 Feb 2026 15:26:41 -0500 Subject: [PATCH] Bug 2018675 - Do not use a callback interface for MetricsHandler Using a callback interface forces the type to come in as `Box` instead of `Arc` as a regular interface. We're storing it as an `Arc>` because we need it to be `Send`, so using a plain interface allows us to drop the `Box` and store a `Arc` directly, preventing an additional layer of indirection. Because the MetricsHandler is in an Arc, we can keep an `Arc` around and use it directly instead of dowcasting the reference inside `Arc` and so we can remove the `NimbusClient::get_metrics_handler()` test method. --- components/nimbus/src/cirrus.udl | 5 +- components/nimbus/src/metrics.rs | 1 + components/nimbus/src/nimbus.udl | 3 +- .../nimbus/src/stateful/nimbus_client.rs | 17 +- .../nimbus/src/stateless/cirrus_client.rs | 6 +- components/nimbus/src/tests/helpers.rs | 18 +- .../tests/stateful/client/test_null_client.rs | 3 +- .../nimbus/src/tests/stateful/test_nimbus.rs | 177 ++++++------------ .../src/tests/stateless/test_cirrus_client.rs | 8 +- components/nimbus/tests/common.rs | 2 +- components/nimbus/tests/test_fs_client.rs | 2 +- examples/nimbus/src/main.rs | 2 +- 12 files changed, 88 insertions(+), 156 deletions(-) diff --git a/components/nimbus/src/cirrus.udl b/components/nimbus/src/cirrus.udl index 49a984b990..37689c0f9b 100644 --- a/components/nimbus/src/cirrus.udl +++ b/components/nimbus/src/cirrus.udl @@ -11,7 +11,8 @@ enum NimbusError { "ParseIntError", "TransformParameterError", "CirrusError", "UniFFICallbackError" }; -callback interface MetricsHandler { +[Trait, WithForeign] +interface MetricsHandler { void record_enrollment_statuses_v2(sequence enrollment_status_extras, string? nimbus_user_id); }; @@ -36,4 +37,4 @@ interface CirrusClient { /// Sets the experiments list in the CirrusClient's internal state. [Throws=NimbusError] void set_experiments(string experiments); -}; \ No newline at end of file +}; diff --git a/components/nimbus/src/metrics.rs b/components/nimbus/src/metrics.rs index a7333caa26..70537b6d51 100644 --- a/components/nimbus/src/metrics.rs +++ b/components/nimbus/src/metrics.rs @@ -10,6 +10,7 @@ use crate::{EnrolledFeature, EnrollmentStatus}; #[cfg(feature = "stateful")] use crate::enrollment::PreviousGeckoPrefState; +#[uniffi::trait_interface] pub trait MetricsHandler: Send + Sync { #[cfg(feature = "stateful")] fn record_enrollment_statuses(&self, enrollment_status_extras: Vec); diff --git a/components/nimbus/src/nimbus.udl b/components/nimbus/src/nimbus.udl index 329b43ac54..a29a42d5f0 100644 --- a/components/nimbus/src/nimbus.udl +++ b/components/nimbus/src/nimbus.udl @@ -83,7 +83,8 @@ enum EnrollmentChangeEventType { "UnenrollFailed", }; -callback interface MetricsHandler { +[Trait, WithForeign] +interface MetricsHandler { void record_enrollment_statuses(sequence enrollment_status_extras); /// Feature activation is the pre-cursor to feature exposure: it is defined as the first time /// the feature configuration is asked for. diff --git a/components/nimbus/src/stateful/nimbus_client.rs b/components/nimbus/src/stateful/nimbus_client.rs index 7df00f9ff5..0ede59dbd9 100644 --- a/components/nimbus/src/stateful/nimbus_client.rs +++ b/components/nimbus/src/stateful/nimbus_client.rs @@ -47,7 +47,7 @@ use crate::stateful::targeting::{RecordedContext, validate_event_queries}; use crate::stateful::updating::{read_and_remove_pending_experiments, write_pending_experiments}; use crate::strings::fmt_with_map; #[cfg(test)] -use crate::tests::helpers::{TestGeckoPrefHandler, TestMetrics, TestRecordedContext}; +use crate::tests::helpers::{TestGeckoPrefHandler, TestRecordedContext}; use crate::{ AvailableExperiment, AvailableRandomizationUnits, EnrolledExperiment, EnrollmentStatus, }; @@ -95,7 +95,7 @@ pub struct NimbusClient { event_store: Arc>, recorded_context: Option>, pub(crate) gecko_prefs: Option>, - metrics_handler: Arc>, + metrics_handler: Arc, } impl NimbusClient { @@ -107,7 +107,7 @@ impl NimbusClient { recorded_context: Option>, coenrolling_feature_ids: Vec, db_path: P, - metrics_handler: Box, + metrics_handler: Arc, gecko_pref_handler: Option>, remote_settings_info: Option, ) -> Result { @@ -146,7 +146,7 @@ impl NimbusClient { event_store: Arc::default(), recorded_context, gecko_prefs: prefs, - metrics_handler: Arc::new(metrics_handler), + metrics_handler, }) } @@ -887,15 +887,6 @@ impl NimbusClient { })) } - #[cfg(test)] - pub fn get_metrics_handler(&self) -> &&TestMetrics { - let metrics = &**self.metrics_handler; - // SAFETY: The cast to TestMetrics is safe because the Rust instance is guaranteed to be - // a TestMetrics instance. TestMetrics is the only Rust-implemented version of - // MetricsHandler, and, like this method, is only used in tests. - unsafe { std::mem::transmute::<&&dyn MetricsHandler, &&TestMetrics>(&metrics) } - } - #[cfg(test)] pub fn get_recorded_context(&self) -> &&TestRecordedContext { self.recorded_context diff --git a/components/nimbus/src/stateless/cirrus_client.rs b/components/nimbus/src/stateless/cirrus_client.rs index 808efd4a4e..a7143f9ca0 100644 --- a/components/nimbus/src/stateless/cirrus_client.rs +++ b/components/nimbus/src/stateless/cirrus_client.rs @@ -65,13 +65,13 @@ pub struct CirrusClient { app_context: AppContext, coenrolling_feature_ids: Vec, state: Mutex, - metrics_handler: Arc>, + metrics_handler: Arc, } impl CirrusClient { pub fn new( app_context: String, - metrics_handler: Box, + metrics_handler: Arc, coenrolling_feature_ids: Vec, ) -> Result { let app_context: AppContext = match serde_json::from_str(&app_context) { @@ -82,7 +82,7 @@ impl CirrusClient { app_context, coenrolling_feature_ids, state: Default::default(), - metrics_handler: Arc::new(metrics_handler), + metrics_handler, }) } diff --git a/components/nimbus/src/tests/helpers.rs b/components/nimbus/src/tests/helpers.rs index 56e120c8bf..56c8746bbc 100644 --- a/components/nimbus/src/tests/helpers.rs +++ b/components/nimbus/src/tests/helpers.rs @@ -154,7 +154,9 @@ impl RecordedContext for TestRecordedContext { } } -#[derive(Default)] +/// A Rust implementation of the MetricsHandler trait +/// Used to test recording of Glean metrics across the FFI within Rust +#[derive(Clone, Default)] struct MetricState { enrollment_statuses: Vec, #[cfg(feature = "stateful")] @@ -173,16 +175,15 @@ struct MetricState { /// Used to test recording of Glean metrics across the FFI within Rust /// /// *NOTE: Use this struct's `new` method when instantiating it to lock the Glean store* -#[derive(Clone, Default)] pub struct TestMetrics { - state: Arc>, + state: Mutex, } impl TestMetrics { - pub fn new() -> Self { - TestMetrics { + pub fn new() -> Arc { + Arc::new(TestMetrics { state: Default::default(), - } + }) } pub fn get_enrollment_statuses(&self) -> Vec { @@ -198,10 +199,7 @@ impl TestMetrics { #[cfg(feature = "stateful")] impl TestMetrics { pub fn clear(&self) { - let mut state = self.state.lock().unwrap(); - state.activations.clear(); - state.enrollment_statuses.clear(); - state.malformeds.clear(); + std::mem::take(&mut *self.state.lock().unwrap()); } pub fn get_activations(&self) -> Vec { diff --git a/components/nimbus/src/tests/stateful/client/test_null_client.rs b/components/nimbus/src/tests/stateful/client/test_null_client.rs index c4aaa45dbd..c8145123b8 100644 --- a/components/nimbus/src/tests/stateful/client/test_null_client.rs +++ b/components/nimbus/src/tests/stateful/client/test_null_client.rs @@ -12,7 +12,6 @@ use crate::tests::helpers::TestMetrics; #[cfg(feature = "rkv-safe-mode")] #[test] fn test_null_client() -> Result<()> { - let metrics = TestMetrics::new(); error_support::init_for_tests(); let tmp_dir = tempfile::tempdir()?; @@ -22,7 +21,7 @@ fn test_null_client() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; diff --git a/components/nimbus/src/tests/stateful/test_nimbus.rs b/components/nimbus/src/tests/stateful/test_nimbus.rs index 4d9debe2a0..c194a98426 100644 --- a/components/nimbus/src/tests/stateful/test_nimbus.rs +++ b/components/nimbus/src/tests/stateful/test_nimbus.rs @@ -49,7 +49,7 @@ fn test_telemetry_reset() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics), + metrics.clone(), None, None, )?; @@ -127,7 +127,6 @@ fn test_telemetry_reset() -> Result<()> { #[test] fn test_installation_date() -> Result<()> { - let metrics = TestMetrics::new(); let tmp_dir = tempfile::tempdir()?; // Step 1: We first test that the SDK will default to using the // value in the app context if it exists @@ -142,7 +141,7 @@ fn test_installation_date() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics.clone()), + TestMetrics::new(), None, None, )?; @@ -177,7 +176,7 @@ fn test_installation_date() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics.clone()), + TestMetrics::new(), None, None, )?; @@ -199,7 +198,7 @@ fn test_installation_date() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics.clone()), + TestMetrics::new(), None, None, )?; @@ -229,7 +228,7 @@ fn test_installation_date() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -242,7 +241,6 @@ fn test_installation_date() -> Result<()> { #[test] fn test_days_since_calculation_happens_at_startup() -> Result<()> { - let metrics = TestMetrics::new(); // Set up a client with an install date. // We'll need two of these, to test the two scenarios. let tmp_dir = tempfile::tempdir()?; @@ -258,7 +256,7 @@ fn test_days_since_calculation_happens_at_startup() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics.clone()), + TestMetrics::new(), None, None, )?; @@ -286,7 +284,7 @@ fn test_days_since_calculation_happens_at_startup() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -300,14 +298,13 @@ fn test_days_since_calculation_happens_at_startup() -> Result<()> { #[test] fn test_days_since_update_changes_with_context() -> Result<()> { - let metrics = TestMetrics::new(); let tmp_dir = tempfile::tempdir()?; let client = NimbusClient::new( AppContext::default(), Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics.clone()), + TestMetrics::new(), None, None, )?; @@ -328,7 +325,7 @@ fn test_days_since_update_changes_with_context() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics.clone()), + TestMetrics::new(), None, None, )?; @@ -356,7 +353,7 @@ fn test_days_since_update_changes_with_context() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics.clone()), + TestMetrics::new(), None, None, )?; @@ -390,7 +387,7 @@ fn test_days_since_update_changes_with_context() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -418,8 +415,6 @@ fn test_days_since_update_changes_with_context() -> Result<()> { #[test] fn test_days_since_install() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { app_name: "fenix".to_string(), @@ -432,7 +427,7 @@ fn test_days_since_install() -> Result<()> { Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -489,8 +484,6 @@ fn test_days_since_install() -> Result<()> { #[test] fn test_days_since_install_failed_targeting() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { app_name: "fenix".to_string(), @@ -503,7 +496,7 @@ fn test_days_since_install_failed_targeting() -> Result<()> { Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -559,8 +552,6 @@ fn test_days_since_install_failed_targeting() -> Result<()> { #[test] fn test_days_since_update() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { app_name: "fenix".to_string(), @@ -573,7 +564,7 @@ fn test_days_since_update() -> Result<()> { Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -630,8 +621,6 @@ fn test_days_since_update() -> Result<()> { #[test] fn test_days_since_update_failed_targeting() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { app_name: "fenix".to_string(), @@ -644,7 +633,7 @@ fn test_days_since_update_failed_targeting() -> Result<()> { Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -700,8 +689,6 @@ fn test_days_since_update_failed_targeting() -> Result<()> { #[test] fn event_store_exists_for_apply_pending_experiments() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let db = Database::new(temp_dir.path())?; @@ -727,7 +714,7 @@ fn event_store_exists_for_apply_pending_experiments() -> Result<()> { Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -822,8 +809,6 @@ fn event_store_exists_for_apply_pending_experiments() -> Result<()> { #[test] fn event_store_on_targeting_attributes_is_updated_after_an_event_is_recorded() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let db = Database::new(temp_dir.path())?; @@ -849,7 +834,7 @@ fn event_store_on_targeting_attributes_is_updated_after_an_event_is_recorded() - Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -917,7 +902,6 @@ fn event_store_on_targeting_attributes_is_updated_after_an_event_is_recorded() - #[cfg(feature = "stateful")] #[test] fn test_ios_rollout() -> Result<()> { - let metrics = TestMetrics::new(); let ctx = AppContext { app_name: "firefox_ios".to_string(), channel: "release".to_string(), @@ -931,7 +915,7 @@ fn test_ios_rollout() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -953,7 +937,6 @@ fn test_ios_rollout() -> Result<()> { #[test] fn test_fetch_enabled() -> Result<()> { - let metrics = TestMetrics::new(); let ctx = AppContext { app_name: "firefox_ios".to_string(), channel: "release".to_string(), @@ -967,7 +950,7 @@ fn test_fetch_enabled() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics.clone()), + TestMetrics::new(), None, None, )?; @@ -981,7 +964,7 @@ fn test_fetch_enabled() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -991,8 +974,6 @@ fn test_fetch_enabled() -> Result<()> { #[test] fn test_active_enrollment_in_targeting() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { @@ -1006,7 +987,7 @@ fn test_active_enrollment_in_targeting() -> Result<()> { Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -1050,8 +1031,6 @@ fn test_active_enrollment_in_targeting() -> Result<()> { #[test] fn test_previous_enrollments_in_targeting() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let slug_1 = "experiment-1-was-enrolled"; @@ -1071,7 +1050,7 @@ fn test_previous_enrollments_in_targeting() -> Result<()> { Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -1197,8 +1176,6 @@ fn test_previous_enrollments_in_targeting() -> Result<()> { #[test] fn test_opt_out_multiple_experiments_same_feature_does_not_re_enroll() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let slug_1 = "experiment-1"; @@ -1215,7 +1192,7 @@ fn test_opt_out_multiple_experiments_same_feature_does_not_re_enroll() -> Result Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -1261,7 +1238,7 @@ fn test_enrollment_status_metrics_recorded() -> Result<()> { let ro_1 = get_bucketed_rollout(slug_3, 10_000); let metrics = TestMetrics::new(); - let client = with_metrics(&metrics, "coenrolling-feature")?; + let client = with_metrics(metrics.clone(), "coenrolling-feature")?; // force the nimbus_id to ensure we end up in the right branch. client.set_nimbus_id(&Uuid::from_str("53baafb3-b800-42ac-878c-c3451e250928")?)?; client.set_experiments_locally(to_local_experiments_string(&[ @@ -1272,14 +1249,9 @@ fn test_enrollment_status_metrics_recorded() -> Result<()> { client.apply_pending_experiments()?; - assert_eq!( - client - .get_metrics_handler() - .get_submit_targeting_context_calls(), - 1u64 - ); + assert_eq!(metrics.get_submit_targeting_context_calls(), 1u64); - let metric_records = client.get_metrics_handler().get_enrollment_statuses(); + let metric_records = metrics.get_enrollment_statuses(); assert_eq!(metric_records.len(), 3); assert_eq!(metric_records[0].slug(), slug_1); @@ -1308,14 +1280,9 @@ fn test_enrollment_status_metrics_recorded() -> Result<()> { ])?)?; client.apply_pending_experiments()?; - assert_eq!( - client - .get_metrics_handler() - .get_submit_targeting_context_calls(), - 2u64 - ); + assert_eq!(metrics.get_submit_targeting_context_calls(), 2u64); - let metric_records = client.get_metrics_handler().get_enrollment_statuses(); + let metric_records = metrics.get_enrollment_statuses(); assert_eq!(metric_records.len(), 6); assert_eq!(metric_records[3].slug(), slug_2); @@ -1356,7 +1323,7 @@ fn test_enrollment_status_metrics_not_recorded_app_name_mismatch() -> Result<()> Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics.clone()), + metrics.clone(), None, None, )?; @@ -1375,13 +1342,9 @@ fn test_enrollment_status_metrics_not_recorded_app_name_mismatch() -> Result<()> client.apply_pending_experiments()?; - assert_eq!( - client - .get_metrics_handler() - .get_submit_targeting_context_calls(), - 1u64 - ); - let metric_records = client.get_metrics_handler().get_enrollment_statuses(); + assert_eq!(metrics.get_submit_targeting_context_calls(), 1u64); + + let metric_records = metrics.get_enrollment_statuses(); assert_eq!(metric_records.len(), 0); Ok(()) @@ -1404,7 +1367,7 @@ fn test_enrollment_status_metrics_not_recorded_channel_mismatch() -> Result<()> Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics.clone()), + metrics.clone(), None, None, )?; @@ -1423,18 +1386,14 @@ fn test_enrollment_status_metrics_not_recorded_channel_mismatch() -> Result<()> client.apply_pending_experiments()?; - assert_eq!( - client - .get_metrics_handler() - .get_submit_targeting_context_calls(), - 1u64 - ); - let metric_records = client.get_metrics_handler().get_enrollment_statuses(); + assert_eq!(metrics.get_submit_targeting_context_calls(), 1u64); + + let metric_records = metrics.get_enrollment_statuses(); assert_eq!(metric_records.len(), 0); Ok(()) } -fn with_metrics(metrics: &TestMetrics, coenrolling_feature: &str) -> Result { +fn with_metrics(metrics: Arc, coenrolling_feature: &str) -> Result { let temp_dir = tempfile::tempdir()?; let app_context = AppContext { @@ -1449,7 +1408,7 @@ fn with_metrics(metrics: &TestMetrics, coenrolling_feature: &str) -> Result Result<()> { let rec_coenr = get_single_feature_experiment(slug_coenr, feature_coenr, json!({})); let metrics = TestMetrics::new(); - let client = with_metrics(&metrics, feature_coenr)?; + let client = with_metrics(metrics.clone(), feature_coenr)?; client.set_experiments_locally(to_local_experiments_string(&[rec_exp, rec_coenr, rec_ro])?)?; client.apply_pending_experiments()?; - let activations = client.get_metrics_handler().get_activations(); + let activations = metrics.get_activations(); assert!(activations.is_empty()); // Assert that all the experiments are active. @@ -1493,17 +1452,17 @@ fn test_feature_activation_events() -> Result<()> { // A feature involved in a rollout doesn't fire activation events. let _ = client.get_feature_config_variables(feature_ro.to_string()); - let activations = client.get_metrics_handler().get_activations(); + let activations = metrics.get_activations(); assert!(activations.is_empty()); // Coenrolled features don't fire activation events. let _ = client.get_feature_config_variables(feature_coenr.to_string()); - let activations = client.get_metrics_handler().get_activations(); + let activations = metrics.get_activations(); assert!(activations.is_empty()); // But features involved in a experiment does! let _ = client.get_feature_config_variables(feature_exp.to_string()); - let activations = client.get_metrics_handler().get_activations(); + let activations = metrics.get_activations(); assert!(!activations.is_empty()); assert_eq!(1, activations.len()); let ev = &activations[0]; @@ -1526,12 +1485,12 @@ fn test_feature_activation_events() -> Result<()> { // Prove to ourselves that activations haven't been sent until feature_config_variables is // called. - let activations = client.get_metrics_handler().get_activations(); + let activations = metrics.get_activations(); assert!(activations.is_empty()); // Now ask for this feature. Recall it's used in both an experiment and a rollout. let _ = client.get_feature_config_variables(feature_exp.to_string()); - let activations = client.get_metrics_handler().get_activations(); + let activations = metrics.get_activations(); assert!(!activations.is_empty()); assert_eq!(1, activations.len()); let ev = &activations[0]; @@ -1560,7 +1519,7 @@ fn test_malformed_feature_events() -> Result<()> { let rec_coenr_2 = get_single_feature_experiment(slug_coenr_2, feature_coenr, json!({})); let metrics = TestMetrics::new(); - let client = with_metrics(&metrics, feature_coenr)?; + let client = with_metrics(metrics.clone(), feature_coenr)?; client.set_experiments_locally(to_local_experiments_string(&[ rec_exp, rec_coenr_1, @@ -1569,14 +1528,14 @@ fn test_malformed_feature_events() -> Result<()> { ])?)?; client.apply_pending_experiments()?; - assert!(client.get_metrics_handler().get_malformeds().is_empty()); + assert!(metrics.get_malformeds().is_empty()); let part = "my-part"; // Experiments! client.record_malformed_feature_config(feature_exp.to_string(), part.to_string()); - let events = client.get_metrics_handler().get_malformeds(); + let events = metrics.get_malformeds(); assert_eq!(1, events.len()); assert_eq!( @@ -1593,7 +1552,7 @@ fn test_malformed_feature_events() -> Result<()> { // Rollouts! client.record_malformed_feature_config(feature_ro.to_string(), part.to_string()); - let events = client.get_metrics_handler().get_malformeds(); + let events = metrics.get_malformeds(); assert_eq!(1, events.len()); assert_eq!( @@ -1610,7 +1569,7 @@ fn test_malformed_feature_events() -> Result<()> { // Coenrolling features! client.record_malformed_feature_config(feature_coenr.to_string(), part.to_string()); - let events = client.get_metrics_handler().get_malformeds(); + let events = metrics.get_malformeds(); assert_eq!(1, events.len()); assert_eq!( @@ -1630,8 +1589,6 @@ fn test_malformed_feature_events() -> Result<()> { #[test] fn test_new_enrollment_in_targeting_mid_run() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { @@ -1645,7 +1602,7 @@ fn test_new_enrollment_in_targeting_mid_run() -> Result<()> { Default::default(), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -1682,8 +1639,6 @@ fn test_new_enrollment_in_targeting_mid_run() -> Result<()> { #[cfg(feature = "stateful")] #[test] fn test_recorded_context_recorded() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { @@ -1698,12 +1653,13 @@ fn test_recorded_context_recorded() -> Result<()> { "app_version": "125.0.0", "other": "stuff", })); + let metrics = TestMetrics::new(); let client = NimbusClient::new( app_context.clone(), Some(recorded_context), Default::default(), temp_dir.path(), - Box::new(metrics), + metrics.clone(), None, None, )?; @@ -1720,20 +1676,13 @@ fn test_recorded_context_recorded() -> Result<()> { let active_experiments = client.get_active_experiments()?; assert_eq!(active_experiments.len(), 1); assert_eq!(client.get_recorded_context().get_record_calls(), 1u64); - assert_eq!( - client - .get_metrics_handler() - .get_submit_targeting_context_calls(), - 1u64 - ); + assert_eq!(metrics.get_submit_targeting_context_calls(), 1u64); Ok(()) } #[test] fn test_recorded_context_event_queries() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { @@ -1757,7 +1706,7 @@ fn test_recorded_context_event_queries() -> Result<()> { Some(recorded_context), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), None, None, )?; @@ -1789,8 +1738,6 @@ fn test_recorded_context_event_queries() -> Result<()> { #[test] fn test_gecko_pref_enrollment() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { @@ -1816,7 +1763,7 @@ fn test_gecko_pref_enrollment() -> Result<()> { Some(recorded_context), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), Some(Box::new(handler)), None, )?; @@ -1869,8 +1816,6 @@ fn test_gecko_pref_enrollment() -> Result<()> { #[test] fn test_gecko_pref_unenrollment() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { @@ -1894,7 +1839,7 @@ fn test_gecko_pref_unenrollment() -> Result<()> { Some(recorded_context), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), Some(Box::new(handler)), None, )?; @@ -1986,8 +1931,6 @@ fn test_gecko_pref_unenrollment() -> Result<()> { #[test] fn test_gecko_pref_unenrollment_reverts() -> Result<()> { - let metrics = TestMetrics::new(); - let temp_dir = tempfile::tempdir()?; let app_context = AppContext { @@ -2011,7 +1954,7 @@ fn test_gecko_pref_unenrollment_reverts() -> Result<()> { Some(recorded_context), Default::default(), temp_dir.path(), - Box::new(metrics), + TestMetrics::new(), Some(Box::new(handler)), None, )?; @@ -2137,7 +2080,7 @@ fn register_previous_gecko_pref_states() -> Result<()> { Some(recorded_context), Default::default(), temp_dir.path(), - Box::new(metrics.clone()), + metrics.clone(), Some(Box::new(handler)), None, )?; @@ -2317,7 +2260,7 @@ fn test_add_prev_gecko_pref_states_for_experiment() -> Result<()> { Some(recorded_context), Default::default(), temp_dir.path(), - Box::new(metrics.clone()), + metrics.clone(), Some(Box::new(handler)), None, )?; diff --git a/components/nimbus/src/tests/stateless/test_cirrus_client.rs b/components/nimbus/src/tests/stateless/test_cirrus_client.rs index 333da1402f..27d52be0b2 100644 --- a/components/nimbus/src/tests/stateless/test_cirrus_client.rs +++ b/components/nimbus/src/tests/stateless/test_cirrus_client.rs @@ -15,7 +15,6 @@ use crate::{ }; fn create_client() -> Result { - let metrics_handler = TestMetrics::new(); CirrusClient::new( to_string(&AppContext { app_id: "test app id".to_string(), @@ -26,7 +25,7 @@ fn create_client() -> Result { custom_targeting_attributes: None, }) .unwrap(), - Box::new(metrics_handler), + TestMetrics::new(), Default::default(), ) } @@ -188,7 +187,7 @@ fn test_sends_metrics_on_enrollment() -> Result<()> { custom_targeting_attributes: None, }) .unwrap(), - Box::new(metrics_handler.clone()), + metrics_handler.clone(), Default::default(), )?; let exp = helpers::get_experiment_with_newtab_feature_branches(); @@ -205,8 +204,7 @@ fn test_sends_metrics_on_enrollment() -> Result<()> { assert_eq!(metric_records[0].branch(), "treatment"); assert_eq!(metric_records[0].user_id(), "test"); - let nimbus_user_id: Option = metrics_handler.get_nimbus_user_id(); - assert_eq!(nimbus_user_id, Some("test".into())); + assert_eq!(metrics_handler.get_nimbus_user_id(), Some("test".into())); Ok(()) } diff --git a/components/nimbus/tests/common.rs b/components/nimbus/tests/common.rs index f9f5ddf462..9d0d129845 100644 --- a/components/nimbus/tests/common.rs +++ b/components/nimbus/tests/common.rs @@ -100,7 +100,7 @@ fn new_test_client_internal( Default::default(), Default::default(), tmp_dir.path(), - Box::new(NoopMetricsHandler), + Arc::new(NoopMetricsHandler), None, Some(NimbusServerSettings { rs_service: Arc::new(remote_settings_service), diff --git a/components/nimbus/tests/test_fs_client.rs b/components/nimbus/tests/test_fs_client.rs index d73dc94539..7087c09b42 100644 --- a/components/nimbus/tests/test_fs_client.rs +++ b/components/nimbus/tests/test_fs_client.rs @@ -44,7 +44,7 @@ fn test_simple() -> Result<()> { Default::default(), Default::default(), tmp_dir.path(), - Box::new(NoopMetricsHandler), + Arc::new(NoopMetricsHandler), None, Some(NimbusServerSettings { rs_service: Arc::new(remote_settings_service), diff --git a/examples/nimbus/src/main.rs b/examples/nimbus/src/main.rs index 7082ccbbc7..c162a012ff 100644 --- a/examples/nimbus/src/main.rs +++ b/examples/nimbus/src/main.rs @@ -204,7 +204,7 @@ fn main() -> Result<()> { Default::default(), Default::default(), db_path, - Box::new(NoopMetricsHandler), + Arc::new(NoopMetricsHandler), None, Some(NimbusServerSettings { rs_service: Arc::new(remote_settings_services),