From e05ab643bfa0ad2219c7185cd2cd132a54b35a91 Mon Sep 17 00:00:00 2001 From: Kate Hudson Date: Wed, 3 Mar 2021 19:54:52 +0000 Subject: [PATCH] Bug 1656568 - Use a specific event object for nimbus events r=andreio Differential Revision: https://phabricator.services.mozilla.com/D106619 --- .../browser_asrouter_experimentsAPILoader.js | 2 +- toolkit/components/nimbus/ExperimentAPI.jsm | 5 +- .../nimbus/lib/ExperimentManager.jsm | 21 ++- .../nimbus/test/browser/browser.ini | 4 +- .../test/browser/browser_nimbus_telemetry.js | 153 ++++++++++++++++++ .../nimbus/test/unit/test_ExperimentAPI.js | 30 ++-- .../unit/test_ExperimentManager_unenroll.js | 4 +- .../content/about-studies/about-studies.js | 8 +- .../test/browser/browser_about_studies.js | 7 +- toolkit/components/telemetry/Events.yaml | 14 +- 10 files changed, 197 insertions(+), 51 deletions(-) create mode 100644 toolkit/components/nimbus/test/browser/browser_nimbus_telemetry.js diff --git a/browser/components/newtab/test/browser/browser_asrouter_experimentsAPILoader.js b/browser/components/newtab/test/browser/browser_asrouter_experimentsAPILoader.js index eaf9931b7970..ad7d7ed57a07 100644 --- a/browser/components/newtab/test/browser/browser_asrouter_experimentsAPILoader.js +++ b/browser/components/newtab/test/browser/browser_asrouter_experimentsAPILoader.js @@ -239,7 +239,7 @@ add_task(async function test_exposure_ping() { TelemetryTestUtils.assertKeyedScalar( scalars, "telemetry.event_counts", - "normandy#expose#feature_study", + "normandy#expose#nimbus_experiment", 1 ); diff --git a/toolkit/components/nimbus/ExperimentAPI.jsm b/toolkit/components/nimbus/ExperimentAPI.jsm index c675bfa668c4..9eeffb47b378 100644 --- a/toolkit/components/nimbus/ExperimentAPI.jsm +++ b/toolkit/components/nimbus/ExperimentAPI.jsm @@ -74,6 +74,7 @@ XPCOMUtils.defineLazyPreferenceGetter( ); const EXPOSURE_EVENT_CATEGORY = "normandy"; const EXPOSURE_EVENT_METHOD = "expose"; +const EXPOSURE_EVENT_OBJECT = "nimbus_experiment"; function parseJSON(value) { if (value) { @@ -122,7 +123,7 @@ const ExperimentAPI = { return { slug: experimentData.slug, active: experimentData.active, - branch: this.activateBranch({ featureId, sendExposureEvent }), + branch: this.activateBranch({ slug, featureId, sendExposureEvent }), }; } @@ -298,7 +299,7 @@ const ExperimentAPI = { Services.telemetry.recordEvent( EXPOSURE_EVENT_CATEGORY, EXPOSURE_EVENT_METHOD, - "feature_study", + EXPOSURE_EVENT_OBJECT, experimentSlug, { branchSlug, diff --git a/toolkit/components/nimbus/lib/ExperimentManager.jsm b/toolkit/components/nimbus/lib/ExperimentManager.jsm index 571d887181cf..2ce9c08a5cef 100644 --- a/toolkit/components/nimbus/lib/ExperimentManager.jsm +++ b/toolkit/components/nimbus/lib/ExperimentManager.jsm @@ -35,13 +35,10 @@ XPCOMUtils.defineLazyGetter(this, "log", () => { return new Logger("ExperimentManager"); }); -// This is included with event telemetry e.g. "enroll" -// TODO: Add a new type called "messaging_study" -const EVENT_TELEMETRY_STUDY_TYPE = "preference_study"; -// This is used by Telemetry.setExperimentActive -const TELEMETRY_EXPERIMENT_TYPE_PREFIX = "normandy-"; -// Also included in telemetry -const DEFAULT_EXPERIMENT_TYPE = "messaging_experiment"; +const TELEMETRY_EVENT_OBJECT = "nimbus_experiment"; +const TELEMETRY_EXPERIMENT_ACTIVE_PREFIX = "nimbus-"; +const TELEMETRY_DEFAULT_EXPERIMENT_TYPE = "nimbus"; + const STUDIES_OPT_OUT_PREF = "app.shield.optoutstudies.enabled"; /** @@ -189,7 +186,7 @@ class _ExperimentManager { { slug, branches, - experimentType = DEFAULT_EXPERIMENT_TYPE, + experimentType = TELEMETRY_DEFAULT_EXPERIMENT_TYPE, userFacingName, userFacingDescription, }, @@ -288,7 +285,7 @@ class _ExperimentManager { this.store.updateExperiment(slug, { active: false }); TelemetryEnvironment.setExperimentInactive(slug); - TelemetryEvents.sendEvent("unenroll", EVENT_TELEMETRY_STUDY_TYPE, slug, { + TelemetryEvents.sendEvent("unenroll", TELEMETRY_EVENT_OBJECT, slug, { reason, branch: experiment.branch.slug, enrollmentId: @@ -318,7 +315,7 @@ class _ExperimentManager { * @param {string} reason */ sendFailureTelemetry(eventName, slug, reason) { - TelemetryEvents.sendEvent(eventName, EVENT_TELEMETRY_STUDY_TYPE, slug, { + TelemetryEvents.sendEvent(eventName, TELEMETRY_EVENT_OBJECT, slug, { reason, }); } @@ -328,7 +325,7 @@ class _ExperimentManager { * @param {Enrollment} experiment */ sendEnrollmentTelemetry({ slug, branch, experimentType, enrollmentId }) { - TelemetryEvents.sendEvent("enroll", EVENT_TELEMETRY_STUDY_TYPE, slug, { + TelemetryEvents.sendEvent("enroll", TELEMETRY_EVENT_OBJECT, slug, { experimentType, branch: branch.slug, enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, @@ -346,7 +343,7 @@ class _ExperimentManager { experiment.slug, experiment.branch.slug, { - type: `${TELEMETRY_EXPERIMENT_TYPE_PREFIX}${experiment.experimentType}`, + type: `${TELEMETRY_EXPERIMENT_ACTIVE_PREFIX}${experiment.experimentType}`, enrollmentId: experiment.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, } diff --git a/toolkit/components/nimbus/test/browser/browser.ini b/toolkit/components/nimbus/test/browser/browser.ini index 4ed022c1a5ea..60ffc60ad558 100644 --- a/toolkit/components/nimbus/test/browser/browser.ini +++ b/toolkit/components/nimbus/test/browser/browser.ini @@ -3,5 +3,7 @@ [browser_experimentstore_load.js] [browser_remotesettings_experiment_enroll.js] [browser_experiment_evaluate_jexl.js] -tags = remote-settings [browser_remotesettingsexperimentloader_init.js] +[browser_nimbus_telemetry.js] +tags = remote-settings + diff --git a/toolkit/components/nimbus/test/browser/browser_nimbus_telemetry.js b/toolkit/components/nimbus/test/browser/browser_nimbus_telemetry.js new file mode 100644 index 000000000000..da4f1eef284a --- /dev/null +++ b/toolkit/components/nimbus/test/browser/browser_nimbus_telemetry.js @@ -0,0 +1,153 @@ +"use strict"; + +const { RemoteSettings } = ChromeUtils.import( + "resource://services-settings/remote-settings.js" +); +const { RemoteSettingsExperimentLoader } = ChromeUtils.import( + "resource://nimbus/lib/RemoteSettingsExperimentLoader.jsm" +); +const { ExperimentAPI, ExperimentFeature } = ChromeUtils.import( + "resource://nimbus/ExperimentAPI.jsm" +); +const { ExperimentManager } = ChromeUtils.import( + "resource://nimbus/lib/ExperimentManager.jsm" +); +const { ExperimentFakes } = ChromeUtils.import( + "resource://testing-common/NimbusTestUtils.jsm" +); +const { TelemetryTestUtils } = ChromeUtils.import( + "resource://testing-common/TelemetryTestUtils.jsm" +); + +const TELEMETRY_CATEGORY = "normandy"; +const TELEMETRY_OBJECT = "nimbus_experiment"; +// Included with active experiment information +const EXPERIMENT_TYPE = "nimbus"; +const EVENT_FILTER = { category: TELEMETRY_CATEGORY }; + +let rsClient; + +add_task(async function setup() { + await SpecialPowers.pushPrefEnv({ + set: [ + ["messaging-system.log", "all"], + ["app.shield.optoutstudies.enabled", true], + ], + }); + rsClient = RemoteSettings("nimbus-desktop-experiments"); + + registerCleanupFunction(async () => { + await SpecialPowers.popPrefEnv(); + await rsClient.db.clear(); + }); +}); + +// TODO Use utilities being added +async function addExperiment() { + const recipe = ExperimentFakes.recipe("foo" + Date.now(), { + bucketConfig: { + start: 0, + count: 10000, + total: 10000, + namespace: "mochitest", + randomizationUnit: "normandy_id", + }, + }); + await rsClient.db.importChanges({}, 42, [recipe], { + clear: true, + }); + + let waitForExperimentEnrollment = ExperimentFakes.waitForExperimentUpdate( + ExperimentAPI, + { slug: recipe.slug } + ); + + RemoteSettingsExperimentLoader.updateRecipes("mochitest"); + + await waitForExperimentEnrollment; + + const cleanup = async () => { + let waitForExperimentUnenrollment = ExperimentFakes.waitForExperimentUpdate( + ExperimentAPI, + { slug: recipe.slug } + ); + ExperimentManager.unenroll(recipe.slug, "mochitest-cleanup"); + + await waitForExperimentUnenrollment; + }; + return { recipe, cleanup }; +} + +add_task(async function test_experiment_enroll_unenroll_Telemetry() { + Services.telemetry.clearEvents(); + const { recipe, cleanup } = await addExperiment(); + let experiment = ExperimentAPI.getExperiment({ + slug: recipe.slug, + }); + + Assert.ok(experiment.branch, "Should be enrolled in the experiment"); + TelemetryTestUtils.assertEvents( + [ + { + method: "enroll", + object: TELEMETRY_OBJECT, + value: experiment.slug, + extra: { + experimentType: EXPERIMENT_TYPE, + branch: experiment.branch.slug, + enrollmentId: experiment.enrollmentId, + }, + }, + ], + EVENT_FILTER + ); + + await cleanup(); + + TelemetryTestUtils.assertEvents( + [ + { + method: "unenroll", + object: TELEMETRY_OBJECT, + value: experiment.slug, + extra: { + reason: "mochitest-cleanup", + branch: experiment.branch.slug, + enrollmentId: experiment.enrollmentId, + }, + }, + ], + EVENT_FILTER + ); +}); + +add_task(async function test_experiment_expose_Telemetry() { + const { recipe, cleanup } = await addExperiment(); + + let experiment = ExperimentAPI.getExperiment({ + slug: recipe.slug, + }); + + const { featureId } = experiment.branch.feature; + const feature = new ExperimentFeature(featureId); + + Services.telemetry.clearEvents(); + feature.getValue({ sendExposureEvent: true }); + + TelemetryTestUtils.assertEvents( + [ + { + method: "expose", + object: TELEMETRY_OBJECT, + value: experiment.slug, + extra: { + branchSlug: experiment.branch.slug, + featureId, + }, + }, + ], + EVENT_FILTER + ); + + await cleanup(); +}); diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentAPI.js b/toolkit/components/nimbus/test/unit/test_ExperimentAPI.js index 9ac325436af2..03cb5674904d 100644 --- a/toolkit/components/nimbus/test/unit/test_ExperimentAPI.js +++ b/toolkit/components/nimbus/test/unit/test_ExperimentAPI.js @@ -40,6 +40,12 @@ add_task(async function test_getExperiment_fromChild_slug() { "should return an experiment by slug" ); + Assert.deepEqual( + ExperimentAPI.getExperiment({ slug: "foo" }).branch, + expected.branch, + "should return the right branch by slug" + ); + sandbox.restore(); }); @@ -156,6 +162,12 @@ add_task(async function test_getExperiment_feature() { "should return an experiment by featureId" ); + Assert.deepEqual( + ExperimentAPI.getExperiment({ featureId: "cfr" }).branch, + expected.branch, + "should return the right branch by featureId" + ); + Assert.ok(exposureStub.notCalled, "Not called by default"); ExperimentAPI.getExperiment({ featureId: "cfr", sendExposureEvent: true }); @@ -472,21 +484,3 @@ add_task(async function test_activateBranch_noActivationEvent() { Assert.equal(stub.callCount, 0, "Not called: sendExposureEvent 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 - ); -}); diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentManager_unenroll.js b/toolkit/components/nimbus/test/unit/test_ExperimentManager_unenroll.js index 68593774b6f5..c3eb1e253a78 100644 --- a/toolkit/components/nimbus/test/unit/test_ExperimentManager_unenroll.js +++ b/toolkit/components/nimbus/test/unit/test_ExperimentManager_unenroll.js @@ -63,7 +63,7 @@ add_task(async function test_unenroll_opt_out() { TelemetryEvents.sendEvent.firstCall.args, [ "unenroll", - "preference_study", + "nimbus_experiment", experiment.slug, { reason: "studies-opt-out", @@ -108,7 +108,7 @@ add_task(async function test_send_unenroll_event() { TelemetryEvents.sendEvent.firstCall.args, [ "unenroll", - "preference_study", // This needs to be updated eventually + "nimbus_experiment", "foo", // slug { reason: "some-reason", diff --git a/toolkit/components/normandy/content/about-studies/about-studies.js b/toolkit/components/normandy/content/about-studies/about-studies.js index c6b344c27df6..4a4834159c80 100644 --- a/toolkit/components/normandy/content/about-studies/about-studies.js +++ b/toolkit/components/normandy/content/about-studies/about-studies.js @@ -208,7 +208,7 @@ class StudyList extends React.Component { translations, }); } - if (study.type === "messaging_experiment") { + if (study.type === "nimbus") { return r(MessagingSystemListItem, { key: study.slug, study, @@ -234,7 +234,7 @@ class StudyList extends React.Component { translations, }); } - if (study.experimentType === "messaging_experiment") { + if (study.experimentType === "nimbus") { return r(MessagingSystemListItem, { key: study.slug, study, @@ -273,11 +273,11 @@ class MessagingSystemListItem extends React.Component { const { study, translations } = this.props; const userFacingName = study.userFacingName || study.slug; const userFacingDescription = - study.userFacingDescription || "Messaging System experiment."; + study.userFacingDescription || "Nimbus experiment."; return r( "li", { - className: classnames("study messaging-system", { + className: classnames("study nimbus", { disabled: !study.active, }), "data-study-slug": study.slug, // used to identify this row in tests diff --git a/toolkit/components/normandy/test/browser/browser_about_studies.js b/toolkit/components/normandy/test/browser/browser_about_studies.js index d2f36812c31c..90f4ccb2e2af 100644 --- a/toolkit/components/normandy/test/browser/browser_about_studies.js +++ b/toolkit/components/normandy/test/browser/browser_about_studies.js @@ -629,7 +629,7 @@ decorate_task( } ); -add_task(async function test_messaging_system_about_studies() { +add_task(async function test_nimbus_about_studies() { const recipe = ExperimentFakes.recipe("about-studies-foo"); await ExperimentManager.enroll(recipe); await BrowserTestUtils.withNewTab( @@ -637,8 +637,7 @@ add_task(async function test_messaging_system_about_studies() { async browser => { const name = await SpecialPowers.spawn(browser, [], async () => { await ContentTaskUtils.waitForCondition( - () => - content.document.querySelector(".messaging-system .remove-button"), + () => content.document.querySelector(".nimbus .remove-button"), "waiting for page/experiment to load" ); return content.document.querySelector(".study-name").innerText; @@ -657,7 +656,7 @@ add_task(async function test_messaging_system_about_studies() { async browser => { const name = await SpecialPowers.spawn(browser, [], async () => { await ContentTaskUtils.waitForCondition( - () => content.document.querySelector(".messaging-system.disabled"), + () => content.document.querySelector(".nimbus.disabled"), "waiting for experiment to become disabled" ); return content.document.querySelector(".study-name").innerText; diff --git a/toolkit/components/telemetry/Events.yaml b/toolkit/components/telemetry/Events.yaml index 917562395d60..1e8dd1657ecb 100644 --- a/toolkit/components/telemetry/Events.yaml +++ b/toolkit/components/telemetry/Events.yaml @@ -573,12 +573,12 @@ urlbar: normandy: enroll: - objects: ["preference_study", "addon_study", "preference_rollout", "addon_rollout"] + objects: ["preference_study", "addon_study", "preference_rollout", "addon_rollout", "nimbus_experiment"] description: > Sent when applying a Normandy recipe of the above types has succeeded. extra_keys: experimentType: > - For preference_study recipes, the type of experiment this is ("exp" or "exp-highpop"). + For preference_study and nimbus_experiment recipes, the type of experiment this is ("exp" or "exp-highpop"). branch: > The slug of the branch that was chosen for this client. addonId: For addon_study recipes, the ID of the addon that was installed. @@ -595,7 +595,7 @@ normandy: enroll_failed: methods: ["enrollFailed"] - objects: ["addon_study", "preference_rollout", "preference_study", "addon_rollout"] + objects: ["addon_study", "preference_rollout", "preference_study", "addon_rollout", "nimbus_experiment"] description: > Sent when applying a Normandy recipe of the above types has failed. extra_keys: @@ -620,7 +620,7 @@ normandy: expiry_version: never update: - objects: ["addon_study", "preference_rollout", "addon_rollout"] + objects: ["addon_study", "preference_rollout", "addon_rollout", "nimbus_experiment"] description: > This event is fired when a client detects that a recipe of the ahove types has changed on the server, and the new version of the @@ -665,7 +665,7 @@ normandy: expiry_version: never unenroll: - objects: ["preference_study", "addon_study", "preference_rollback", "addon_rollback"] + objects: ["preference_study", "addon_study", "preference_rollback", "addon_rollback", "nimbus_experiment"] description: > Sent when a Normandy recipe of certain types "ends". N.B. For preference_rollback, this is fired when the recipe is fired (the @@ -692,7 +692,7 @@ normandy: methods: ["unenrollFailed"] description: > Sent when unenrolling a user fails (see the unenroll event). - objects: ["preference_rollback", "preference_study", "addon_rollback"] + objects: ["preference_rollback", "preference_study", "addon_rollback", "nimbus_experiment"] extra_keys: reason: A code describing the reason the unenroll failed. enrollmentId: A unique ID for this enrollment that will be included in all related Telemetry. @@ -727,7 +727,7 @@ normandy: expose: objects: [ - "feature_study", + "nimbus_experiment", ] methods: ["expose"] release_channel_collection: opt-out