Bug 1732724 - Report targeting expression source in the case of undesired events r=k88hudson

Differential Revision: https://phabricator.services.mozilla.com/D126776

browser/components/newtab/docs/v2-system-addon/data_events.md

@@ -1068,21 +1068,28 @@ Unlike other Activity Stream pings, this is a Firefox Events telemetry event, an
 ### Experiment attribute errors
 
 This records whether issues were encountered with any of the targeting attributes used in the experiment enrollment or message targeting.
-Two different types of events are sent: `attribute_error` and `attribute_timeout` along with the attribute that caused it.
+Two different types of events are sent: `attribute_error` and `attribute_timeout` along with the attribute that caused it. An attribute
+is a variable inside the JEXL targeting expression that is evaluated client side by the browser.
 
 ```js
-[
+{
   "messaging_experiments",
   "targeting",
   "attribute_error", // event
-  "foo" // attribute
-],
-[
+  "foo", // attribute,
+  "extra_keys": {
+    "source": "message id or experiment slug",
+  },
+},
+{
   "messaging_experiments",
   "targeting",
   "attribute_timeout", // event
-  "bar" // attribute
-]
+  "bar", // attribute,
+  "extra_keys": {
+    "source": "message id or experiment slug",
+  },
+}
 ```
 
 ## Firefox Onboarding (about:welcome) pings

browser/components/newtab/lib/ASRouterTargeting.jsm

@@ -743,6 +743,9 @@ this.ASRouterTargeting = {
           return result.value;
         }
       }
+      // Used to report the source of the targeting error in the case of
+      // undesired events
+      targetingContext.setTelemetrySource(message.id);
       result = await targetingContext.evalWithDefault(message.targeting);
       if (shouldCache) {
         jexlEvaluationCache.set(message.targeting, {

browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js

@@ -313,6 +313,7 @@ describe("ASRouterTargeting", () => {
     fakeTargetingContext = {
       combineContexts: sandbox.stub(),
       evalWithDefault: sandbox.stub().resolves(),
+      setTelemetrySource: sandbox.stub(),
     };
     globals = new GlobalOverrider();
     globals.set(
@@ -322,6 +323,10 @@ describe("ASRouterTargeting", () => {
           return fakeTargetingContext.combineContexts.apply(sandbox, args);
         }
 
+        setTelemetrySource(id) {
+          fakeTargetingContext.setTelemetrySource(id);
+        }
+
         evalWithDefault(expr) {
           return fakeTargetingContext.evalWithDefault(expr);
         }
@@ -334,6 +339,23 @@ describe("ASRouterTargeting", () => {
     sandbox.restore();
     globals.restore();
   });
+  it("should provide message.id as source", async () => {
+    await ASRouterTargeting.checkMessageTargeting(
+      {
+        id: "message",
+        targeting: "true",
+      },
+      fakeTargetingContext,
+      sandbox.stub(),
+      false
+    );
+    assert.calledOnce(fakeTargetingContext.evalWithDefault);
+    assert.calledWithExactly(fakeTargetingContext.evalWithDefault, "true");
+    assert.calledWithExactly(
+      fakeTargetingContext.setTelemetrySource,
+      "message"
+    );
+  });
   it("should cache evaluation result", async () => {
     evalStub.resolves(true);
     let targetingContext = new global.TargetingContext();

toolkit/components/messaging-system/targeting/Targeting.jsm

@@ -67,7 +67,9 @@ const TargetingEnvironment = {
 };
 
 class TargetingContext {
-  constructor(customContext) {
+  #telemetrySource = null;
+
+  constructor(customContext, options = { source: null }) {
     if (customContext) {
       this.ctx = new Proxy(customContext, {
         get: (customCtx, prop) => {
@@ -81,17 +83,36 @@ class TargetingContext {
       this.ctx = TargetingEnvironment;
     }
 
+    // Used in telemetry to report where the targeting expression is coming from
+    this.#telemetrySource = options.source;
+
     // Enable event recording
     Services.telemetry.setEventRecordingEnabled(TARGETING_EVENT_CATEGORY, true);
   }
 
+  setTelemetrySource(source) {
+    if (source) {
+      this.#telemetrySource = source;
+    }
+  }
+
   _sendUndesiredEvent(eventData) {
-    Services.telemetry.recordEvent(
-      TARGETING_EVENT_CATEGORY,
-      TARGETING_EVENT_METHOD,
-      eventData.event,
-      eventData.value
-    );
+    if (this.#telemetrySource) {
+      Services.telemetry.recordEvent(
+        TARGETING_EVENT_CATEGORY,
+        TARGETING_EVENT_METHOD,
+        eventData.event,
+        eventData.value,
+        { source: this.#telemetrySource }
+      );
+    } else {
+      Services.telemetry.recordEvent(
+        TARGETING_EVENT_CATEGORY,
+        TARGETING_EVENT_METHOD,
+        eventData.event,
+        eventData.value
+      );
+    }
   }
 
   /**
@@ -215,7 +236,7 @@ class TargetingContext {
    * @param {string} expression JEXL expression
    * @returns {promise} Evaluation result
    */
-  evalWithDefault(expression) {
+  evalWithDefault(expression, options = { source: null }) {
     return FilterExpressions.eval(
       expression,
       this.createContextWithTimeout(this.ctx)

toolkit/components/messaging-system/targeting/test/unit/test_targeting.js

@@ -260,3 +260,67 @@ add_task(async function test_targeting_os() {
   );
   Assert.ok(res, `Should detect platform version got: ${res}`);
 });
+
+add_task(async function test_targeting_source_constructor() {
+  Services.telemetry.clearEvents();
+  const targeting = new TargetingContext(
+    {
+      foo: true,
+      get bar() {
+        throw new Error("bar");
+      },
+    },
+    { source: "unit_testing" }
+  );
+
+  let res = await targeting.eval("ctx.foo");
+  Assert.ok(res, "Should eval to true");
+
+  let expectedEvents = [
+    [
+      "messaging_experiments",
+      "targeting",
+      "attribute_error",
+      "ctx.bar",
+      JSON.stringify({ source: "unit_testing" }),
+    ],
+  ];
+  try {
+    await targeting.eval("ctx.bar");
+  } catch (e) {}
+
+  TelemetryTestUtils.assertEvents(expectedEvents);
+  Services.telemetry.clearEvents();
+});
+
+add_task(async function test_targeting_source_override() {
+  Services.telemetry.clearEvents();
+  const targeting = new TargetingContext(
+    {
+      foo: true,
+      get bar() {
+        throw new Error("bar");
+      },
+    },
+    { source: "unit_testing" }
+  );
+
+  let res = await targeting.eval("ctx.foo");
+  Assert.ok(res, "Should eval to true");
+
+  let expectedEvents = [
+    [
+      "messaging_experiments",
+      "targeting",
+      "attribute_error",
+      "ctx.bar",
+      JSON.stringify({ source: "override" }),
+    ],
+  ];
+  try {
+    await targeting.evalWithDefault("ctx.bar", { source: "override" });
+  } catch (e) {}
+
+  TelemetryTestUtils.assertEvents(expectedEvents);
+  Services.telemetry.clearEvents();
+});

toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm

@@ -109,7 +109,10 @@ const RemoteDefaultsLoader = {
           try {
             result = await RemoteSettingsExperimentLoader.evaluateJexl(
               configuration.targeting,
-              { activeRemoteDefaults: existingConfigIds }
+              {
+                activeRemoteDefaults: existingConfigIds,
+                source: configuration.slug,
+              }
             );
           } catch (e) {
             Cu.reportError(e);
@@ -246,6 +249,12 @@ class _RemoteSettingsExperimentLoader {
       );
     }
 
+    if (!customContext.source) {
+      throw new Error(
+        "Expected a .source property that identifies which targeting expression is being evaluated."
+      );
+    }
+
     const context = TargetingContext.combineContexts(
       customContext,
       this.manager.createTargetingContext(),
@@ -253,7 +262,9 @@ class _RemoteSettingsExperimentLoader {
     );
 
     log.debug("Testing targeting expression:", jexlString);
-    const targetingContext = new TargetingContext(context);
+    const targetingContext = new TargetingContext(context, {
+      source: customContext.source,
+    });
 
     let result = null;
     try {
@@ -279,6 +290,7 @@ class _RemoteSettingsExperimentLoader {
 
     const result = await this.evaluateJexl(recipe.targeting, {
       experiment: recipe,
+      source: recipe.slug,
     });
 
     return Boolean(result);

toolkit/components/nimbus/test/browser/browser_experiment_evaluate_jexl.js

@@ -25,11 +25,15 @@ add_task(async function setup() {
 
 const FAKE_CONTEXT = {
   experiment: ExperimentFakes.recipe("fake-test-experiment"),
+  source: "browser_experiment_evaluate_jexl",
 };
 
 add_task(async function test_throws_if_no_experiment_in_context() {
   await Assert.rejects(
-    RemoteSettingsExperimentLoader.evaluateJexl("true", { customThing: 1 }),
+    RemoteSettingsExperimentLoader.evaluateJexl("true", {
+      customThing: 1,
+      source: "test_throws_if_no_experiment_in_context",
+    }),
     /Expected an .experiment or .activeRemoteDefaults/,
     "should throw if experiment is not passed to the custom context"
   );

toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js

@@ -178,12 +178,18 @@ add_task(async function test_checkTargeting() {
     "should return true if .targeting is not defined"
   );
   equal(
-    await loader.checkTargeting({ targeting: "'foo'" }),
+    await loader.checkTargeting({
+      targeting: "'foo'",
+      slug: "test_checkTargeting",
+    }),
     true,
     "should return true for truthy expression"
   );
   equal(
-    await loader.checkTargeting({ targeting: "aPropertyThatDoesNotExist" }),
+    await loader.checkTargeting({
+      targeting: "aPropertyThatDoesNotExist",
+      slug: "test_checkTargeting",
+    }),
     false,
     "should return false for falsey expression"
   );

toolkit/components/telemetry/Events.yaml

@@ -1200,6 +1200,8 @@ messaging_experiments:
       - attribute_timeout
     methods:
       - targeting
+    extra_keys:
+      source: "Source of targeting expression: experiment slug or message id"
     release_channel_collection: opt-out
     products:
       - firefox