Bug 1530508: convert reportRecipe to take the recipe, not just its ID r=mythmon
authorEthan Glasser-Camp <ethan@betacantrips.com>
Wed, 06 Mar 2019 21:11:52 +0000
changeset 520758 fa9146245803ec512e886835accb896c3d80f981
parent 520757 cb745568d893c43e5ca7cb7dcda7d4c8464bd62b
child 520759 0d1b3156e7c82db4faa0fabed419fa9a1be29006
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmythmon
bugs1530508
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1530508: convert reportRecipe to take the recipe, not just its ID r=mythmon This will make it easier to report recipe freshness. Differential Revision: https://phabricator.services.mozilla.com/D22016
toolkit/components/normandy/actions/BaseAction.jsm
toolkit/components/normandy/lib/ActionsManager.jsm
toolkit/components/normandy/lib/Uptake.jsm
toolkit/components/normandy/test/browser/browser_ActionsManager.js
toolkit/components/normandy/test/browser/browser_BaseAction.js
toolkit/components/normandy/test/browser/browser_actions_ConsoleLogAction.js
toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js
--- a/toolkit/components/normandy/actions/BaseAction.jsm
+++ b/toolkit/components/normandy/actions/BaseAction.jsm
@@ -108,38 +108,38 @@ class BaseAction {
   async runRecipe(recipe) {
     this._ensurePreExecution();
 
     if (this.state === BaseAction.STATE_FINALIZED) {
       throw new Error("Action has already been finalized");
     }
 
     if (this.state !== BaseAction.STATE_READY) {
-      Uptake.reportRecipe(recipe.id, Uptake.RECIPE_ACTION_DISABLED);
+      Uptake.reportRecipe(recipe, Uptake.RECIPE_ACTION_DISABLED);
       this.log.warn(`Skipping recipe ${recipe.name} because ${this.name} was disabled during preExecution.`);
       return;
     }
 
     let [valid, validatedArguments] = JsonSchemaValidator.validateAndParseParameters(recipe.arguments, this.schema);
     if (!valid) {
       Cu.reportError(new Error(`Arguments do not match schema. arguments: ${JSON.stringify(recipe.arguments)}. schema: ${JSON.stringify(this.schema)}`));
-      Uptake.reportRecipe(recipe.id, Uptake.RECIPE_EXECUTION_ERROR);
+      Uptake.reportRecipe(recipe, Uptake.RECIPE_EXECUTION_ERROR);
       return;
     }
 
     recipe.arguments = validatedArguments;
 
     let status = Uptake.RECIPE_SUCCESS;
     try {
       await this._run(recipe);
     } catch (err) {
       Cu.reportError(err);
       status = Uptake.RECIPE_EXECUTION_ERROR;
     }
-    Uptake.reportRecipe(recipe.id, status);
+    Uptake.reportRecipe(recipe, status);
   }
 
   /**
    * Action specific recipe behavior must be implemented here. It
    * will be executed once for reach recipe, being passed the recipe
    * as a parameter.
    */
   async _run(recipe) {
--- a/toolkit/components/normandy/lib/ActionsManager.jsm
+++ b/toolkit/components/normandy/lib/ActionsManager.jsm
@@ -112,23 +112,23 @@ class ActionsManager {
           await manager.runAsyncCallback("action", recipe);
           status = Uptake.RECIPE_SUCCESS;
         } catch (e) {
           e.message = `Could not execute recipe ${recipe.name}: ${e.message}`;
           Cu.reportError(e);
           status = Uptake.RECIPE_EXECUTION_ERROR;
         }
       }
-      Uptake.reportRecipe(recipe.id, status);
+      Uptake.reportRecipe(recipe, status);
     } else {
       log.error(
         `Could not execute recipe ${recipe.name}:`,
         `Action ${recipe.action} is either missing or invalid.`
       );
-      Uptake.reportRecipe(recipe.id, Uptake.RECIPE_INVALID_ACTION);
+      Uptake.reportRecipe(recipe, Uptake.RECIPE_INVALID_ACTION);
     }
   }
 
   async finalize() {
     if (this.finalized) {
       throw new Error("ActionsManager has already been finalized");
     }
     this.finalized = true;
--- a/toolkit/components/normandy/lib/Uptake.jsm
+++ b/toolkit/components/normandy/lib/Uptake.jsm
@@ -30,16 +30,16 @@ var Uptake = {
   RUNNER_NETWORK_ERROR: UptakeTelemetry.STATUS.NETWORK_ERROR,
   RUNNER_SERVER_ERROR: UptakeTelemetry.STATUS.SERVER_ERROR,
   RUNNER_SUCCESS: UptakeTelemetry.STATUS.SUCCESS,
 
   reportRunner(status) {
     UptakeTelemetry.report(COMPONENT, status, { source: `${COMPONENT}/runner` });
   },
 
-  reportRecipe(recipeId, status) {
-    UptakeTelemetry.report(COMPONENT, status, { source: `${COMPONENT}/recipe/${recipeId}` });
+  reportRecipe(recipe, status) {
+    UptakeTelemetry.report(COMPONENT, status, { source: `${COMPONENT}/recipe/${recipe.id}` });
   },
 
   reportAction(actionName, status) {
     UptakeTelemetry.report(COMPONENT, status, { source: `${COMPONENT}/action/${actionName}` });
   },
 };
--- a/toolkit/components/normandy/test/browser/browser_ActionsManager.js
+++ b/toolkit/components/normandy/test/browser/browser_ActionsManager.js
@@ -80,17 +80,17 @@ decorate_task(
       [["ActionsManager"]],
       "ActionsManager should remove holds on the sandbox managers during finalize.",
     );
 
     Assert.deepEqual(reportActionStub.args, [
       ["test-remote-action-used", Uptake.ACTION_SUCCESS],
       ["test-remote-action-unused", Uptake.ACTION_SUCCESS],
     ]);
-    Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_SUCCESS]]);
+    Assert.deepEqual(reportRecipeStub.args, [[recipe, Uptake.RECIPE_SUCCESS]]);
   },
 );
 
 // Test life cycle for remote action that fails in pre-step
 decorate_task(
   withStub(Uptake, "reportAction"),
   withStub(Uptake, "reportRecipe"),
   async function(reportActionStub, reportRecipeStub) {
@@ -123,17 +123,17 @@ decorate_task(
       sandboxManagerBroken.removeHold.args,
       [["ActionsManager"]],
       "sandbox holds should still be removed after a failure",
     );
 
     Assert.deepEqual(reportActionStub.args, [
       ["test-remote-action-broken", Uptake.ACTION_PRE_EXECUTION_ERROR],
     ]);
-    Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_ACTION_DISABLED]]);
+    Assert.deepEqual(reportRecipeStub.args, [[recipe, Uptake.RECIPE_ACTION_DISABLED]]);
   },
 );
 
 // Test life cycle for remote action that fails on a recipe-step
 decorate_task(
   withStub(Uptake, "reportAction"),
   withStub(Uptake, "reportRecipe"),
   async function(reportActionStub, reportRecipeStub) {
@@ -164,17 +164,17 @@ decorate_task(
     );
     Assert.deepEqual(
       sandboxManagerBroken.removeHold.args,
       [["ActionsManager"]],
       "sandbox holds should still be removed after a recipe failure",
     );
 
     Assert.deepEqual(reportActionStub.args, [["test-remote-action-broken", Uptake.ACTION_SUCCESS]]);
-    Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]]);
+    Assert.deepEqual(reportRecipeStub.args, [[recipe, Uptake.RECIPE_EXECUTION_ERROR]]);
   },
 );
 
 // Test life cycle for remote action that fails in post-step
 decorate_task(
   withStub(Uptake, "reportAction"),
   withStub(Uptake, "reportRecipe"),
   async function(reportActionStub, reportRecipeStub) {
@@ -204,17 +204,17 @@ decorate_task(
       "All callbacks should be executed",
     );
     Assert.deepEqual(
       sandboxManagerBroken.removeHold.args,
       [["ActionsManager"]],
       "sandbox holds should still be removed after a failure",
     );
 
-    Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_SUCCESS]]);
+    Assert.deepEqual(reportRecipeStub.args, [[recipe, Uptake.RECIPE_SUCCESS]]);
     Assert.deepEqual(reportActionStub.args, [
       ["test-remote-action-broken", Uptake.ACTION_POST_EXECUTION_ERROR],
     ]);
   },
 );
 
 // Test life cycle methods for local actions
 decorate_task(
--- a/toolkit/components/normandy/test/browser/browser_BaseAction.js
+++ b/toolkit/components/normandy/test/browser/browser_BaseAction.js
@@ -75,17 +75,17 @@ decorate_task(
 decorate_task(
   withStub(Uptake, "reportRecipe"),
   async function(reportRecipeStub) {
     const action = new NoopAction();
     const recipe = recipeFactory();
     await action.runRecipe(recipe);
     Assert.deepEqual(
       reportRecipeStub.args,
-      [[recipe.id, Uptake.RECIPE_SUCCESS]],
+      [[recipe, Uptake.RECIPE_SUCCESS]],
       "per-recipe uptake telemetry should be reported",
     );
   },
 );
 
 // Finalize causes action telemetry to be recorded
 decorate_task(
   withStub(Uptake, "reportAction"),
@@ -115,17 +115,17 @@ decorate_task(
     await Assert.rejects(
       action.runRecipe(recipe2),
       /^Error: Action has already been finalized$/,
       "running recipes after finalization is an error",
     );
 
     Assert.deepEqual(
       reportRecipeStub.args,
-      [[recipe1.id, Uptake.RECIPE_SUCCESS]],
+      [[recipe1, Uptake.RECIPE_SUCCESS]],
       "Only recipes executed prior to finalizer should report uptake telemetry",
     );
   },
 );
 
 // Test an action with a failing pre-execution step
 decorate_task(
   withStub(Uptake, "reportRecipe"),
@@ -146,17 +146,17 @@ decorate_task(
     is(action.state, FailPreExecutionAction.STATE_FINALIZED, "Action should be finalized");
     is(action.lastError, NoopAction._errorToThrow, "lastError should not have changed");
 
     is(action._testRunFlag, false, "_run should not have been called");
     is(action._testFinalizeFlag, false, "_finalize should not have been called");
 
     Assert.deepEqual(
       reportRecipeStub.args,
-      [[recipe.id, Uptake.RECIPE_ACTION_DISABLED]],
+      [[recipe, Uptake.RECIPE_ACTION_DISABLED]],
       "Recipe should report recipe status as action disabled",
     );
 
     Assert.deepEqual(
       reportActionStub.args,
       [[action.name, Uptake.ACTION_PRE_EXECUTION_ERROR]],
       "Action should report pre execution error",
     );
@@ -174,17 +174,17 @@ decorate_task(
     is(action.state, FailRunAction.STATE_READY, "Action should not be marked as failed due to a recipe failure");
     await action.finalize();
     is(action.state, FailRunAction.STATE_FINALIZED, "Action should be marked as finalized after finalize is called");
 
     ok(action._testFinalizeFlag, "_finalize should have been called");
 
     Assert.deepEqual(
       reportRecipeStub.args,
-      [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]],
+      [[recipe, Uptake.RECIPE_EXECUTION_ERROR]],
       "Recipe should report recipe execution error",
     );
 
     Assert.deepEqual(
       reportActionStub.args,
       [[action.name, Uptake.ACTION_SUCCESS]],
       "Action should report success",
     );
@@ -198,17 +198,17 @@ decorate_task(
   async function(reportRecipeStub, reportActionStub) {
     const recipe = recipeFactory();
     const action = new FailFinalizeAction();
     await action.runRecipe(recipe);
     await action.finalize();
 
     Assert.deepEqual(
       reportRecipeStub.args,
-      [[recipe.id, Uptake.RECIPE_SUCCESS]],
+      [[recipe, Uptake.RECIPE_SUCCESS]],
       "Recipe should report success",
     );
 
     Assert.deepEqual(
       reportActionStub.args,
       [[action.name, Uptake.ACTION_POST_EXECUTION_ERROR]],
       "Action should report post execution error",
     );
@@ -238,13 +238,13 @@ decorate_task(
     Assert.deepEqual(
       reportActionStub.args,
       [[action.name, Uptake.ACTION_SUCCESS]],
       "Action should not report pre execution error",
     );
 
     Assert.deepEqual(
       reportRecipeStub.args,
-      [[recipe.id, Uptake.RECIPE_ACTION_DISABLED]],
+      [[recipe, Uptake.RECIPE_ACTION_DISABLED]],
       "Recipe should report recipe status as action disabled",
     );
   },
 );
--- a/toolkit/components/normandy/test/browser/browser_actions_ConsoleLogAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_ConsoleLogAction.js
@@ -26,23 +26,23 @@ decorate_task(
     const infoStub = sinon.stub(action.log, "info");
 
     try {
       // message is required
       let recipe = {id: 1, arguments: {}};
       await action.runRecipe(recipe);
       is(action.lastError, null, "lastError should be null");
       Assert.deepEqual(infoStub.args, [], "no message should be logged");
-      Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]]);
+      Assert.deepEqual(reportRecipeStub.args, [[recipe, Uptake.RECIPE_EXECUTION_ERROR]]);
 
       reportRecipeStub.reset();
 
       // message must be a string
       recipe = {id: 1, arguments: {message: 1}};
       await action.runRecipe(recipe);
       is(action.lastError, null, "lastError should be null");
       Assert.deepEqual(infoStub.args, [], "no message should be logged");
-      Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]]);
+      Assert.deepEqual(reportRecipeStub.args, [[recipe, Uptake.RECIPE_EXECUTION_ERROR]]);
     } finally {
       infoStub.restore();
     }
   },
 );
--- a/toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
@@ -38,17 +38,17 @@ decorate_task(
   async function run_without_errors(reportRecipe) {
     const action = new PreferenceExperimentAction();
     const recipe = preferenceExperimentFactory();
     await action.runRecipe(recipe);
     await action.finalize();
     // runRecipe catches exceptions thrown by _run(), so
     // explicitly check for reported success here.
     Assert.deepEqual(reportRecipe.args,
-                     [[recipe.id, Uptake.RECIPE_SUCCESS]]);
+                     [[recipe, Uptake.RECIPE_SUCCESS]]);
   }
 );
 
 decorate_task(
   withStub(Uptake, "reportRecipe"),
   withStub(Uptake, "reportAction"),
   withPrefEnv({set: [["app.shield.optoutstudies.enabled", false]]}),
   async function checks_disabled(reportRecipe, reportAction) {
@@ -63,17 +63,17 @@ decorate_task(
     Assert.deepEqual(action.log.warn.args,
                      [["Skipping recipe preference-experiment because PreferenceExperimentAction " +
                        "was disabled during preExecution."]]);
 
     await action.finalize();
     Assert.deepEqual(action.log.debug.args,
                      [["Skipping post-execution hook for PreferenceExperimentAction because it is disabled."]]);
     Assert.deepEqual(reportRecipe.args,
-                     [[recipe.id, Uptake.RECIPE_ACTION_DISABLED]]);
+                     [[recipe, Uptake.RECIPE_ACTION_DISABLED]]);
     Assert.deepEqual(reportAction.args,
                      [[action.name, Uptake.ACTION_SUCCESS]]);
   }
 );
 
 decorate_task(
   withStub(PreferenceExperiments, "start"),
   PreferenceExperiments.withMockExperiments([]),
@@ -193,17 +193,17 @@ decorate_task(
     action.chooseBranch = sinon.stub().callsFake(async function(slug, branches) {
       return branches[0];
     });
 
     await action.runRecipe(recipe);
     await action.finalize();
 
     Assert.deepEqual(reportRecipeStub.args,
-                     [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]]);
+                     [[recipe, Uptake.RECIPE_EXECUTION_ERROR]]);
     Assert.deepEqual(startStub.args, [], "start not called");
     // No way to get access to log message/Error thrown
   }
 );
 
 decorate_task(
   withStub(PreferenceExperiments, "start"),
   PreferenceExperiments.withMockExperiments([]),
--- a/toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js
@@ -126,17 +126,17 @@ decorate_task(
     const action = new PreferenceRollbackAction();
     await action.runRecipe(recipe);
     await action.finalize();
     is(action.lastError, null, "lastError should be null");
 
     sendEventStub.assertEvents([]);
     Assert.deepEqual(
       reportRecipeStub.args,
-      [[recipe.id, Uptake.RECIPE_SUCCESS]],
+      [[recipe, Uptake.RECIPE_SUCCESS]],
       "recipe should be reported as succesful",
     );
   },
 );
 
 // Test that rolling back an already rolled back recipe doesn't do anything
 decorate_task(
   PreferenceRollouts.withTestMock,