diff --git a/CHANGELOG.md b/CHANGELOG.md index 297b586cf7..0ccf9a0b83 100644 --- a/CHANGELOG.md +++ b/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)) +* Move `nimbus_events.enrollment_status` to new `nimbus-targeting-context` ping, and add Nimbus `MetricsHandler` interface method `submit_targeting_context` to submit the ping. Consumers implementing this interface must add the new method. ([#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 diff --git a/automation/build_ios_artifacts.sh b/automation/build_ios_artifacts.sh index f114a3c58c..10e9612c9a 100755 --- a/automation/build_ios_artifacts.sh +++ b/automation/build_ios_artifacts.sh @@ -13,6 +13,7 @@ export PROJECT=MozillaRustComponentsWrapper -o ./megazords/ios-rust/Sources/MozillaRustComponentsWrapper/Generated/Glean \ "${SOURCE_ROOT}"/components/ads-client/metrics.yaml \ "${SOURCE_ROOT}"/components/nimbus/metrics.yaml \ + "${SOURCE_ROOT}"/components/nimbus/pings.yaml \ "${SOURCE_ROOT}"/components/logins/metrics.yaml \ "${SOURCE_ROOT}"/components/sync_manager/metrics.yaml \ "${SOURCE_ROOT}"/components/sync_manager/pings.yaml diff --git a/components/nimbus/android/build.gradle b/components/nimbus/android/build.gradle index 74b3aec478..915abb43f5 100644 --- a/components/nimbus/android/build.gradle +++ b/components/nimbus/android/build.gradle @@ -27,7 +27,7 @@ apply from: "$appServicesRootDir/publish.gradle" ext { gleanNamespace = "mozilla.telemetry.glean" - gleanYamlFiles = ["${project.projectDir}/../metrics.yaml"] + gleanYamlFiles = ["${project.projectDir}/../metrics.yaml", "${project.projectDir}/../pings.yaml"] if (gradle.hasProperty("mozconfig")) { gleanPythonEnvDir = gradle.mozconfig.substs.GRADLE_GLEAN_PARSER_VENV } diff --git a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt index 966e2503d8..df315ae97f 100644 --- a/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt +++ b/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt @@ -29,6 +29,7 @@ import mozilla.telemetry.glean.Glean import org.json.JSONObject import org.mozilla.experiments.nimbus.GleanMetrics.NimbusEvents import org.mozilla.experiments.nimbus.GleanMetrics.NimbusHealth +import org.mozilla.experiments.nimbus.GleanMetrics.Pings import org.mozilla.experiments.nimbus.internal.AppContext import org.mozilla.experiments.nimbus.internal.AvailableExperiment import org.mozilla.experiments.nimbus.internal.EnrolledExperiment @@ -137,6 +138,10 @@ open class Nimbus( ), ) } + + override fun submitTargetingContext() { + Pings.nimbusTargetingContext.submit() + } } private val nimbusClient: NimbusClientInterface diff --git a/components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusTests.kt b/components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusTests.kt index 4ca1555107..571d13b144 100644 --- a/components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusTests.kt +++ b/components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusTests.kt @@ -19,6 +19,7 @@ import mozilla.telemetry.glean.Glean import mozilla.telemetry.glean.config.Configuration import mozilla.telemetry.glean.net.HttpStatus import mozilla.telemetry.glean.net.PingUploader +import mozilla.telemetry.glean.private.RecordedEvent import mozilla.telemetry.glean.testing.GleanTestRule import org.json.JSONObject import org.junit.Assert.assertEquals @@ -36,6 +37,7 @@ import org.mockito.Mockito import org.mockito.Mockito.`when` import org.mozilla.experiments.nimbus.GleanMetrics.NimbusEvents import org.mozilla.experiments.nimbus.GleanMetrics.NimbusHealth +import org.mozilla.experiments.nimbus.GleanMetrics.Pings import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEvent import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEventType import org.mozilla.experiments.nimbus.internal.GeckoPref @@ -723,17 +725,21 @@ class NimbusTests { return testExperimentsJsonString(appInfo, packageName) } + var enrollmentStatusEvents: List? = null + val gleanJob = Pings.nimbusTargetingContext.testBeforeNextSubmit { + enrollmentStatusEvents = NimbusEvents.enrollmentStatus.testGetValue() + } + val job = nimbus.applyLocalExperiments(::getString) runBlocking { job.join() + gleanJob.join() } assertEquals(1, nimbus.getAvailableExperiments().size) - assertNotNull("Event must have a value", NimbusEvents.enrollmentStatus.testGetValue()) - val enrollmentStatusEvents = NimbusEvents.enrollmentStatus.testGetValue()!! - assertEquals("Event count must match", enrollmentStatusEvents.count(), 1) + assertEquals("Event count must match", 1, enrollmentStatusEvents?.size) - val enrolledExtra = enrollmentStatusEvents[0].extra!! + val enrolledExtra = enrollmentStatusEvents!!.single().extra!! assertEquals("branch must match", "test-branch", enrolledExtra["branch"]) assertEquals("slug must match", "test-experiment", enrolledExtra["slug"]) assertEquals("status must match", "Enrolled", enrolledExtra["status"]) diff --git a/components/nimbus/metrics.yaml b/components/nimbus/metrics.yaml index 13fb20aad8..a7f0f1994e 100644 --- a/components/nimbus/metrics.yaml +++ b/components/nimbus/metrics.yaml @@ -249,6 +249,7 @@ nimbus_events: bugs: - https://mozilla-hub.atlassian.net/browse/EXP-3827 - https://bugzilla.mozilla.org/show_bug.cgi?id=1996105 + - https://bugzilla.mozilla.org/show_bug.cgi?id=1996120 data_reviews: - https://github.com/mozilla/application-services/pull/5857#issuecomment-1749722071 data_sensitivity: @@ -258,6 +259,8 @@ nimbus_events: - project-nimbus@mozilla.com expires: never disabled: true + send_in_pings: + - nimbus-targeting-context is_ready: type: event description: > diff --git a/components/nimbus/pings.yaml b/components/nimbus/pings.yaml new file mode 100644 index 0000000000..f692a5944c --- /dev/null +++ b/components/nimbus/pings.yaml @@ -0,0 +1,20 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +--- +$schema: moz://mozilla.org/schemas/glean/pings/2-0-0 + +nimbus-targeting-context: + description: | + This ping is submitted by Nimbus each time the enrollment workflow has completed. + include_client_id: true + send_if_empty: true + bugs: + - https://mozilla-hub.atlassian.net/browse/EXP-6084 + data_reviews: + - https://github.com/mozilla/application-services/pull/7225 + notification_emails: + - beth@mozilla.com + - dthorn@mozilla.com + - project-nimbus@mozilla.com diff --git a/components/nimbus/src/metrics.rs b/components/nimbus/src/metrics.rs index 39fff19036..ac34099867 100644 --- a/components/nimbus/src/metrics.rs +++ b/components/nimbus/src/metrics.rs @@ -27,6 +27,9 @@ pub trait MetricsHandler: Send + Sync { #[cfg(feature = "stateful")] fn record_malformed_feature_config(&self, event: MalformedFeatureConfigExtraDef); + + #[cfg(feature = "stateful")] + fn submit_targeting_context(&self); } #[derive(Serialize, Deserialize, Clone)] diff --git a/components/nimbus/src/nimbus.udl b/components/nimbus/src/nimbus.udl index c45dc9c574..292b261fe2 100644 --- a/components/nimbus/src/nimbus.udl +++ b/components/nimbus/src/nimbus.udl @@ -92,6 +92,8 @@ callback interface MetricsHandler { void record_feature_exposure(FeatureExposureExtraDef event); void record_malformed_feature_config(MalformedFeatureConfigExtraDef event); + + void submit_targeting_context(); }; dictionary EnrollmentStatusExtraDef { diff --git a/components/nimbus/src/stateful/nimbus_client.rs b/components/nimbus/src/stateful/nimbus_client.rs index c1129edf9c..b9e61206f6 100644 --- a/components/nimbus/src/stateful/nimbus_client.rs +++ b/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 = 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 = 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) } @@ -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,11 @@ 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) } @@ -1013,6 +1021,7 @@ impl NimbusClient { }) .collect(), ); + self.metrics_handler.submit_targeting_context(); Ok(()) } } diff --git a/components/nimbus/src/tests/helpers.rs b/components/nimbus/src/tests/helpers.rs index 90176ec6ed..6429e25b74 100644 --- a/components/nimbus/src/tests/helpers.rs +++ b/components/nimbus/src/tests/helpers.rs @@ -156,6 +156,8 @@ struct MetricState { exposures: Vec, #[cfg(feature = "stateful")] malformeds: Vec, + #[cfg(feature = "stateful")] + submit_targeting_context_calls: u64, #[cfg(not(feature = "stateful"))] nimbus_user_id: Option, } @@ -202,6 +204,10 @@ impl TestMetrics { pub fn get_malformeds(&self) -> Vec { self.state.lock().unwrap().malformeds.clone() } + + pub fn get_submit_targeting_context_calls(&self) -> u64 { + self.state.lock().unwrap().submit_targeting_context_calls + } } impl MetricsHandler for TestMetrics { @@ -239,6 +245,12 @@ impl MetricsHandler for TestMetrics { let mut state = self.state.lock().unwrap(); state.malformeds.push(event); } + + #[cfg(feature = "stateful")] + fn submit_targeting_context(&self) { + let mut state = self.state.lock().unwrap(); + state.submit_targeting_context_calls += 1; + } } #[cfg(feature = "stateful")] diff --git a/components/nimbus/src/tests/stateful/test_nimbus.rs b/components/nimbus/src/tests/stateful/test_nimbus.rs index c30ae98125..c322e9f014 100644 --- a/components/nimbus/src/tests/stateful/test_nimbus.rs +++ b/components/nimbus/src/tests/stateful/test_nimbus.rs @@ -1296,6 +1296,13 @@ fn test_enrollment_status_metrics_recorded() -> 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!(metric_records.len(), 3); @@ -1325,6 +1332,13 @@ fn test_enrollment_status_metrics_recorded() -> Result<()> { ])?)?; client.apply_pending_experiments()?; + assert_eq!( + client + .get_metrics_handler() + .get_submit_targeting_context_calls(), + 2u64 + ); + let metric_records = client.get_metrics_handler().get_enrollment_statuses(); assert_eq!(metric_records.len(), 6); @@ -1386,6 +1400,12 @@ 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!(metric_records.len(), 0); @@ -1429,6 +1449,12 @@ 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!(metric_records.len(), 0); Ok(()) @@ -1723,6 +1749,12 @@ 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 + ); Ok(()) } diff --git a/components/nimbus/tests/common/mod.rs b/components/nimbus/tests/common/mod.rs index 2fb39314d8..526b789c41 100644 --- a/components/nimbus/tests/common/mod.rs +++ b/components/nimbus/tests/common/mod.rs @@ -41,6 +41,11 @@ impl MetricsHandler for NoopMetricsHandler { fn record_malformed_feature_config(&self, _event: MalformedFeatureConfigExtraDef) { // do nothing } + + #[cfg(feature = "stateful")] + fn submit_targeting_context(&self) { + // do nothing + } } #[allow(dead_code)] // work around https://github.com/rust-lang/rust/issues/46379 diff --git a/examples/nimbus/src/main.rs b/examples/nimbus/src/main.rs index e63319cd8d..e0bcc8f48e 100644 --- a/examples/nimbus/src/main.rs +++ b/examples/nimbus/src/main.rs @@ -125,6 +125,10 @@ fn main() -> Result<()> { fn record_malformed_feature_config(&self, _event: MalformedFeatureConfigExtraDef) { // do nothing } + + fn submit_targeting_context(&self) { + // do nothing + } } // We set the logging level to be `warn` here, meaning that only diff --git a/taskcluster/scripts/build-and-test-swift.py b/taskcluster/scripts/build-and-test-swift.py index 9e9b281225..9042d710ca 100755 --- a/taskcluster/scripts/build-and-test-swift.py +++ b/taskcluster/scripts/build-and-test-swift.py @@ -101,12 +101,19 @@ def generate_glean_metrics(args): ) out_dir = args.out_dir / "all" / "Generated" / "Metrics" focus_out_dir = args.out_dir / "focus" / "Generated" / "Metrics" - focus_glean_files = map(str, [ROOT_DIR / "components/nimbus/metrics.yaml"]) + focus_glean_files = map( + str, + [ + ROOT_DIR / "components/nimbus/metrics.yaml", + ROOT_DIR / "components/nimbus/pings.yaml", + ] + ) firefox_glean_files = map( str, [ ROOT_DIR / "components/ads-client/metrics.yaml", ROOT_DIR / "components/nimbus/metrics.yaml", + ROOT_DIR / "components/nimbus/pings.yaml", ROOT_DIR / "components/logins/metrics.yaml", ROOT_DIR / "components/sync_manager/metrics.yaml", ROOT_DIR / "components/sync_manager/pings.yaml",