Bug 1530508: report additional recipe information r=mythmon
authorEthan Glasser-Camp <ethan@betacantrips.com>
Wed, 06 Mar 2019 21:12:03 +0000
changeset 520760 10333e6aa4b0a68b31732d0cd5a6bcc56019d2d1
parent 520759 0d1b3156e7c82db4faa0fabed419fa9a1be29006
child 520761 b7e35dab3ad56566cf69bd6930db6967ff985d64
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: report additional recipe information r=mythmon Report when recipes don't match the filter. Report when Jexl filters themselves fail, with an added test. The existing test for remote-settings usage had a bug, so fix that too. Depends on D22017 Differential Revision: https://phabricator.services.mozilla.com/D22018
toolkit/components/normandy/lib/RecipeRunner.jsm
toolkit/components/normandy/lib/Uptake.jsm
toolkit/components/normandy/test/browser/browser_RecipeRunner.js
--- a/toolkit/components/normandy/lib/RecipeRunner.jsm
+++ b/toolkit/components/normandy/lib/RecipeRunner.jsm
@@ -45,17 +45,17 @@ const PREFS_TO_WATCH = [
   RUN_INTERVAL_PREF,
   TELEMETRY_ENABLED_PREF,
   SHIELD_ENABLED_PREF,
   API_URL_PREF,
 ];
 
 XPCOMUtils.defineLazyGetter(this, "gRemoteSettingsClient", () => {
   return RemoteSettings(REMOTE_SETTINGS_COLLECTION, {
-    filterFunc: async recipe => RecipeRunner.checkFilter(recipe) ? recipe : null,
+    filterFunc: async recipe => (await RecipeRunner.checkFilter(recipe)) ? recipe : null,
   });
 });
 
 /**
  * cacheProxy returns an object Proxy that will memoize properties of the target.
  */
 function cacheProxy(target) {
   const cache = new Map();
@@ -294,23 +294,34 @@ var RecipeRunner = {
    * Evaluate a recipe's filter expression against the environment.
    * @param {object} recipe
    * @param {string} recipe.filter The expression to evaluate against the environment.
    * @return {boolean} The result of evaluating the filter, cast to a bool, or false
    *                   if an error occurred during evaluation.
    */
   async checkFilter(recipe) {
     const context = this.getFilterContext(recipe);
+    let result;
     try {
-      const result = await FilterExpressions.eval(recipe.filter_expression, context);
-      return !!result;
+      result = await FilterExpressions.eval(recipe.filter_expression, context);
     } catch (err) {
       log.error(`Error checking filter for "${recipe.name}". Filter: [${recipe.filter_expression}]. Error: "${err}"`);
+      Uptake.reportRecipe(recipe, Uptake.RECIPE_FILTER_BROKEN);
       return false;
     }
+
+    if (!result) {
+      // This represents a terminal state for the given recipe, so
+      // report its outcome. Others are reported when executed in
+      // ActionsManager.
+      Uptake.reportRecipe(recipe, Uptake.RECIPE_DIDNT_MATCH_FILTER);
+      return false;
+    }
+
+    return true;
   },
 
   /**
    * Clear all caches of systems used by RecipeRunner, in preparation
    * for a clean run.
    */
   clearCaches() {
     ClientEnvironment.clearClassifyCache();
@@ -330,9 +341,23 @@ var RecipeRunner = {
       Storage.clearAllStorage();
       this.clearCaches();
       await this.run();
     } finally {
       Services.prefs.setCharPref(API_URL_PREF, oldApiUrl);
       this.clearCaches();
     }
   },
+
+  /**
+   * Offer a mechanism to get access to the lazily-instantiated
+   * gRemoteSettingsClient, because if someone instantiates it
+   * themselves, it won't have the options we provided in this module,
+   * and it will prevent instantiation by this module later.
+   *
+   * This is only meant to be used in testing, where it is a
+   * convenient hook to store data in the underlying remote-settings
+   * collection.
+   */
+  get _remoteSettingsClientForTesting() {
+    return gRemoteSettingsClient;
+  },
 };
--- a/toolkit/components/normandy/lib/Uptake.jsm
+++ b/toolkit/components/normandy/lib/Uptake.jsm
@@ -18,17 +18,19 @@ var Uptake = {
   ACTION_NETWORK_ERROR: UptakeTelemetry.STATUS.NETWORK_ERROR,
   ACTION_PRE_EXECUTION_ERROR: UptakeTelemetry.STATUS.CUSTOM_1_ERROR,
   ACTION_POST_EXECUTION_ERROR: UptakeTelemetry.STATUS.CUSTOM_2_ERROR,
   ACTION_SERVER_ERROR: UptakeTelemetry.STATUS.SERVER_ERROR,
   ACTION_SUCCESS: UptakeTelemetry.STATUS.SUCCESS,
 
   // Per-recipe uptake
   RECIPE_ACTION_DISABLED: UptakeTelemetry.STATUS.CUSTOM_1_ERROR,
+  RECIPE_DIDNT_MATCH_FILTER: UptakeTelemetry.STATUS.CUSTOM_2_ERROR,
   RECIPE_EXECUTION_ERROR: UptakeTelemetry.STATUS.APPLY_ERROR,
+  RECIPE_FILTER_BROKEN: UptakeTelemetry.STATUS.CONTENT_ERROR,
   RECIPE_INVALID_ACTION: UptakeTelemetry.STATUS.DOWNLOAD_ERROR,
   RECIPE_SUCCESS: UptakeTelemetry.STATUS.SUCCESS,
 
   // Uptake for the runner as a whole
   RUNNER_INVALID_SIGNATURE: UptakeTelemetry.STATUS.SIGNATURE_ERROR,
   RUNNER_NETWORK_ERROR: UptakeTelemetry.STATUS.NETWORK_ERROR,
   RUNNER_SERVER_ERROR: UptakeTelemetry.STATUS.SERVER_ERROR,
   RUNNER_SUCCESS: UptakeTelemetry.STATUS.SUCCESS,
--- a/toolkit/components/normandy/test/browser/browser_RecipeRunner.js
+++ b/toolkit/components/normandy/test/browser/browser_RecipeRunner.js
@@ -1,11 +1,12 @@
 "use strict";
 
 ChromeUtils.import("resource://testing-common/TestUtils.jsm", this);
+ChromeUtils.import("resource://gre/modules/components-utils/FilterExpressions.jsm", this);
 ChromeUtils.import("resource://normandy/lib/RecipeRunner.jsm", this);
 ChromeUtils.import("resource://normandy/lib/ClientEnvironment.jsm", this);
 ChromeUtils.import("resource://normandy/lib/CleanupManager.jsm", this);
 ChromeUtils.import("resource://normandy/lib/NormandyApi.jsm", this);
 ChromeUtils.import("resource://normandy/lib/ActionSandboxManager.jsm", this);
 ChromeUtils.import("resource://normandy/lib/ActionsManager.jsm", this);
 ChromeUtils.import("resource://normandy/lib/AddonStudies.jsm", this);
 ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
@@ -74,16 +75,35 @@ add_task(async function checkFilter() {
   // The given recipe must be available to the filter context.
   const recipe = {filter_expression: "normandy.recipe.id == 7", id: 7};
   ok(await RecipeRunner.checkFilter(recipe), "The recipe is available in the filter context");
   recipe.id = 4;
   ok(!(await RecipeRunner.checkFilter(recipe)), "The recipe is available in the filter context");
 });
 
 decorate_task(
+  withStub(FilterExpressions, "eval"),
+  withStub(Uptake, "reportRecipe"),
+  async function checkFilterCanHandleExceptions(
+    evalStub,
+    reportRecipeStub,
+  ) {
+    evalStub.throws("this filter was broken somehow");
+    const someRecipe = {id: "1", action: "action", filter_expression: "broken"};
+    const result = await RecipeRunner.checkFilter(someRecipe);
+
+    Assert.deepEqual(result, false, "broken filters are treated as false");
+    Assert.deepEqual(
+      reportRecipeStub.args,
+      [[someRecipe, Uptake.RECIPE_FILTER_BROKEN]]
+    );
+  }
+);
+
+decorate_task(
   withMockNormandyApi,
   withStub(ClientEnvironment, "getClientClassification"),
   async function testClientClassificationCache(api, getStub) {
     getStub.returns(Promise.resolve(false));
 
     await SpecialPowers.pushPrefEnv({set: [
       ["app.normandy.api_url",
        "https://example.com/selfsupport-dummy"],
@@ -133,23 +153,25 @@ async function withMockActionSandboxMana
 
 decorate_task(
   withStub(Uptake, "reportRunner"),
   withStub(NormandyApi, "fetchRecipes"),
   withStub(ActionsManager.prototype, "fetchRemoteActions"),
   withStub(ActionsManager.prototype, "preExecution"),
   withStub(ActionsManager.prototype, "runRecipe"),
   withStub(ActionsManager.prototype, "finalize"),
+  withStub(Uptake, "reportRecipe"),
   async function testRun(
     reportRunnerStub,
     fetchRecipesStub,
     fetchRemoteActionsStub,
     preExecutionStub,
     runRecipeStub,
-    finalizeStub
+    finalizeStub,
+    reportRecipeStub,
   ) {
     const runRecipeReturn = Promise.resolve();
     const runRecipeReturnThen = sinon.spy(runRecipeReturn, "then");
     runRecipeStub.returns(runRecipeReturn);
 
     const matchRecipe = {id: "match", action: "matchAction", filter_expression: "true"};
     const noMatchRecipe = {id: "noMatch", action: "noMatchAction", filter_expression: "false"};
     const missingRecipe = {id: "missing", action: "missingAction", filter_expression: "true"};
@@ -167,51 +189,63 @@ decorate_task(
     ok(runRecipeReturnThen.called, "the run method should be used asyncronously");
 
     // Test uptake reporting
     Assert.deepEqual(
       reportRunnerStub.args,
       [[Uptake.RUNNER_SUCCESS]],
       "RecipeRunner should report uptake telemetry",
     );
+    Assert.deepEqual(
+      reportRecipeStub.args,
+      [[noMatchRecipe, Uptake.RECIPE_DIDNT_MATCH_FILTER]],
+      "Filtered-out recipes should be reported",
+    );
   }
 );
 
 decorate_task(
   withPrefEnv({
     set: [
       ["features.normandy-remote-settings.enabled", true],
     ],
   }),
   withStub(ActionsManager.prototype, "runRecipe"),
   withStub(ActionsManager.prototype, "fetchRemoteActions"),
   withStub(ActionsManager.prototype, "finalize"),
+  withStub(Uptake, "reportRecipe"),
   async function testReadFromRemoteSettings(
     runRecipeStub,
     fetchRemoteActionsStub,
     finalizeStub,
+    reportRecipeStub,
   ) {
     const matchRecipe = { id: "match", action: "matchAction", filter_expression: "true", _status: "synced", enabled: true };
     const noMatchRecipe = { id: "noMatch", action: "noMatchAction", filter_expression: "false", _status: "synced", enabled: true };
     const missingRecipe = { id: "missing", action: "missingAction", filter_expression: "true", _status: "synced", enabled: true };
 
-    const rsCollection = await RemoteSettings("normandy-recipes").openCollection();
+    const rsCollection = await RecipeRunner._remoteSettingsClientForTesting.openCollection();
     await rsCollection.create(matchRecipe, { synced: true });
     await rsCollection.create(noMatchRecipe, { synced: true });
     await rsCollection.create(missingRecipe, { synced: true });
     await rsCollection.db.saveLastModified(42);
     rsCollection.db.close();
 
     await RecipeRunner.run();
 
     Assert.deepEqual(
       runRecipeStub.args,
       [[matchRecipe], [missingRecipe]],
       "recipe with matching filters should be executed",
     );
+    Assert.deepEqual(
+      reportRecipeStub.args,
+      [[noMatchRecipe, Uptake.RECIPE_DIDNT_MATCH_FILTER]],
+      "Filtered-out recipes should be reported",
+    );
   }
 );
 
 decorate_task(
   withMockNormandyApi,
   async function testRunFetchFail(mockApi) {
     const reportRunner = sinon.stub(Uptake, "reportRunner");