Bug 1876178 - Improve default search engine changed telemetry. r=Standard8

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

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<string>} 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);
+    });
+  }
 }

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) {

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"
   );
 

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"
   );
 

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();
+  }
+);