From 5096499a649457da0c2733033f18776e4c5ac127 Mon Sep 17 00:00:00 2001 From: Michael Cooper Date: Tue, 26 Nov 2019 23:43:42 +0000 Subject: [PATCH] Bug 1594035 - Don't send `null` enrollmentIds for Normandy telemetry r=rhelmer Differential Revision: https://phabricator.services.mozilla.com/D54834 --HG-- extra : moz-landing-system : lando --- .../normandy/actions/AddonRollbackAction.jsm | 10 +++++-- .../normandy/actions/AddonRolloutAction.jsm | 18 +++++++++---- .../actions/BranchedAddonStudyAction.jsm | 26 ++++++++++++------- .../actions/PreferenceRollbackAction.jsm | 12 +++++++-- .../actions/PreferenceRolloutAction.jsm | 8 +++--- .../components/normandy/lib/AddonStudies.jsm | 3 ++- .../normandy/lib/PreferenceExperiments.jsm | 16 ++++++++---- .../normandy/lib/PreferenceRollouts.jsm | 6 +++-- .../normandy/lib/TelemetryEvents.jsm | 9 +++++++ 9 files changed, 79 insertions(+), 29 deletions(-) diff --git a/toolkit/components/normandy/actions/AddonRollbackAction.jsm b/toolkit/components/normandy/actions/AddonRollbackAction.jsm index e4031df643af..a48c161c5c92 100644 --- a/toolkit/components/normandy/actions/AddonRollbackAction.jsm +++ b/toolkit/components/normandy/actions/AddonRollbackAction.jsm @@ -51,7 +51,12 @@ class AddonRollbackAction extends BaseAction { "unenrollFailed", "addon_rollback", rolloutSlug, - { reason: "uninstall-failed", enrollmentId: rollout.enrollmentId } + { + reason: "uninstall-failed", + enrollmentId: + rollout.enrollmentId || + TelemetryEvents.NO_ENROLLMENT_ID_MARKER, + } ); throw err; } @@ -65,7 +70,8 @@ class AddonRollbackAction extends BaseAction { TelemetryEvents.sendEvent("unenroll", "addon_rollback", rolloutSlug, { reason: "rollback", - enrollmentId: rollout.enrollmentId, + enrollmentId: + rollout.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); TelemetryEnvironment.setExperimentInactive(rolloutSlug); break; diff --git a/toolkit/components/normandy/actions/AddonRolloutAction.jsm b/toolkit/components/normandy/actions/AddonRolloutAction.jsm index 7e0276a476c9..e9c1ebb63ea5 100644 --- a/toolkit/components/normandy/actions/AddonRolloutAction.jsm +++ b/toolkit/components/normandy/actions/AddonRolloutAction.jsm @@ -118,7 +118,9 @@ class AddonRolloutAction extends BaseAction { const conflictError = createError("conflict", { addonId: conflictingRollout.addonId, conflictingSlug: conflictingRollout.slug, - enrollmentId: conflictingRollout.enrollmentId, + enrollmentId: + conflictingRollout.enrollmentId || + TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); this.reportError(conflictError, "enrollFailed"); throw conflictError; @@ -129,7 +131,10 @@ class AddonRolloutAction extends BaseAction { if (existingRollout && existingRollout.addonId !== install.addon.id) { installDeferred.reject( - createError("addon-id-changed", { enrollmentId }) + createError("addon-id-changed", { + enrollmentId: + enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, + }) ); return false; // cancel the upgrade, the add-on ID has changed } @@ -139,7 +144,10 @@ class AddonRolloutAction extends BaseAction { Services.vc.compare(existingAddon.version, install.addon.version) > 0 ) { installDeferred.reject( - createError("upgrade-required", { enrollmentId }) + createError("upgrade-required", { + enrollmentId: + enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, + }) ); return false; // cancel the installation, must be an upgrade } @@ -168,7 +176,7 @@ class AddonRolloutAction extends BaseAction { recipeId: recipe.id, state: AddonRollouts.STATE_ACTIVE, slug, - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, ...details, }); } @@ -211,7 +219,7 @@ class AddonRolloutAction extends BaseAction { TelemetryEvents.sendEvent(eventName, "addon_rollout", slug, { addonId: installedId, addonVersion: installedVersion, - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } diff --git a/toolkit/components/normandy/actions/BranchedAddonStudyAction.jsm b/toolkit/components/normandy/actions/BranchedAddonStudyAction.jsm index 67ae3db215d4..414079e12a79 100644 --- a/toolkit/components/normandy/actions/BranchedAddonStudyAction.jsm +++ b/toolkit/components/normandy/actions/BranchedAddonStudyAction.jsm @@ -353,7 +353,7 @@ class BranchedAddonStudyAction extends BaseStudyAction { addonId: AddonStudies.NO_ADDON_MARKER, addonVersion: AddonStudies.NO_ADDON_MARKER, branch: branch.slug, - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } else { const extensionDetails = await NormandyApi.fetchExtensionDetails( @@ -435,13 +435,13 @@ class BranchedAddonStudyAction extends BaseStudyAction { addonId: installedId, addonVersion: installedVersion, branch: branch.slug, - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } TelemetryEnvironment.setExperimentActive(slug, branch.slug, { type: "normandy-addonstudy", - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } @@ -474,7 +474,8 @@ class BranchedAddonStudyAction extends BaseStudyAction { error = new AddonStudyUpdateError(slug, { branch: branch.slug, reason: "addon-id-mismatch", - enrollmentId: study.enrollmentId, + enrollmentId: + study.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } @@ -486,7 +487,8 @@ class BranchedAddonStudyAction extends BaseStudyAction { error = new AddonStudyUpdateError(slug, { branch: branch.slug, reason: "no-downgrade", - enrollmentId: study.enrollmentId, + enrollmentId: + study.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } else if (versionCompare === 0) { return; // Unchanged, do nothing @@ -507,7 +509,8 @@ class BranchedAddonStudyAction extends BaseStudyAction { new AddonStudyUpdateError(slug, { branch: branch.slug, reason: "addon-does-not-exist", - enrollmentId: study.enrollmentId, + enrollmentId: + study.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }) ); return false; // cancel the installation, must upgrade an existing add-on @@ -516,7 +519,8 @@ class BranchedAddonStudyAction extends BaseStudyAction { new AddonStudyUpdateError(slug, { branch: branch.slug, reason: "metadata-mismatch", - enrollmentId: study.enrollmentId, + enrollmentId: + study.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }) ); return false; // cancel the installation, server metadata do not match downloaded add-on @@ -556,7 +560,10 @@ class BranchedAddonStudyAction extends BaseStudyAction { onFailedInstall, errorClass: AddonStudyUpdateError, reportError: this.reportUpdateError, - errorExtra: { enrollmentId: study.enrollmentId }, + errorExtra: { + enrollmentId: + study.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, + }, }); // All done, report success to Telemetry @@ -564,7 +571,8 @@ class BranchedAddonStudyAction extends BaseStudyAction { addonId: installedId, addonVersion: installedVersion, branch: branch.slug, - enrollmentId: study.enrollmentId, + enrollmentId: + study.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } diff --git a/toolkit/components/normandy/actions/PreferenceRollbackAction.jsm b/toolkit/components/normandy/actions/PreferenceRollbackAction.jsm index 0df18e0078ee..f87f7b8321e1 100644 --- a/toolkit/components/normandy/actions/PreferenceRollbackAction.jsm +++ b/toolkit/components/normandy/actions/PreferenceRollbackAction.jsm @@ -61,7 +61,11 @@ class PreferenceRollbackAction extends BaseAction { "unenroll", "preference_rollback", rolloutSlug, - { reason: "rollback", enrollmentId: rollout.enrollmentId } + { + reason: "rollback", + enrollmentId: + rollout.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, + } ); TelemetryEnvironment.setExperimentInactive(rolloutSlug); break; @@ -76,7 +80,11 @@ class PreferenceRollbackAction extends BaseAction { "unenrollFailed", "preference_rollback", rolloutSlug, - { reason: "graduated", enrollmentId: rollout.enrollmentId } + { + reason: "graduated", + enrollmentId: + rollout.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, + } ); throw new Error( `Cannot rollback already graduated rollout ${rolloutSlug}` diff --git a/toolkit/components/normandy/actions/PreferenceRolloutAction.jsm b/toolkit/components/normandy/actions/PreferenceRolloutAction.jsm index 4110dfcd3b50..b5b92890eb9d 100644 --- a/toolkit/components/normandy/actions/PreferenceRolloutAction.jsm +++ b/toolkit/components/normandy/actions/PreferenceRolloutAction.jsm @@ -81,7 +81,9 @@ class PreferenceRolloutAction extends BaseAction { await PreferenceRollouts.update(newRollout); TelemetryEvents.sendEvent("update", "preference_rollout", args.slug, { previousState: existingRollout.state, - enrollmentId: existingRollout.enrollmentId, + enrollmentId: + existingRollout.enrollmentId || + TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); switch (existingRollout.state) { @@ -143,10 +145,10 @@ class PreferenceRolloutAction extends BaseAction { this.log.debug(`Enrolled in preference rollout ${args.slug}`); TelemetryEnvironment.setExperimentActive(args.slug, newRollout.state, { type: "normandy-prefrollout", - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); TelemetryEvents.sendEvent("enroll", "preference_rollout", args.slug, { - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } } diff --git a/toolkit/components/normandy/lib/AddonStudies.jsm b/toolkit/components/normandy/lib/AddonStudies.jsm index 334f7d5162e4..8b847a4bbeb4 100644 --- a/toolkit/components/normandy/lib/AddonStudies.jsm +++ b/toolkit/components/normandy/lib/AddonStudies.jsm @@ -352,7 +352,8 @@ var AddonStudies = { addonVersion: study.addonVersion || AddonStudies.NO_ADDON_MARKER, reason, branch: study.branch, - enrollmentId: study.enrollmentId, + enrollmentId: + study.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); TelemetryEnvironment.setExperimentInactive(study.slug); diff --git a/toolkit/components/normandy/lib/PreferenceExperiments.jsm b/toolkit/components/normandy/lib/PreferenceExperiments.jsm index 0e8b727adb67..27cacdc44424 100644 --- a/toolkit/components/normandy/lib/PreferenceExperiments.jsm +++ b/toolkit/components/normandy/lib/PreferenceExperiments.jsm @@ -275,7 +275,8 @@ var PreferenceExperiments = { experiment.branch, { type: EXPERIMENT_TYPE_PREFIX + experiment.experimentType, - enrollmentId: experiment.enrollmentId, + enrollmentId: + experiment.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, } ); @@ -551,12 +552,12 @@ var PreferenceExperiments = { TelemetryEnvironment.setExperimentActive(slug, branch, { type: EXPERIMENT_TYPE_PREFIX + experimentType, - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); TelemetryEvents.sendEvent("enroll", "preference_study", slug, { experimentType, branch, - enrollmentId, + enrollmentId: enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); await this.saveStartupPrefs(); @@ -712,7 +713,11 @@ var PreferenceExperiments = { "unenrollFailed", "preference_study", experimentSlug, - { reason: "already-unenrolled", enrollmentId: experiment.enrollmentId } + { + reason: "already-unenrolled", + enrollmentId: + experiment.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, + } ); throw new Error( `Cannot stop preference experiment "${experimentSlug}" because it is already expired` @@ -764,7 +769,8 @@ var PreferenceExperiments = { didResetValue: resetValue ? "true" : "false", branch: experiment.branch, reason, - enrollmentId: experiment.enrollmentId, + enrollmentId: + experiment.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); await this.saveStartupPrefs(); }, diff --git a/toolkit/components/normandy/lib/PreferenceRollouts.jsm b/toolkit/components/normandy/lib/PreferenceRollouts.jsm index c3da3834a130..1546ac535e4d 100644 --- a/toolkit/components/normandy/lib/PreferenceRollouts.jsm +++ b/toolkit/components/normandy/lib/PreferenceRollouts.jsm @@ -156,7 +156,8 @@ var PreferenceRollouts = { "preference_rollout", rollout.slug, { - enrollmentId: rollout.enrollmentId, + enrollmentId: + rollout.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, } ); } @@ -172,7 +173,8 @@ var PreferenceRollouts = { for (const rollout of await this.getAllActive()) { TelemetryEnvironment.setExperimentActive(rollout.slug, rollout.state, { type: "normandy-prefrollout", - enrollmentId: rollout.enrollmentId, + enrollmentId: + rollout.enrollmentId || TelemetryEvents.NO_ENROLLMENT_ID_MARKER, }); } }, diff --git a/toolkit/components/normandy/lib/TelemetryEvents.jsm b/toolkit/components/normandy/lib/TelemetryEvents.jsm index 801fc57d306a..5b13bdf0c3c6 100644 --- a/toolkit/components/normandy/lib/TelemetryEvents.jsm +++ b/toolkit/components/normandy/lib/TelemetryEvents.jsm @@ -10,11 +10,20 @@ var EXPORTED_SYMBOLS = ["TelemetryEvents"]; const TELEMETRY_CATEGORY = "normandy"; const TelemetryEvents = { + NO_ENROLLMENT_ID_MARKER: "__NO_ENROLLMENT_ID__", + init() { Services.telemetry.setEventRecordingEnabled(TELEMETRY_CATEGORY, true); }, sendEvent(method, object, value, extra) { + for (const val of Object.values(extra)) { + if (val == null) { + throw new Error( + "Extra parameters in telemetry events must not be null" + ); + } + } Services.telemetry.recordEvent( TELEMETRY_CATEGORY, method,