Bug 1728843 - Add once option to NimbusFeatures.recordExposureEvent r=andreio

Depends on D123763

Differential Revision: https://phabricator.services.mozilla.com/D124381

browser/actors/AboutNewTabChild.jsm

@@ -83,7 +83,7 @@ class AboutNewTabChild extends JSWindowActorChild {
 
         // Note: newtab feature info is currently being loaded in PrefsFeed.jsm,
         // But we're recording exposure events here.
-        NimbusFeatures.newtab.recordExposureEvent();
+        NimbusFeatures.newtab.recordExposureEvent({ once: true });
       }
     }
   }

browser/components/newtab/AboutNewTabService.jsm

@@ -443,7 +443,7 @@ class BaseAboutNewTabService {
      * This is calculated in the same way the default URL is.
      */
 
-    NimbusFeatures.aboutwelcome.recordExposureEvent();
+    NimbusFeatures.aboutwelcome.recordExposureEvent({ once: true });
     if (NimbusFeatures.aboutwelcome.isEnabled({ defaultValue: true })) {
       return ABOUT_WELCOME_URL;
     }

browser/components/newtab/test/browser/browser_aboutwelcome_multistage_experimentAPI.js

@@ -227,7 +227,7 @@ add_task(async function test_multistage_aboutwelcome_experimentAPI() {
     },
   ];
   const sandbox = sinon.createSandbox();
-  NimbusFeatures.aboutwelcome._sendExposureEventOnce = true;
+  NimbusFeatures.aboutwelcome._didSendExposureEvent = false;
   await setAboutWelcomePref(true);
   await ExperimentAPI.ready();
 
@@ -341,9 +341,10 @@ add_task(async function test_multistage_aboutwelcome_experimentAPI() {
     ["div.onboardingContainer"]
   );
 
-  Assert.ok(
+  Assert.equal(
-    ExperimentAPI.recordExposureEvent.called,
+    ExperimentAPI.recordExposureEvent.callCount,
-    "Called for exposure event"
+    1,
+    "Called only once for exposure event"
   );
 
   const scalars = TelemetryTestUtils.getProcessScalars("parent", true, true);

browser/components/urlbar/UrlbarProviderQuickSuggest.jsm

@@ -183,7 +183,7 @@ class ProviderQuickSuggest extends UrlbarProvider {
     if (!this._recordedExposureEvent) {
       this._recordedExposureEvent = true;
       Services.tm.idleDispatchToMainThread(() =>
-        NimbusFeatures.urlbar.recordExposureEvent()
+        NimbusFeatures.urlbar.recordExposureEvent({ once: true })
       );
     }
   }

toolkit/components/nimbus/ExperimentAPI.jsm

@@ -308,8 +308,7 @@ class _ExperimentFeature {
         `No manifest entry for ${featureId}. Please add one to toolkit/components/nimbus/FeatureManifest.js`
       );
     }
-    // Prevent the instance from sending multiple exposure events
+    this._didSendExposureEvent = false;
-    this._sendExposureEventOnce = true;
     this._onRemoteReady = null;
     this._waitForRemote = new Promise(
       resolve => (this._onRemoteReady = resolve)
@@ -514,20 +513,23 @@ class _ExperimentFeature {
     return remoteConfig;
   }
 
-  recordExposureEvent() {
+  recordExposureEvent({ once = false } = {}) {
-    if (this._sendExposureEventOnce) {
+    if (once && this._didSendExposureEvent) {
-      let experimentData = ExperimentAPI.getExperiment({
+      return;
+    }
+
+    let experimentData = ExperimentAPI.getExperiment({
+      featureId: this.featureId,
+    });
+
+    // Exposure only sent if user is enrolled in an experiment
+    if (experimentData) {
+      ExperimentAPI.recordExposureEvent({
         featureId: this.featureId,
+        experimentSlug: experimentData.slug,
+        branchSlug: experimentData.branch?.slug,
       });
-      // Exposure only sent if user is enrolled in an experiment
+      this._didSendExposureEvent = true;
-      if (experimentData) {
-        ExperimentAPI.recordExposureEvent({
-          featureId: this.featureId,
-          experimentSlug: experimentData.slug,
-          branchSlug: experimentData.branch?.slug,
-        });
-        this._sendExposureEventOnce = false;
-      }
     }
   }
 

toolkit/components/nimbus/lib/NimbusFeatures.cpp

@@ -120,15 +120,13 @@ nsresult NimbusFeatures::GetExperimentSlug(const nsACString& aFeatureId,
 
 /**
  * Sends an exposure event for aFeatureId when enrolled in an experiment.
- * By default we only attempt to send once. For some usecases it might be useful
+ * By default attempt to send once per function call. For some usecases it might
- * to send multiple times or retry to send (when for example we are enrolled
+ * be useful to send only once, in which case set the optional aOnce to `true`.
- * after the first call to this function) in which case set the optional
- * aForce to `true`.
  */
 nsresult NimbusFeatures::RecordExposureEvent(const nsACString& aFeatureId,
-                                             const bool aForce) {
+                                             const bool aOnce) {
   nsAutoCString featureName(aFeatureId);
-  if (!sExposureFeatureSet.EnsureInserted(featureName) && !aForce) {
+  if (!sExposureFeatureSet.EnsureInserted(featureName) && aOnce) {
     // We already sent (or tried to send) an exposure ping for this featureId
     return NS_ERROR_ABORT;
   }

toolkit/components/nimbus/lib/NimbusFeatures.h

@@ -37,7 +37,7 @@ class NimbusFeatures {
                             PrefChangedFunc aUserCallback, void* aUserData);
 
   static nsresult RecordExposureEvent(const nsACString& aFeatureId,
-                                      const bool aForce = false);
+                                      const bool aOnce = false);
 };
 
 }  // namespace mozilla

toolkit/components/nimbus/test/browser/browser_experiment_single_feature_enrollment.js

@@ -127,5 +127,5 @@ add_task(async function test_TODO() {
 
   await doExperimentCleanup();
   sandbox.restore();
-  NimbusFeatures.urlbar._sendExposureEventOnce = true;
+  NimbusFeatures.urlbar._didSendExposureEvent = false;
 });

toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp

@@ -29,14 +29,13 @@ TEST_F(NimbusTelemetryFixture, NimbusFeaturesTelemetry) {
       << "Should fail because not enrolled in experiment";
   // Set the experiment info for `foo`
   Preferences::SetCString(prefName.get(), prefValue);
-  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns), NS_ERROR_ABORT)
+  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns), NS_OK)
-      << "Should fail even though enrolled because this is the 2nd call";
+      << "Should work for the 2nd call";
-  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns, true), NS_OK)
+  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns, true), NS_ERROR_ABORT)
-      << "Should work because we set aForce=true";
+      << "Should abort because we've sent exposure and aOnce is true";
   ASSERT_EQ(NimbusFeatures::RecordExposureEvent("bar"_ns), NS_ERROR_UNEXPECTED)
       << "Should fail because we don't have an experiment for bar";
-  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns), NS_ERROR_ABORT)
+
-      << "Should abort because we've already send exposure for this featureId";
   JS::RootedValue eventsSnapshot(cx.GetJSContext());
   GetEventSnapshot(cx.GetJSContext(), &eventsSnapshot);
   ASSERT_TRUE(EventPresent(cx.GetJSContext(), eventsSnapshot, "normandy"_ns,

toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js

@@ -273,16 +273,19 @@ add_task(async function test_record_exposure_event_once() {
     })
   );
 
-  featureInstance.recordExposureEvent();
+  featureInstance.recordExposureEvent({ once: true });
-  featureInstance.recordExposureEvent();
+  featureInstance.recordExposureEvent({ once: true });
-  featureInstance.recordExposureEvent();
+  featureInstance.recordExposureEvent({ once: true });
 
-  Assert.ok(exposureSpy.calledOnce, "Should emit a single exposure event.");
+  Assert.ok(
+    exposureSpy.calledOnce,
+    "Should emit a single exposure event when the once param is true."
+  );
 
   sandbox.restore();
 });
 
-add_task(async function test_prevent_double_exposure() {
+add_task(async function test_allow_multiple_exposure_events() {
   const { sandbox, manager } = await setupForExperimentFeature();
 
   const featureInstance = new ExperimentFeature("foo", FAKE_FEATURE_MANIFEST);
@@ -300,7 +303,11 @@ add_task(async function test_prevent_double_exposure() {
   featureInstance.recordExposureEvent();
 
   Assert.ok(exposureSpy.called, "Should emit exposure event");
-  Assert.ok(exposureSpy.calledOnce, "Should emit a single exposure event");
+  Assert.equal(
+    exposureSpy.callCount,
+    3,
+    "Should emit an exposure event for each function call"
+  );
 
   sandbox.restore();
   await doExperimentCleanup();