diff --git a/browser/actors/AboutNewTabChild.jsm b/browser/actors/AboutNewTabChild.jsm index 1bf1cfb5fa28..d43d9797be92 100644 --- a/browser/actors/AboutNewTabChild.jsm +++ b/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({ once: true }); + NimbusFeatures.newtab.recordExposureEvent(); } } } diff --git a/browser/components/newtab/AboutNewTabService.jsm b/browser/components/newtab/AboutNewTabService.jsm index f3bc40019f8f..d49fe70e851f 100644 --- a/browser/components/newtab/AboutNewTabService.jsm +++ b/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({ once: true }); + NimbusFeatures.aboutwelcome.recordExposureEvent(); if (NimbusFeatures.aboutwelcome.isEnabled({ defaultValue: true })) { return ABOUT_WELCOME_URL; } diff --git a/browser/components/newtab/test/browser/browser_aboutwelcome_multistage_experimentAPI.js b/browser/components/newtab/test/browser/browser_aboutwelcome_multistage_experimentAPI.js index eedcb85bc780..f79188637bd9 100644 --- a/browser/components/newtab/test/browser/browser_aboutwelcome_multistage_experimentAPI.js +++ b/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._didSendExposureEvent = false; + NimbusFeatures.aboutwelcome._sendExposureEventOnce = true; await setAboutWelcomePref(true); await ExperimentAPI.ready(); @@ -341,10 +341,9 @@ add_task(async function test_multistage_aboutwelcome_experimentAPI() { ["div.onboardingContainer"] ); - Assert.equal( - ExperimentAPI.recordExposureEvent.callCount, - 1, - "Called only once for exposure event" + Assert.ok( + ExperimentAPI.recordExposureEvent.called, + "Called for exposure event" ); const scalars = TelemetryTestUtils.getProcessScalars("parent", true, true); diff --git a/browser/components/urlbar/UrlbarProviderQuickSuggest.jsm b/browser/components/urlbar/UrlbarProviderQuickSuggest.jsm index 8a1f360004ec..094a5de0e38f 100644 --- a/browser/components/urlbar/UrlbarProviderQuickSuggest.jsm +++ b/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({ once: true }) + NimbusFeatures.urlbar.recordExposureEvent() ); } } diff --git a/toolkit/components/nimbus/ExperimentAPI.jsm b/toolkit/components/nimbus/ExperimentAPI.jsm index 475996b9f297..8b813094ac4b 100644 --- a/toolkit/components/nimbus/ExperimentAPI.jsm +++ b/toolkit/components/nimbus/ExperimentAPI.jsm @@ -308,7 +308,8 @@ class _ExperimentFeature { `No manifest entry for ${featureId}. Please add one to toolkit/components/nimbus/FeatureManifest.js` ); } - this._didSendExposureEvent = false; + // Prevent the instance from sending multiple exposure events + this._sendExposureEventOnce = true; this._onRemoteReady = null; this._waitForRemote = new Promise( resolve => (this._onRemoteReady = resolve) @@ -513,23 +514,20 @@ class _ExperimentFeature { return remoteConfig; } - recordExposureEvent({ once = false } = {}) { - if (once && this._didSendExposureEvent) { - return; - } - - let experimentData = ExperimentAPI.getExperiment({ - featureId: this.featureId, - }); - - // Exposure only sent if user is enrolled in an experiment - if (experimentData) { - ExperimentAPI.recordExposureEvent({ + recordExposureEvent() { + if (this._sendExposureEventOnce) { + let experimentData = ExperimentAPI.getExperiment({ featureId: this.featureId, - experimentSlug: experimentData.slug, - branchSlug: experimentData.branch?.slug, }); - this._didSendExposureEvent = true; + // Exposure only sent if user is enrolled in an experiment + if (experimentData) { + ExperimentAPI.recordExposureEvent({ + featureId: this.featureId, + experimentSlug: experimentData.slug, + branchSlug: experimentData.branch?.slug, + }); + this._sendExposureEventOnce = false; + } } } diff --git a/toolkit/components/nimbus/lib/NimbusFeatures.cpp b/toolkit/components/nimbus/lib/NimbusFeatures.cpp index ec7d073f5ffa..95d9c089a041 100644 --- a/toolkit/components/nimbus/lib/NimbusFeatures.cpp +++ b/toolkit/components/nimbus/lib/NimbusFeatures.cpp @@ -120,13 +120,15 @@ nsresult NimbusFeatures::GetExperimentSlug(const nsACString& aFeatureId, /** * Sends an exposure event for aFeatureId when enrolled in an experiment. - * By default attempt to send once per function call. For some usecases it might - * be useful to send only once, in which case set the optional aOnce to `true`. + * By default we only attempt to send once. For some usecases it might be useful + * to send multiple times or retry to send (when for example we are enrolled + * after the first call to this function) in which case set the optional + * aForce to `true`. */ nsresult NimbusFeatures::RecordExposureEvent(const nsACString& aFeatureId, - const bool aOnce) { + const bool aForce) { nsAutoCString featureName(aFeatureId); - if (!sExposureFeatureSet.EnsureInserted(featureName) && aOnce) { + if (!sExposureFeatureSet.EnsureInserted(featureName) && !aForce) { // We already sent (or tried to send) an exposure ping for this featureId return NS_ERROR_ABORT; } diff --git a/toolkit/components/nimbus/lib/NimbusFeatures.h b/toolkit/components/nimbus/lib/NimbusFeatures.h index 6ba19eaa38d0..e5d3a9691b0f 100644 --- a/toolkit/components/nimbus/lib/NimbusFeatures.h +++ b/toolkit/components/nimbus/lib/NimbusFeatures.h @@ -37,7 +37,7 @@ class NimbusFeatures { PrefChangedFunc aUserCallback, void* aUserData); static nsresult RecordExposureEvent(const nsACString& aFeatureId, - const bool aOnce = false); + const bool aForce = false); }; } // namespace mozilla diff --git a/toolkit/components/nimbus/test/browser/browser_experiment_single_feature_enrollment.js b/toolkit/components/nimbus/test/browser/browser_experiment_single_feature_enrollment.js index c7a1d41d0b61..370afc8d51d0 100644 --- a/toolkit/components/nimbus/test/browser/browser_experiment_single_feature_enrollment.js +++ b/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._didSendExposureEvent = false; + NimbusFeatures.urlbar._sendExposureEventOnce = true; }); diff --git a/toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp b/toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp index 69c6f3c97bb3..cbb296d5996b 100644 --- a/toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp +++ b/toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp @@ -29,13 +29,14 @@ 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_OK) - << "Should work for the 2nd call"; - ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns, true), NS_ERROR_ABORT) - << "Should abort because we've sent exposure and aOnce is true"; + ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns), NS_ERROR_ABORT) + << "Should fail even though enrolled because this is the 2nd call"; + ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns, true), NS_OK) + << "Should work because we set aForce=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, diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js b/toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js index d76ecc0a2335..29cc9f690abf 100644 --- a/toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js +++ b/toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js @@ -273,19 +273,16 @@ add_task(async function test_record_exposure_event_once() { }) ); - featureInstance.recordExposureEvent({ once: true }); - featureInstance.recordExposureEvent({ once: true }); - featureInstance.recordExposureEvent({ once: true }); + featureInstance.recordExposureEvent(); + featureInstance.recordExposureEvent(); + featureInstance.recordExposureEvent(); - Assert.ok( - exposureSpy.calledOnce, - "Should emit a single exposure event when the once param is true." - ); + Assert.ok(exposureSpy.calledOnce, "Should emit a single exposure event."); sandbox.restore(); }); -add_task(async function test_allow_multiple_exposure_events() { +add_task(async function test_prevent_double_exposure() { const { sandbox, manager } = await setupForExperimentFeature(); const featureInstance = new ExperimentFeature("foo", FAKE_FEATURE_MANIFEST); @@ -303,11 +300,7 @@ add_task(async function test_allow_multiple_exposure_events() { featureInstance.recordExposureEvent(); Assert.ok(exposureSpy.called, "Should emit exposure event"); - Assert.equal( - exposureSpy.callCount, - 3, - "Should emit an exposure event for each function call" - ); + Assert.ok(exposureSpy.calledOnce, "Should emit a single exposure event"); sandbox.restore(); await doExperimentCleanup();