From a2367c38e34b9b5982ff17e5d24af68fb362a6fe Mon Sep 17 00:00:00 2001 From: Kate Hudson Date: Tue, 7 Sep 2021 13:49:51 +0000 Subject: [PATCH] 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 | 2 +- .../components/newtab/AboutNewTabService.jsm | 2 +- ...r_aboutwelcome_multistage_experimentAPI.js | 9 +++--- .../urlbar/UrlbarProviderQuickSuggest.jsm | 2 +- toolkit/components/nimbus/ExperimentAPI.jsm | 30 ++++++++++--------- .../components/nimbus/lib/NimbusFeatures.cpp | 10 +++---- .../components/nimbus/lib/NimbusFeatures.h | 2 +- ...er_experiment_single_feature_enrollment.js | 2 +- .../gtest/NimbusFeatures_RecordExposure.cpp | 11 ++++--- .../test_ExperimentAPI_ExperimentFeature.js | 19 ++++++++---- 10 files changed, 48 insertions(+), 41 deletions(-) diff --git a/browser/actors/AboutNewTabChild.jsm b/browser/actors/AboutNewTabChild.jsm index d43d9797be92..1bf1cfb5fa28 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(); + NimbusFeatures.newtab.recordExposureEvent({ once: true }); } } } diff --git a/browser/components/newtab/AboutNewTabService.jsm b/browser/components/newtab/AboutNewTabService.jsm index d49fe70e851f..f3bc40019f8f 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(); + NimbusFeatures.aboutwelcome.recordExposureEvent({ once: true }); 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 f79188637bd9..eedcb85bc780 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._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( - ExperimentAPI.recordExposureEvent.called, - "Called for exposure event" + Assert.equal( + ExperimentAPI.recordExposureEvent.callCount, + 1, + "Called only once 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 e55a4bdcb1ec..a128bda03927 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() + NimbusFeatures.urlbar.recordExposureEvent({ once: true }) ); } } diff --git a/toolkit/components/nimbus/ExperimentAPI.jsm b/toolkit/components/nimbus/ExperimentAPI.jsm index 8b813094ac4b..475996b9f297 100644 --- a/toolkit/components/nimbus/ExperimentAPI.jsm +++ b/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._sendExposureEventOnce = true; + this._didSendExposureEvent = false; this._onRemoteReady = null; this._waitForRemote = new Promise( resolve => (this._onRemoteReady = resolve) @@ -514,20 +513,23 @@ class _ExperimentFeature { return remoteConfig; } - recordExposureEvent() { - if (this._sendExposureEventOnce) { - let experimentData = ExperimentAPI.getExperiment({ + 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({ featureId: this.featureId, + experimentSlug: experimentData.slug, + branchSlug: experimentData.branch?.slug, }); - // 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; - } + this._didSendExposureEvent = true; } } diff --git a/toolkit/components/nimbus/lib/NimbusFeatures.cpp b/toolkit/components/nimbus/lib/NimbusFeatures.cpp index 95d9c089a041..ec7d073f5ffa 100644 --- a/toolkit/components/nimbus/lib/NimbusFeatures.cpp +++ b/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 - * 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`. + * 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`. */ 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; } diff --git a/toolkit/components/nimbus/lib/NimbusFeatures.h b/toolkit/components/nimbus/lib/NimbusFeatures.h index e5d3a9691b0f..6ba19eaa38d0 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 aForce = false); + const bool aOnce = 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 370afc8d51d0..c7a1d41d0b61 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._sendExposureEventOnce = true; + NimbusFeatures.urlbar._didSendExposureEvent = false; }); diff --git a/toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp b/toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp index cbb296d5996b..69c6f3c97bb3 100644 --- a/toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp +++ b/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) - << "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("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("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 29cc9f690abf..d76ecc0a2335 100644 --- a/toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js +++ b/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(); - featureInstance.recordExposureEvent(); + featureInstance.recordExposureEvent({ once: true }); + featureInstance.recordExposureEvent({ once: true }); + 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();