From 15fd0cc1ffebf8dd31fd85554f11b556a46dc550 Mon Sep 17 00:00:00 2001 From: Stephanie Cunnane Date: Wed, 20 Nov 2024 19:08:19 +0000 Subject: [PATCH] Bug 1876178 - Improve default search engine changed telemetry. r=Standard8 Differential Revision: https://phabricator.services.mozilla.com/D224553 --- .../search/AppProvidedSearchEngine.sys.mjs | 98 +++++++++++++- .../components/search/SearchService.sys.mjs | 7 +- .../tests/xpcshell/test_reload_engines.js | 2 +- .../test_reload_engines_experiment.js | 2 +- .../xpcshell/test_telemetry_event_default.js | 127 ++++++++++++++++-- 5 files changed, 217 insertions(+), 19 deletions(-) diff --git a/toolkit/components/search/AppProvidedSearchEngine.sys.mjs b/toolkit/components/search/AppProvidedSearchEngine.sys.mjs index 6241687ad5b7..ce1fd7f78cab 100644 --- a/toolkit/components/search/AppProvidedSearchEngine.sys.mjs +++ b/toolkit/components/search/AppProvidedSearchEngine.sys.mjs @@ -18,6 +18,7 @@ const lazy = {}; ChromeUtils.defineESModuleGetters(lazy, { NimbusFeatures: "resource://nimbus/ExperimentAPI.sys.mjs", + ObjectUtils: "resource://gre/modules/ObjectUtils.sys.mjs", RemoteSettings: "resource://services-settings/remote-settings.sys.mjs", SearchUtils: "resource://gre/modules/SearchUtils.sys.mjs", }); @@ -396,6 +397,15 @@ export class AppProvidedSearchEngine extends SearchEngine { */ #isGeneralPurposeSearchEngine = false; + /** + * Stores certain initial info about an engine. Used to verify whether we've + * actually changed the engine, so that we don't record default engine + * changed telemetry unnecessarily. + * + * @type {Map} + */ + #prevEngineInfo = null; + /** * @param {object} options * The options for this search engine. @@ -415,6 +425,13 @@ export class AppProvidedSearchEngine extends SearchEngine { this.#init(config); this._loadSettings(settings); + + this.#prevEngineInfo = new Map([ + ["name", this.name], + ["_loadPath", this._loadPath], + ["submissionURL", this.getSubmission("foo").uri.spec], + ["aliases", this._definedAliases], + ]); } /** @@ -441,7 +458,26 @@ export class AppProvidedSearchEngine extends SearchEngine { update({ configuration }) { this._urls = []; this.#init(configuration); - lazy.SearchUtils.notifyAction(this, lazy.SearchUtils.MODIFIED_TYPE.CHANGED); + + let needToSendUpdate = this.#hasBeenModified(this, this.#prevEngineInfo, [ + "name", + "_loadPath", + "aliases", + ]); + + // We only send a notification if critical fields have changed, e.g., ones + // that may affect the UI or telemetry. If we want to add more fields here + // in the future, we need to ensure we don't send unnecessary + // `engine-update` telemetry. Therefore we may need additional notification + // types or to implement an alternative. + if (needToSendUpdate) { + lazy.SearchUtils.notifyAction( + this, + lazy.SearchUtils.MODIFIED_TYPE.CHANGED + ); + + this._resetPrevEngineInfo(); + } } /** @@ -481,6 +517,17 @@ export class AppProvidedSearchEngine extends SearchEngine { return this.#isGeneralPurposeSearchEngine; } + /** + * Stores certain initial info about an engine. Used to verify whether we've + * actually changed the engine, so that we don't record default engine + * changed telemetry unnecessarily. + * + * @returns {Map} + */ + get prevEngineInfo() { + return this.#prevEngineInfo; + } + /** * Returns the icon URL for the search engine closest to the preferred width. * @@ -640,4 +687,53 @@ export class AppProvidedSearchEngine extends SearchEngine { this._urls.push(engineURL); } + + /** + * Determines whether the specified engine properties differ between their + * current and initial values. + * + * @param {Engine} currentEngine + * The current engine. + * @param {Map} initialValues + * The initial values stored for the currentEngine. + * @param {Array} targetKeys + * The relevant keys to compare (current value for the engine vs. initial + * value). + * @returns {boolean} + * Returns true if any of the properties relevant to default engine changed + * telemetry was changed. + */ + #hasBeenModified(currentEngine, initialValues, targetKeys) { + for (let i = 0; i < targetKeys.length; i++) { + let key = targetKeys[i]; + + if ( + !lazy.ObjectUtils.deepEqual(currentEngine[key], initialValues.get(key)) + ) { + return true; + } + + let currentEngineSubmissionURL = + currentEngine.getSubmission("foo").uri.spec; + if (currentEngineSubmissionURL != initialValues.get("submissionURL")) { + return true; + } + } + + return false; + } + + // Not #-prefixed private because it's spied upon in a test. + _resetPrevEngineInfo() { + this.#prevEngineInfo.forEach((_value, key) => { + let newValue; + if (key == "submissionURL") { + newValue = this.submissionURL ?? this.getSubmission("foo").uri.spec; + } else { + newValue = this[key]; + } + + this.#prevEngineInfo.set(key, newValue); + }); + } } diff --git a/toolkit/components/search/SearchService.sys.mjs b/toolkit/components/search/SearchService.sys.mjs index 1205d55cfe70..97dfe77ab686 100644 --- a/toolkit/components/search/SearchService.sys.mjs +++ b/toolkit/components/search/SearchService.sys.mjs @@ -1969,8 +1969,9 @@ export class SearchService { engine.pendingRemoval = true; continue; } else { - // This is an existing engine that we should update (we don't know if - // the configuration for this engine has changed or not). + // This is an existing engine that we should update. (However + // notification will happen only if the configuration for this engine + // has changed). await engine.update({ configuration: configEngines[index], }); @@ -2917,7 +2918,7 @@ export class SearchService { * check the "separatePrivateDefault" preference - that is up to the caller. * @param {nsISearchEngine} newEngine * The search engine to select - * @param {SearchUtils.REASON_CHANGE_MAP} changeSource + * @param {REASON_CHANGE_MAP} changeSource * The source of the change of engine. */ #setEngineDefault(privateMode, newEngine, changeSource) { diff --git a/toolkit/components/search/tests/xpcshell/test_reload_engines.js b/toolkit/components/search/tests/xpcshell/test_reload_engines.js index f097c8c0d243..b342345a941e 100644 --- a/toolkit/components/search/tests/xpcshell/test_reload_engines.js +++ b/toolkit/components/search/tests/xpcshell/test_reload_engines.js @@ -222,7 +222,7 @@ add_task(async function test_config_updated_engine_changes() { Assert.deepEqual( enginesModified.sort(), - ["appDefault", "defaultInFRRegion", "hasParamsInFRRegion"], + ["hasParamsInFRRegion"], "Should have modified the expected engines" ); diff --git a/toolkit/components/search/tests/xpcshell/test_reload_engines_experiment.js b/toolkit/components/search/tests/xpcshell/test_reload_engines_experiment.js index ba76c16274c0..eafe8819a390 100644 --- a/toolkit/components/search/tests/xpcshell/test_reload_engines_experiment.js +++ b/toolkit/components/search/tests/xpcshell/test_reload_engines_experiment.js @@ -98,7 +98,7 @@ add_task(async function test_config_updated_engine_changes() { Assert.deepEqual( enginesModified.sort(), - ["appDefault", "non-experiment"], + [], "Should have modified the expected engines" ); diff --git a/toolkit/components/search/tests/xpcshell/test_telemetry_event_default.js b/toolkit/components/search/tests/xpcshell/test_telemetry_event_default.js index 2e124dbff901..6d1fbc907489 100644 --- a/toolkit/components/search/tests/xpcshell/test_telemetry_event_default.js +++ b/toolkit/components/search/tests/xpcshell/test_telemetry_event_default.js @@ -10,6 +10,8 @@ "use strict"; ChromeUtils.defineESModuleGetters(this, { + AppProvidedSearchEngine: + "resource://gre/modules/AppProvidedSearchEngine.sys.mjs", NimbusFeatures: "resource://nimbus/ExperimentAPI.sys.mjs", }); @@ -128,6 +130,38 @@ const MAIN_CONFIG = [ }, ]; +const CONFIG_WITH_MODIFIED_CLASSIFICATION = [ + { + identifier: "originalDefault", + base: { + name: "Original Default", + urls: { + search: { + base: "https://example.com/search", + searchTermParamName: "q", + }, + }, + classification: "unknown", + }, + }, +]; + +const CONFIG_WITH_MODIFIED_NAME = [ + { + identifier: "originalDefault", + base: { + name: "Modified Engine Name", + urls: { + search: { + base: "https://example.com/search", + searchTermParamName: "q", + }, + }, + classification: "general", + }, + }, +]; + const testSearchEngine = { id: "originalDefault", name: "Original Default", @@ -171,13 +205,6 @@ async function checkTelemetry( checkPrivate = false, additionalEventsExpected = false ) { - // TODO Bug 1876178 - Improve engine change telemetry. - // When we reload engines due to a config change, we update the engines as - // they may have changed, we don't track if any attribute has actually changed - // from previous, and so we send out an update regardless. This is why in - // this test we test for the additional `engine-update` event that's recorded. - // In future, we should be more specific about when to record the event and - // so only one event is captured and not two. let snapshot; if (checkPrivate) { snapshot = await Glean.searchEnginePrivate.changed.testGetValue(); @@ -185,6 +212,8 @@ async function checkTelemetry( snapshot = await Glean.searchEngineDefault.changed.testGetValue(); } + // additionalEventsExpected should be true whenever we expect something + // stored in AppProvidedSearchEngine.#prevEngineInfo to have changed. if (additionalEventsExpected) { delete snapshot[0].timestamp; Assert.deepEqual( @@ -286,8 +315,7 @@ add_task(async function test_experiment_changes_default() { "experiment", testNewDefaultEngine, testDefaultForExperiment, - false, - true + false ); // Reset the stub so that we are no longer in an experiment. @@ -306,8 +334,7 @@ add_task(async function test_locale_changes_default() { "locale", testDefaultForExperiment, testDefaultInLocaleFRNotRegionDEEngine, - false, - true + false ); }); @@ -323,8 +350,7 @@ add_task(async function test_region_changes_default() { "region", testDefaultInLocaleFRNotRegionDEEngine, testPrefEngine, - false, - true + false ); }); @@ -445,3 +471,78 @@ add_task(async function test_default_engine_update() { await checkTelemetry("engine-update", defaultEngineData, defaultEngineData); await extension.unload(); }); + +add_task(async function test_only_notify_on_relevant_engine_property_change() { + clearTelemetry(); + await SearchTestUtils.updateRemoteSettingsConfig(BASE_CONFIG); + + // Since SearchUtils.notifyAction can be called for multiple different search + // engine topics, `resetPrevEngineInfo` is a better way to track + // notifications in this case. + let notificationSpy = sinon.spy( + AppProvidedSearchEngine.prototype, + "_resetPrevEngineInfo" + ); + + // Change an engine property that is not stored in + // AppProvidedSearchEngine.#prevEngineInfo. + let reloadObserved = + SearchTestUtils.promiseSearchNotification("engines-reloaded"); + await SearchTestUtils.updateRemoteSettingsConfig( + CONFIG_WITH_MODIFIED_CLASSIFICATION + ); + await reloadObserved; + + Assert.equal( + notificationSpy.callCount, + 0, + "Should not have sent a notification" + ); + + notificationSpy.restore(); +}); + +add_task( + async function test_multiple_updates_only_notify_on_relevant_engine_property_change() { + clearTelemetry(); + await SearchTestUtils.updateRemoteSettingsConfig(BASE_CONFIG); + + // Since SearchUtils.notifyAction can be called for multiple different search + // engine topics, `resetPrevEngineInfo` is a better way to track + // notifications in this case. + let notificationSpy = sinon.spy( + AppProvidedSearchEngine.prototype, + "_resetPrevEngineInfo" + ); + + // Change an engine property that is not stored in + // AppProvidedSearchEngine.#prevEngineInfo. + let reloadObserved1 = + SearchTestUtils.promiseSearchNotification("engines-reloaded"); + await SearchTestUtils.updateRemoteSettingsConfig( + CONFIG_WITH_MODIFIED_CLASSIFICATION + ); + await reloadObserved1; + + Assert.equal( + notificationSpy.callCount, + 0, + "Should not have sent a notification" + ); + + // Now change an engine property that is stored in + // AppProvidedSearchEngine.#prevEngineInfo. + let reloadObserved2 = + SearchTestUtils.promiseSearchNotification("engines-reloaded"); + await SearchTestUtils.updateRemoteSettingsConfig(CONFIG_WITH_MODIFIED_NAME); + await reloadObserved2; + + Assert.equal( + notificationSpy.callCount, + 1, + "Should have sent a notification" + ); + + notificationSpy.restore(); + } +);