Bug 1690312 - Remove checks for exposure ping and allow it to be sent multiple times r=k88hudson

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

Author: piatra

browser/components/BrowserGlue.jsm

@@ -3849,7 +3849,6 @@ BrowserGlue.prototype = {
       );
       let isFeatureEnabled = ExperimentAPI.getExperiment({
         featureId: "infobar",
-        sendExposurePing: false,
       })?.branch.feature.enabled;
       if (willPrompt) {
         // Prevent the related notification from appearing and

browser/components/newtab/aboutwelcome/AboutWelcomeChild.jsm

@@ -310,8 +310,6 @@ class AboutWelcomeChild extends JSWindowActorChild {
     // about:welcome loads outside of the "FirstStartup" scenario this will likely not be ready
     let experimentData = ExperimentAPI.getExperiment({
       featureId: "aboutwelcome",
-      // Telemetry handled in AboutNewTabService.jsm
-      sendExposurePing: false,
     });
 
     if (experimentData?.slug) {

browser/components/newtab/lib/ASRouter.jsm

@@ -348,10 +348,7 @@ const MessageLoaderUtils = {
 
     let experiments = [];
     for (const featureId of provider.messageGroups) {
-      let experimentData = ExperimentAPI.getExperiment({
-        featureId,
-        sendExposurePing: false,
-      });
+      let experimentData = ExperimentAPI.getExperiment({ featureId });
       // Not enrolled in any experiment for this feature, we can skip
       if (!experimentData) {
         continue;
@@ -364,7 +361,6 @@ const MessageLoaderUtils = {
       if (featureData.enabled) {
         experiments.push({
           forExposureEvent: {
-            sent: experimentData.exposurePingSent,
             experimentSlug: experimentData.slug,
             branchSlug: experimentData.branch.slug,
           },
@@ -1698,12 +1694,13 @@ class _ASRouter {
     // Exposure events only apply to messages that come from the
     // messaging-experiments provider
     if (nonReachMessages.length && nonReachMessages[0].forExposureEvent) {
-      ExperimentAPI.recordExposureEvent(
+      ExperimentAPI.recordExposureEvent({
         // Any message processed by ASRouter will report the exposure event
         // as `cfr`
-        "cfr",
-        nonReachMessages[0].forExposureEvent
-      );
+        featureId: "cfr",
+        // experimentSlug and branchSlug
+        ...nonReachMessages[0].forExposureEvent,
+      });
     }
 
     return this.routeCFRMessage(

browser/components/newtab/test/unit/asrouter/ASRouter.test.js

@@ -1976,11 +1976,10 @@ describe("ASRouter", () => {
       });
 
       assert.calledOnce(global.ExperimentAPI.recordExposureEvent);
-      assert.calledWithExactly(
-        global.ExperimentAPI.recordExposureEvent,
-        "cfr",
-        messages[0].forExposureEvent
-      );
+      assert.calledWithExactly(global.ExperimentAPI.recordExposureEvent, {
+        featureId: "cfr",
+        ...messages[0].forExposureEvent,
+      });
     });
   });
 
@@ -2659,7 +2658,6 @@ describe("ASRouter", () => {
       assert.calledOnce(global.ExperimentAPI.getExperiment);
       assert.calledWithExactly(global.ExperimentAPI.getExperiment, {
         featureId: "asrouter",
-        sendExposurePing: false,
       });
     });
     it("should handle the case of no experiments in the ExperimentAPI", async () => {

toolkit/components/messaging-system/experiments/ExperimentAPI.jsm

@@ -65,6 +65,8 @@ XPCOMUtils.defineLazyPreferenceGetter(
   COLLECTION_ID_PREF,
   COLLECTION_ID_FALLBACK
 );
+const EXPOSURE_EVENT_CATEGORY = "normandy";
+const EXPOSURE_EVENT_METHOD = "expose";
 
 function parseJSON(value) {
   if (value) {
@@ -91,7 +93,7 @@ const ExperimentAPI = {
    *
    * @param {{slug?: string, featureId?: string}} options slug = An experiment identifier
    * or feature = a stable identifier for a type of experiment
-   * @returns {{slug: string, active: bool, exposurePingSent: bool}} A matching experiment if one is found.
+   * @returns {{slug: string, active: bool}} A matching experiment if one is found.
    */
   getExperiment({ slug, featureId, sendExposurePing } = {}) {
     if (!slug && !featureId) {
@@ -113,7 +115,6 @@ const ExperimentAPI = {
       return {
         slug: experimentData.slug,
         active: experimentData.active,
-        exposurePingSent: experimentData.exposurePingSent,
         branch: this.activateBranch({ featureId, sendExposurePing }),
       };
     }
@@ -146,7 +147,6 @@ const ExperimentAPI = {
       return {
         slug: experimentData.slug,
         active: experimentData.active,
-        exposurePingSent: experimentData.exposurePingSent,
         branch: { slug: experimentData.branch.slug },
       };
     }
@@ -159,7 +159,7 @@ const ExperimentAPI = {
    * @param {{slug: string, featureId: string, sendExposurePing: bool}}
    * @returns {Branch | null}
    */
-  activateBranch({ slug, featureId, sendExposurePing = true }) {
+  activateBranch({ slug, featureId, sendExposurePing }) {
     let experiment = null;
     try {
       if (slug) {
@@ -176,9 +176,9 @@ const ExperimentAPI = {
     }
 
     if (sendExposurePing) {
-      this._store._emitExperimentExposure({
+      this.recordExposureEvent({
         experimentSlug: experiment.slug,
-        branchSlug: experiment?.branch?.slug,
+        branchSlug: experiment.branch.slug,
         featureId,
       });
     }
@@ -285,23 +285,22 @@ const ExperimentAPI = {
     return recipe?.branches;
   },
 
-  recordExposureEvent(name, { sent, experimentSlug, branchSlug }) {
-    if (!IS_MAIN_PROCESS) {
-      Cu.reportError("Need to call from Parent process");
-      return false;
-    }
-    if (sent) {
-      return false;
+  recordExposureEvent({ featureId, experimentSlug, branchSlug }) {
+    Services.telemetry.setEventRecordingEnabled(EXPOSURE_EVENT_CATEGORY, true);
+    try {
+      Services.telemetry.recordEvent(
+        EXPOSURE_EVENT_CATEGORY,
+        EXPOSURE_EVENT_METHOD,
+        "feature_study",
+        experimentSlug,
+        {
+          branchSlug,
+          featureId,
+        }
+      );
+    } catch (e) {
+      Cu.reportError(e);
     }
-
-    // Notify listener to record that the ping was sent
-    this._store._emitExperimentExposure({
-      featureId: name,
-      experimentSlug,
-      branchSlug,
-    });
-
-    return true;
   },
 };
 

toolkit/components/messaging-system/experiments/ExperimentManager.jsm

@@ -44,8 +44,6 @@ const TELEMETRY_EXPERIMENT_TYPE_PREFIX = "normandy-";
 // Also included in telemetry
 const DEFAULT_EXPERIMENT_TYPE = "messaging_experiment";
 const STUDIES_OPT_OUT_PREF = "app.shield.optoutstudies.enabled";
-const EXPOSURE_EVENT_CATEGORY = "normandy";
-const EXPOSURE_EVENT_METHOD = "expose";
 
 /**
  * A module for processes Experiment recipes, choosing and storing enrollment state,
@@ -56,7 +54,6 @@ class _ExperimentManager {
     this.id = id;
     this.store = store || new ExperimentStore();
     this.sessions = new Map();
-    this._onExposureEvent = this._onExposureEvent.bind(this);
     Services.prefs.addObserver(STUDIES_OPT_OUT_PREF, this);
   }
 
@@ -88,7 +85,6 @@ class _ExperimentManager {
    */
   async onStartup() {
     await this.store.init();
-    this.store.on("exposure", this._onExposureEvent);
     const restoredExperiments = this.store.getAllActive();
 
     for (const experiment of restoredExperiments) {
@@ -228,8 +224,6 @@ class _ExperimentManager {
       slug,
       branch,
       active: true,
-      // Sent first time feature value is used
-      exposurePingSent: false,
       enrollmentId,
       experimentType,
       source,
@@ -343,32 +337,6 @@ class _ExperimentManager {
     });
   }
 
-  async _onExposureEvent(event, experimentData) {
-    await this.store.ready();
-    this.store.updateExperiment(experimentData.experimentSlug, {
-      exposurePingSent: true,
-    });
-    // featureId is not validated and might be rejected by recordEvent if not
-    // properly defined in Events.yaml. Saving experiment state regardless of
-    // the result, no use retrying.
-    try {
-      Services.telemetry.recordEvent(
-        EXPOSURE_EVENT_CATEGORY,
-        EXPOSURE_EVENT_METHOD,
-        "feature_study",
-        experimentData.experimentSlug,
-        {
-          branchSlug: experimentData.branchSlug,
-          featureId: experimentData.featureId,
-        }
-      );
-    } catch (e) {
-      Cu.reportError(e);
-    }
-
-    log.debug(`Experiment exposure: ${experimentData.experimentSlug}`);
-  }
-
   /**
    * Sets Telemetry when activating an experiment.
    *

toolkit/components/messaging-system/experiments/ExperimentStore.jsm

@@ -142,13 +142,6 @@ class ExperimentStore extends SharedDataMap {
     this.off(`featureUpdate:${featureId}`, callback);
   }
 
-  /**
-   * @param {{featureId: string, experimentSlug: string, branchSlug: string}} experimentData
-   */
-  _emitExperimentExposure(experimentData) {
-    this.emit("exposure", experimentData);
-  }
-
   /**
    * @param {Enrollment} experiment
    */

toolkit/components/messaging-system/test/unit/test_ExperimentAPI.js

@@ -9,6 +9,9 @@ const { ExperimentFakes } = ChromeUtils.import(
 const { TestUtils } = ChromeUtils.import(
   "resource://testing-common/TestUtils.jsm"
 );
+const { TelemetryTestUtils } = ChromeUtils.import(
+  "resource://testing-common/TelemetryTestUtils.jsm"
+);
 const COLLECTION_ID_PREF = "messaging-system.rsexperimentloader.collection_id";
 
 /**
@@ -64,6 +67,7 @@ add_task(async function test_getExperimentMetaData() {
   const sandbox = sinon.createSandbox();
   const manager = ExperimentFakes.manager();
   const expected = ExperimentFakes.experiment("foo");
+  let exposureStub = sandbox.stub(ExperimentAPI, "recordExposureEvent");
 
   await manager.onStartup();
   sandbox.stub(ExperimentAPI, "_store").get(() => manager.store);
@@ -84,11 +88,14 @@ add_task(async function test_getExperimentMetaData() {
     "Should have the slug prop"
   );
 
+  Assert.ok(exposureStub.notCalled, "Not called for this method");
+
   sandbox.restore();
 });
 
 add_task(function test_getExperimentMetaData_safe() {
   const sandbox = sinon.createSandbox();
+  let exposureStub = sandbox.stub(ExperimentAPI, "recordExposureEvent");
 
   sandbox.stub(ExperimentAPI._store, "get").throws();
   sandbox.stub(ExperimentAPI._store, "getExperimentForFeature").throws();
@@ -114,6 +121,8 @@ add_task(function test_getExperimentMetaData_safe() {
     "Sanity check"
   );
 
+  Assert.ok(exposureStub.notCalled, "Not called for this feature");
+
   sandbox.restore();
 });
 
@@ -131,6 +140,7 @@ add_task(async function test_getExperiment_feature() {
   await manager.onStartup();
 
   sandbox.stub(ExperimentAPI, "_store").get(() => ExperimentFakes.childStore());
+  let exposureStub = sandbox.stub(ExperimentAPI, "recordExposureEvent");
 
   manager.store.addExperiment(expected);
 
@@ -146,6 +156,12 @@ add_task(async function test_getExperiment_feature() {
     "should return an experiment by featureId"
   );
 
+  Assert.ok(exposureStub.notCalled, "Not called by default");
+
+  ExperimentAPI.getExperiment({ featureId: "cfr", sendExposurePing: true });
+
+  Assert.ok(exposureStub.calledOnce, "Called explicitly.");
+
   sandbox.restore();
 });
 
@@ -385,15 +401,24 @@ add_task(async function test_activateBranch_activationEvent() {
   await store.init();
   store.addExperiment(experiment);
   // Adding stub later because `addExperiment` emits update events
-  const stub = sandbox.stub(store, "emit");
-  // Call activateBranch to trigger an activation event
+  const stub = sandbox.stub(ExperimentAPI, "recordExposureEvent");
   ExperimentAPI.activateBranch({ featureId: "green" });
+  Assert.equal(
+    stub.callCount,
+    0,
+    "Exposure is not sent by default by activateBranch"
+  );
+
+  ExperimentAPI.activateBranch({ featureId: "green", sendExposurePing: true });
 
   Assert.equal(stub.callCount, 1, "Called by doing activateBranch");
-  Assert.equal(stub.firstCall.args[0], "exposure", "Has correct event name");
-  Assert.equal(
-    stub.firstCall.args[1].experimentSlug,
-    experiment.slug,
+  Assert.deepEqual(
+    stub.firstCall.args[0],
+    {
+      featureId: experiment.branch.feature.featureId,
+      experimentSlug: experiment.slug,
+      branchSlug: experiment.branch.slug,
+    },
     "Has correct payload"
   );
   sandbox.restore();
@@ -442,8 +467,26 @@ add_task(async function test_activateBranch_noActivationEvent() {
   // Adding stub later because `addExperiment` emits update events
   const stub = sandbox.stub(store, "emit");
   // Call activateBranch to trigger an activation event
-  ExperimentAPI.activateBranch({ featureId: "green", sendExposurePing: false });
+  ExperimentAPI.activateBranch({ featureId: "green" });
 
   Assert.equal(stub.callCount, 0, "Not called: sendExposurePing is false");
   sandbox.restore();
 });
+
+add_task(function test_recordExposureEvent() {
+  Services.telemetry.clearScalars();
+
+  ExperimentAPI.recordExposureEvent({
+    featureId: "aboutwelcome",
+    experimentSlug: "my-xpcshell-experiment",
+    branchSlug: "treatment-a",
+  });
+
+  const scalars = TelemetryTestUtils.getProcessScalars("parent", true, true);
+  TelemetryTestUtils.assertKeyedScalar(
+    scalars,
+    "telemetry.event_counts",
+    "normandy#expose#feature_study",
+    1
+  );
+});