chore(nimbus-sdk): move enrollment status off events ping

Author: relud

CHANGELOG.md

@@ -12,8 +12,9 @@
 * Added `eval_jexl_debug()` method to `NimbusTargetingHelper` interface for CLI testing and debugging. Evaluates JEXL expressions and returns debug results as JSON. Consumers implementing this interface must add the new method.
 ([#7156](https://github.com/mozilla/application-services/pull/7156))
 ([#31607](https://github.com/mozilla-mobile/firefox-ios/pull/31607))
-* Update Cirrus metrics handler interface for recording enrollment status to specify nimbus user id as separate metric and change method name from `record_enrollment_statuses` to `record_enrollment_statuses_v2`. Consumers implementing this interface must add the new method.
+* Update Cirrus `MetricsHandler` interface for recording enrollment status to specify nimbus user id as separate metric and change method name from `record_enrollment_statuses` to `record_enrollment_statuses_v2`. Consumers implementing this interface must add the new method.
 ([#14280](https://github.com/mozilla/experimenter/pull/14280))
+* Split Nimbus `RecordedContext` interface method `record` into `recordContext` and `submit`, and moved `record_enrollment_statuses` method over from `MetricsHandler` interface. Consumers implementing these interfaces must provide the new methods. ([#14542](https://github.com/mozilla/experimenter/issues/14542))
 * Enable using `PreviousGeckoPrefState` to revert Gecko pref experiments when applicable ([#7157](https://github.com/mozilla/application-services/pull/7157))
 
 ### Error support

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt

@@ -34,7 +34,6 @@ import org.mozilla.experiments.nimbus.internal.AvailableExperiment
 import org.mozilla.experiments.nimbus.internal.EnrolledExperiment
 import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEvent
 import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEventType
-import org.mozilla.experiments.nimbus.internal.EnrollmentStatusExtraDef
 import org.mozilla.experiments.nimbus.internal.FeatureExposureExtraDef
 import org.mozilla.experiments.nimbus.internal.GeckoPrefHandler
 import org.mozilla.experiments.nimbus.internal.GeckoPrefState
@@ -92,21 +91,6 @@ open class Nimbus(
     private val logger = delegate.logger
 
     private val metricsHandler = object : MetricsHandler {
-        override fun recordEnrollmentStatuses(enrollmentStatusExtras: List<EnrollmentStatusExtraDef>) {
-            for (extra in enrollmentStatusExtras) {
-                NimbusEvents.enrollmentStatus.record(
-                    NimbusEvents.EnrollmentStatusExtra(
-                        branch = extra.branch,
-                        slug = extra.slug,
-                        status = extra.status,
-                        reason = extra.reason,
-                        errorString = extra.errorString,
-                        conflictSlug = extra.conflictSlug,
-                    ),
-                )
-            }
-        }
-
         override fun recordFeatureActivation(event: FeatureExposureExtraDef) {
             NimbusEvents.activation.record(
                 NimbusEvents.ActivationExtra(

components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusTests.kt

@@ -38,6 +38,7 @@ import org.mozilla.experiments.nimbus.GleanMetrics.NimbusEvents
 import org.mozilla.experiments.nimbus.GleanMetrics.NimbusHealth
 import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEvent
 import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEventType
+import org.mozilla.experiments.nimbus.internal.EnrollmentStatusExtraDef
 import org.mozilla.experiments.nimbus.internal.GeckoPref
 import org.mozilla.experiments.nimbus.internal.GeckoPrefHandler
 import org.mozilla.experiments.nimbus.internal.GeckoPrefState
@@ -767,10 +768,18 @@ class NimbusTests {
             this.eventQueryValues = eventQueryValues
         }
 
-        override fun record() {
+        override fun recordContext() {
             recorded.add(this.toJson())
         }
 
+        override fun recordEnrollmentStatuses(enrollmentStatusExtras: List<EnrollmentStatusExtraDef>) {
+            //
+        }
+
+        override fun submit() {
+            //
+        }
+
         override fun toJson(): JsonObject {
             val contextToRecord = JSONObject()
             contextToRecord.put("enabled", true)

components/nimbus/src/metrics.rs

@@ -9,9 +9,6 @@ use crate::{enrollment::ExperimentEnrollment, EnrolledFeature, EnrollmentStatus}
 use serde_derive::{Deserialize, Serialize};
 
 pub trait MetricsHandler: Send + Sync {
-    #[cfg(feature = "stateful")]
-    fn record_enrollment_statuses(&self, enrollment_status_extras: Vec<EnrollmentStatusExtraDef>);
-
     #[cfg(not(feature = "stateful"))]
     fn record_enrollment_statuses_v2(
         &self,

components/nimbus/src/nimbus.udl

@@ -84,7 +84,6 @@ enum EnrollmentChangeEventType {
 };
 
 callback interface MetricsHandler {
-    void record_enrollment_statuses(sequence<EnrollmentStatusExtraDef> 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.
     void record_feature_activation(FeatureExposureExtraDef event);
@@ -195,7 +194,11 @@ interface RecordedContext {
 
     void set_event_query_values(record<string, f64> event_query_values);
 
-    void record();
+    void record_context();
+
+    void record_enrollment_statuses(sequence<EnrollmentStatusExtraDef> enrollment_status_extras);
+
+    void submit();
 };
 
 interface NimbusClient {

components/nimbus/src/stateful/nimbus_client.rs

@@ -229,7 +229,6 @@ impl NimbusClient {
             &coenrolling_ids,
             self.gecko_prefs.clone(),
         )?;
-        self.record_enrollment_status_telemetry(state)?;
         Ok(())
     }
 
@@ -288,7 +287,9 @@ impl NimbusClient {
         let existing_experiments: Vec<Experiment> =
             db.get_store(StoreId::Experiments).collect_all(&writer)?;
         let events = self.evolve_experiments(db, &mut writer, &mut state, &existing_experiments)?;
-        self.end_initialize(db, writer, &mut state)?;
+        let res = self.end_initialize(db, writer, &mut state);
+        self.record_enrollment_status_telemetry(&mut state)?;
+        res?;
         Ok(events)
     }
 
@@ -304,7 +305,9 @@ impl NimbusClient {
         let existing_experiments: Vec<Experiment> =
             db.get_store(StoreId::Experiments).collect_all(&writer)?;
         let events = self.evolve_experiments(db, &mut writer, &mut state, &existing_experiments)?;
-        self.end_initialize(db, writer, &mut state)?;
+        let res = self.end_initialize(db, writer, &mut state);
+        self.record_enrollment_status_telemetry(&mut state)?;
+        res?;
         Ok(events)
     }
 
@@ -449,7 +452,7 @@ impl NimbusClient {
             self.gecko_prefs.clone(),
         );
         if let Some(ref recorded_context) = self.recorded_context {
-            recorded_context.record();
+            recorded_context.record_context();
         }
         let coenrolling_feature_ids = self
             .coenrolling_feature_ids
@@ -475,6 +478,7 @@ impl NimbusClient {
         let mut state = self.mutable_state.lock().unwrap();
         self.begin_initialize(db, &mut writer, &mut state)?;
 
+        let should_record_enrollment_status = pending_updates.is_some();
         let res = match pending_updates {
             Some(new_experiments) => {
                 self.update_ta_active_experiments(db, &writer, &mut state)?;
@@ -485,7 +489,12 @@ impl NimbusClient {
         };
 
         // Finish up any cleanup, e.g. copying from database in to memory.
-        self.end_initialize(db, writer, &mut state)?;
+        let end_init_res = self.end_initialize(db, writer, &mut state);
+        if should_record_enrollment_status {
+            self.record_enrollment_status_telemetry(&mut state)?;
+        }
+        end_init_res?;
+
         Ok(res)
     }
 
@@ -1003,16 +1012,19 @@ impl NimbusClient {
             })
             .map(|exp| &*exp.slug)
             .collect::<HashSet<&str>>();
-        self.metrics_handler.record_enrollment_statuses(
-            self.database_cache
-                .get_enrollments()?
-                .into_iter()
-                .filter_map(|e| match experiments.contains(&*e.slug) {
-                    true => Some(e.into()),
-                    false => None,
-                })
-                .collect(),
-        );
+        let statuses = self
+            .database_cache
+            .get_enrollments()?
+            .into_iter()
+            .filter_map(|e| match experiments.contains(&*e.slug) {
+                true => Some(e.into()),
+                false => None,
+            })
+            .collect();
+        if let Some(ref recorded_context) = self.recorded_context {
+            recorded_context.record_enrollment_statuses(statuses);
+            recorded_context.submit();
+        }
         Ok(())
     }
 }

components/nimbus/src/stateful/targeting.rs

@@ -7,6 +7,7 @@ use crate::{
     enrollment::ExperimentEnrollment,
     error::{warn, BehaviorError},
     json::JsonObject,
+    metrics::EnrollmentStatusExtraDef,
     stateful::behavior::{EventQueryType, EventStore},
     NimbusError, NimbusTargetingHelper, Result, TargetingAttributes,
 };
@@ -60,7 +61,17 @@ pub trait RecordedContext: Send + Sync {
     /// Records the context object to Glean
     ///
     /// This method will be implemented in foreign code, i.e: Kotlin, Swift, Python, etc...
-    fn record(&self);
+    fn record_context(&self);
+
+    /// Records the enrollment statuses to Glean
+    ///
+    /// This method will be implemented in foreign code, i.e: Kotlin, Swift, Python, etc...
+    fn record_enrollment_statuses(&self, enrollment_status_extras: Vec<EnrollmentStatusExtraDef>);
+
+    /// Submits the ping for context and enrollment statuses to Glean
+    ///
+    /// This method will be implemented in foreign code, i.e: Kotlin, Swift, Python, etc...
+    fn submit(&self);
 }
 
 impl dyn RecordedContext {

components/nimbus/src/tests/helpers.rs

@@ -67,7 +67,9 @@ impl Default for NimbusTargetingHelper {
 #[derive(Default)]
 struct RecordedContextState {
     context: Map<String, Value>,
-    record_calls: u64,
+    record_context_calls: u64,
+    enrollment_statuses: Vec<EnrollmentStatusExtraDef>,
+    submit_calls: u64,
     event_queries: HashMap<String, String>,
     event_query_values: HashMap<String, f64>,
 }
@@ -86,11 +88,26 @@ impl TestRecordedContext {
         }
     }
 
-    pub fn get_record_calls(&self) -> u64 {
+    pub fn get_record_context_calls(&self) -> u64 {
+        self.state
+            .lock()
+            .expect("could not lock state mutex")
+            .record_context_calls
+    }
+
+    pub fn get_enrollment_statuses(&self) -> Vec<EnrollmentStatusExtraDef> {
         self.state
             .lock()
             .expect("could not lock state mutex")
-            .record_calls
+            .enrollment_statuses
+            .clone()
+    }
+
+    pub fn get_submit_calls(&self) -> u64 {
+        self.state
+            .lock()
+            .expect("could not lock state mutex")
+            .submit_calls
     }
 
     pub fn set_context(&self, context: Value) {
@@ -141,14 +158,26 @@ impl RecordedContext for TestRecordedContext {
             .insert("events".into(), json!(event_query_values));
     }
 
-    fn record(&self) {
+    fn record_context(&self) {
+        let mut state = self.state.lock().expect("could not lock state mutex");
+        state.record_context_calls += 1;
+    }
+
+    #[cfg(feature = "stateful")]
+    fn record_enrollment_statuses(&self, enrollment_status_extras: Vec<EnrollmentStatusExtraDef>) {
+        let mut state = self.state.lock().unwrap();
+        state.enrollment_statuses.extend(enrollment_status_extras);
+    }
+
+    fn submit(&self) {
         let mut state = self.state.lock().expect("could not lock state mutex");
-        state.record_calls += 1;
+        state.submit_calls += 1;
     }
 }
 
 #[derive(Default)]
 struct MetricState {
+    #[cfg(not(feature = "stateful"))]
     enrollment_statuses: Vec<EnrollmentStatusExtraDef>,
     #[cfg(feature = "stateful")]
     activations: Vec<FeatureExposureExtraDef>,
@@ -176,6 +205,7 @@ impl TestMetrics {
         }
     }
 
+    #[cfg(not(feature = "stateful"))]
     pub fn get_enrollment_statuses(&self) -> Vec<EnrollmentStatusExtraDef> {
         self.state.lock().unwrap().enrollment_statuses.clone()
     }
@@ -191,7 +221,6 @@ impl TestMetrics {
     pub fn clear(&self) {
         let mut state = self.state.lock().unwrap();
         state.activations.clear();
-        state.enrollment_statuses.clear();
         state.malformeds.clear();
     }
 
@@ -205,12 +234,6 @@ impl TestMetrics {
 }
 
 impl MetricsHandler for TestMetrics {
-    #[cfg(feature = "stateful")]
-    fn record_enrollment_statuses(&self, enrollment_status_extras: Vec<EnrollmentStatusExtraDef>) {
-        let mut state = self.state.lock().unwrap();
-        state.enrollment_statuses.extend(enrollment_status_extras);
-    }
-
     #[cfg(not(feature = "stateful"))]
     fn record_enrollment_statuses_v2(
         &self,