diff --git a/browser/components/newtab/docs/v2-system-addon/data_events.md b/browser/components/newtab/docs/v2-system-addon/data_events.md index 2d5f9531203d..7bd2022b598d 100644 --- a/browser/components/newtab/docs/v2-system-addon/data_events.md +++ b/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 diff --git a/browser/components/newtab/lib/ASRouterTargeting.jsm b/browser/components/newtab/lib/ASRouterTargeting.jsm index 6091e4d85b1e..0a49ebc890b1 100644 --- a/browser/components/newtab/lib/ASRouterTargeting.jsm +++ b/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, { diff --git a/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js b/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js index f6b3230498da..8f829d693097 100644 --- a/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js +++ b/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(); diff --git a/toolkit/components/messaging-system/targeting/Targeting.jsm b/toolkit/components/messaging-system/targeting/Targeting.jsm index 402bf4d2824e..3be1dd1551cc 100644 --- a/toolkit/components/messaging-system/targeting/Targeting.jsm +++ b/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) diff --git a/toolkit/components/messaging-system/targeting/test/unit/test_targeting.js b/toolkit/components/messaging-system/targeting/test/unit/test_targeting.js index 661c21dd3d4b..7f6f6bef5eb7 100644 --- a/toolkit/components/messaging-system/targeting/test/unit/test_targeting.js +++ b/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(); +}); diff --git a/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm b/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm index 5a74b9037a2e..877d815141ae 100644 --- a/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm +++ b/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); diff --git a/toolkit/components/nimbus/test/browser/browser_experiment_evaluate_jexl.js b/toolkit/components/nimbus/test/browser/browser_experiment_evaluate_jexl.js index 126c139f355d..f2199c19cf61 100644 --- a/toolkit/components/nimbus/test/browser/browser_experiment_evaluate_jexl.js +++ b/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" ); diff --git a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js index 169515f4a52d..16b5380c6b31 100644 --- a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js +++ b/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" ); diff --git a/toolkit/components/telemetry/Events.yaml b/toolkit/components/telemetry/Events.yaml index d965f99a9d3b..e6f3d90b5cc7 100644 --- a/toolkit/components/telemetry/Events.yaml +++ b/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