Bug 1860791 - Fix dismissals and telemetry for Rust Wikipedia Suggest results. r=daisuke

Wikipedia suggestions from Rust don't have an `advertiser` property, unlike
suggestions from the JS backend [1]. That causes an exception to be thrown when
`UrlbarProviderQuickSuggest` tries to convert the advertister to lower case for
telemetry. The result is that telemetry isn't properly recorded for Rust
Wikipedia suggestions, and anything that happens after telemetry is supposed to
be recorded doesn't actually happen, which means dismissals aren't handled.

I fixed this by adding `advertiser` to Rust Wikipedia suggestions, and I
modified the telemetry tests for sponsored and nonsponsored suggestions so they
also run with Rust enabled. This tests the code path where the error was thrown.

Running the telemetry tests with Rust enabled revealed some more problems that I
also fixed:

* Rust Wikipedia suggestions also don't have `iab_category`, so I added that to
  them too.
* `_assertGleanPing()` in head.js wasn't checking a few properties, so I added
  them. Now it checks them all.
* URLs in the Glean ping are declared with type `url`, which means our tests
  need to use valid URLs in their dummy data. The mock Merino server and some
  related tests are using invalid URL values like `"url"`, so I fixed that.

[1] The suggestions data in remote settings does include an `advertiser`, so
it's possible to consider this a bug in the Rust implementation, but IMO it
makes sense for nonsponsored suggestions to lack an advertiser, and it's always
set to `"Wikipedia"` anyway.

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

browser/components/urlbar/private/AdmWikipedia.sys.mjs

@@ -125,17 +125,26 @@ export class AdmWikipedia extends BaseFeature {
 
   makeResult(queryContext, suggestion, searchString) {
     if (suggestion.source == "rust") {
-      suggestion = {
+      // The Rust backend uses camelCase instead of snake_case, and it excludes
+      // some properties in non-sponsored suggestions that we expect, so convert
+      // the Rust suggestion to a suggestion object we expect here on desktop.
+      let desktopSuggestion = {
         title: suggestion.title,
         url: suggestion.url,
         is_sponsored: suggestion.is_sponsored,
         full_keyword: suggestion.fullKeyword,
-        impression_url: suggestion.impressionUrl,
-        click_url: suggestion.clickUrl,
-        block_id: suggestion.blockId,
-        advertiser: suggestion.advertiser,
-        iab_category: suggestion.iabCategory,
       };
+      if (suggestion.is_sponsored) {
+        desktopSuggestion.impression_url = suggestion.impressionUrl;
+        desktopSuggestion.click_url = suggestion.clickUrl;
+        desktopSuggestion.block_id = suggestion.blockId;
+        desktopSuggestion.advertiser = suggestion.advertiser;
+        desktopSuggestion.iab_category = suggestion.iabCategory;
+      } else {
+        desktopSuggestion.advertiser = "Wikipedia";
+        desktopSuggestion.iab_category = "5 - Education";
+      }
+      suggestion = desktopSuggestion;
     }
 
     // Replace the suggestion's template substrings, but first save the original

browser/components/urlbar/tests/quicksuggest/MerinoTestUtils.sys.mjs

@@ -519,12 +519,12 @@ class MockMerinoServer {
             provider: "adm",
             full_keyword: "full_keyword",
             title: "title",
-            url: "url",
+            url: "http://example.com/amp",
             icon: null,
-            impression_url: "impression_url",
+            impression_url: "http://example.com/amp-impression",
-            click_url: "click_url",
+            click_url: "http://example.com/amp-click",
             block_id: 1,
-            advertiser: "advertiser",
+            advertiser: "amp",
             is_sponsored: true,
             score: 1,
           },

browser/components/urlbar/tests/quicksuggest/browser/browser_telemetry_nonsponsored.js

@@ -19,9 +19,7 @@ const REMOTE_SETTINGS_RESULT = {
   url: "https://example.com/nonsponsored",
   title: "Non-sponsored suggestion",
   keywords: ["nonsponsored"],
-  click_url: "https://example.com/click",
+  advertiser: "Wikipedia",
-  impression_url: "https://example.com/impression",
-  advertiser: "testadvertiser",
   iab_category: "5 - Education",
 };
 
@@ -40,8 +38,14 @@ add_setup(async function () {
   });
 });
 
-add_task(async function nonsponsored() {
+add_tasks_with_rust(async function nonsponsored() {
   let match_type = "firefox-suggest";
+  let advertiser = REMOTE_SETTINGS_RESULT.advertiser.toLowerCase();
+  let reporting_url = undefined;
+  let source = UrlbarPrefs.get("quicksuggest.rustEnabled")
+    ? "rust"
+    : "remote-settings";
+  let block_id = source == "rust" ? undefined : REMOTE_SETTINGS_RESULT.id;
 
   // Make sure `improve_suggest_experience_checked` is recorded correctly
   // depending on the value of the related pref.
@@ -75,14 +79,16 @@ add_task(async function nonsponsored() {
         ping: {
           type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
           payload: {
+            source,
             match_type,
             position,
+            block_id,
+            advertiser,
+            reporting_url,
             suggested_index: -1,
             suggested_index_relative_to_group: true,
             improve_suggest_experience_checked,
             is_clicked: false,
-            block_id: REMOTE_SETTINGS_RESULT.id,
-            advertiser: REMOTE_SETTINGS_RESULT.advertiser,
           },
         },
       },
@@ -106,26 +112,30 @@ add_task(async function nonsponsored() {
           {
             type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
             payload: {
+              source,
               match_type,
               position,
+              block_id,
+              advertiser,
+              reporting_url,
               suggested_index: -1,
               suggested_index_relative_to_group: true,
               improve_suggest_experience_checked,
               is_clicked: true,
-              block_id: REMOTE_SETTINGS_RESULT.id,
-              advertiser: REMOTE_SETTINGS_RESULT.advertiser,
             },
           },
           {
             type: CONTEXTUAL_SERVICES_PING_TYPES.QS_SELECTION,
             payload: {
+              source,
               match_type,
               position,
+              block_id,
+              advertiser,
+              reporting_url,
               suggested_index: -1,
               suggested_index_relative_to_group: true,
               improve_suggest_experience_checked,
-              block_id: REMOTE_SETTINGS_RESULT.id,
-              advertiser: REMOTE_SETTINGS_RESULT.advertiser,
             },
           },
         ],
@@ -152,26 +162,29 @@ add_task(async function nonsponsored() {
             {
               type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
               payload: {
+                source,
                 match_type,
                 position,
+                block_id,
+                advertiser,
+                reporting_url,
                 suggested_index: -1,
                 suggested_index_relative_to_group: true,
                 improve_suggest_experience_checked,
                 is_clicked: false,
-                block_id: REMOTE_SETTINGS_RESULT.id,
-                advertiser: REMOTE_SETTINGS_RESULT.advertiser,
               },
             },
             {
               type: CONTEXTUAL_SERVICES_PING_TYPES.QS_BLOCK,
               payload: {
+                source,
                 match_type,
                 position,
+                block_id,
+                advertiser,
                 suggested_index: -1,
                 suggested_index_relative_to_group: true,
                 improve_suggest_experience_checked,
-                block_id: REMOTE_SETTINGS_RESULT.id,
-                advertiser: REMOTE_SETTINGS_RESULT.advertiser,
                 iab_category: REMOTE_SETTINGS_RESULT.iab_category,
               },
             },
@@ -198,14 +211,16 @@ add_task(async function nonsponsored() {
             {
               type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
               payload: {
+                source,
                 match_type,
                 position,
+                block_id,
+                advertiser,
+                reporting_url,
                 suggested_index: -1,
                 suggested_index_relative_to_group: true,
                 improve_suggest_experience_checked,
                 is_clicked: false,
-                block_id: REMOTE_SETTINGS_RESULT.id,
-                advertiser: REMOTE_SETTINGS_RESULT.advertiser,
               },
             },
           ],

browser/components/urlbar/tests/quicksuggest/browser/browser_telemetry_sponsored.js

@@ -22,6 +22,7 @@ const REMOTE_SETTINGS_RESULT = {
   click_url: "https://example.com/click",
   impression_url: "https://example.com/impression",
   advertiser: "testadvertiser",
+  iab_category: "22 - Shopping",
 };
 
 const suggestion_type = "sponsored";
@@ -40,8 +41,11 @@ add_setup(async function () {
 });
 
 // sponsored
-add_task(async function sponsored() {
+add_tasks_with_rust(async function sponsored() {
   let match_type = "firefox-suggest";
+  let source = UrlbarPrefs.get("quicksuggest.rustEnabled")
+    ? "rust"
+    : "remote-settings";
 
   // Make sure `improve_suggest_experience_checked` is recorded correctly
   // depending on the value of the related pref.
@@ -75,6 +79,7 @@ add_task(async function sponsored() {
         ping: {
           type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
           payload: {
+            source,
             match_type,
             position,
             suggested_index: -1,
@@ -106,6 +111,7 @@ add_task(async function sponsored() {
           {
             type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
             payload: {
+              source,
               match_type,
               position,
               suggested_index: -1,
@@ -119,6 +125,7 @@ add_task(async function sponsored() {
           {
             type: CONTEXTUAL_SERVICES_PING_TYPES.QS_SELECTION,
             payload: {
+              source,
               match_type,
               position,
               suggested_index: -1,
@@ -152,6 +159,7 @@ add_task(async function sponsored() {
             {
               type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
               payload: {
+                source,
                 match_type,
                 position,
                 suggested_index: -1,
@@ -165,6 +173,7 @@ add_task(async function sponsored() {
             {
               type: CONTEXTUAL_SERVICES_PING_TYPES.QS_BLOCK,
               payload: {
+                source,
                 match_type,
                 position,
                 suggested_index: -1,
@@ -198,6 +207,7 @@ add_task(async function sponsored() {
             {
               type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
               payload: {
+                source,
                 match_type,
                 position,
                 suggested_index: -1,
@@ -217,8 +227,12 @@ add_task(async function sponsored() {
 });
 
 // higher-placement sponsored, a.k.a sponsored priority, sponsored best match
-add_task(async function sponsoredBestMatch() {
+add_tasks_with_rust(async function sponsoredBestMatch() {
   let match_type = "best-match";
+  let source = UrlbarPrefs.get("quicksuggest.rustEnabled")
+    ? "rust"
+    : "remote-settings";
+
   await SpecialPowers.pushPrefEnv({
     set: [["browser.urlbar.quicksuggest.sponsoredPriority", true]],
   });
@@ -243,6 +257,7 @@ add_task(async function sponsoredBestMatch() {
       ping: {
         type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
         payload: {
+          source,
           match_type,
           position,
           suggested_index: 1,
@@ -274,6 +289,7 @@ add_task(async function sponsoredBestMatch() {
         {
           type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
           payload: {
+            source,
             match_type,
             position,
             suggested_index: 1,
@@ -287,6 +303,7 @@ add_task(async function sponsoredBestMatch() {
         {
           type: CONTEXTUAL_SERVICES_PING_TYPES.QS_SELECTION,
           payload: {
+            source,
             match_type,
             position,
             suggested_index: 1,
@@ -320,6 +337,7 @@ add_task(async function sponsoredBestMatch() {
           {
             type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
             payload: {
+              source,
               match_type,
               position,
               suggested_index: 1,
@@ -333,6 +351,7 @@ add_task(async function sponsoredBestMatch() {
           {
             type: CONTEXTUAL_SERVICES_PING_TYPES.QS_BLOCK,
             payload: {
+              source,
               match_type,
               position,
               suggested_index: 1,
@@ -366,6 +385,7 @@ add_task(async function sponsoredBestMatch() {
           {
             type: CONTEXTUAL_SERVICES_PING_TYPES.QS_IMPRESSION,
             payload: {
+              source,
               match_type,
               position,
               suggested_index: 1,

browser/components/urlbar/tests/quicksuggest/browser/head.js

@@ -117,9 +117,6 @@ async function setUpTelemetryTest({
   await PlacesUtils.bookmarks.eraseEverything();
   await UrlbarTestUtils.formHistory.clear();
 
-  Services.telemetry.clearScalars();
-  Services.telemetry.clearEvents();
-
   // Add a mock engine so we don't hit the network.
   await SearchTestUtils.installSearchExtension({}, { setAsDefault: true });
 
@@ -200,6 +197,9 @@ async function doTelemetryTest({
       fireInputEvent: true,
     }),
 }) {
+  Services.telemetry.clearScalars();
+  Services.telemetry.clearEvents();
+
   await doImpressionOnlyTest({
     index,
     suggestion,
@@ -624,6 +624,8 @@ function watchGleanPings(pings) {
 function _assertGleanPing(ping) {
   Assert.equal(Glean.quickSuggest.pingType.testGetValue(), ping.type);
   const keymap = {
+    // present in all pings
+    source: Glean.quickSuggest.source,
     match_type: Glean.quickSuggest.matchType,
     position: Glean.quickSuggest.position,
     suggested_index: Glean.quickSuggest.suggestedIndex,
@@ -631,13 +633,64 @@ function _assertGleanPing(ping) {
       Glean.quickSuggest.suggestedIndexRelativeToGroup,
     improve_suggest_experience_checked:
       Glean.quickSuggest.improveSuggestExperience,
-    is_clicked: Glean.quickSuggest.isClicked,
     block_id: Glean.quickSuggest.blockId,
     advertiser: Glean.quickSuggest.advertiser,
+    request_id: Glean.quickSuggest.requestId,
+    context_id: Glean.quickSuggest.contextId,
+    // impression and click pings
+    reporting_url: Glean.quickSuggest.reportingUrl,
+    // impression ping
+    is_clicked: Glean.quickSuggest.isClicked,
+    // block/dismiss ping
     iab_category: Glean.quickSuggest.iabCategory,
   };
   for (const [key, value] of Object.entries(ping.payload)) {
     Assert.ok(key in keymap, `A Glean metric exists for field ${key}`);
-    Assert.equal(value ?? null, keymap[key].testGetValue());
+    Assert.equal(
+      keymap[key].testGetValue(),
+      value ?? null,
+      `Glean metric field ${key} should be the expected value`
+    );
+  }
+}
+
+/**
+ * Adds two tasks: One with the Rust backend disabled and one with it enabled.
+ * The names of the task functions will be the name of the passed-in task
+ * function appended with "_rustDisabled" and "_rustEnabled" respectively. Call
+ * with the usual `add_task()` arguments.
+ *
+ * @param {...any} args
+ *   The usual `add_task()` arguments.
+ */
+function add_tasks_with_rust(...args) {
+  let taskFnIndex = args.findIndex(a => typeof a == "function");
+  let taskFn = args[taskFnIndex];
+
+  for (let rustEnabled of [false, true]) {
+    let newTaskFn = async (...taskFnArgs) => {
+      info("Setting rustEnabled: " + rustEnabled);
+      UrlbarPrefs.set("quicksuggest.rustEnabled", rustEnabled);
+      info("Done setting rustEnabled: " + rustEnabled);
+
+      let rv;
+      try {
+        info("Calling original task function: " + taskFn.name);
+        rv = await taskFn(...taskFnArgs);
+      } finally {
+        info("Done calling original task function: " + taskFn.name);
+        info("Clearing rustEnabled");
+        UrlbarPrefs.clear("quicksuggest.rustEnabled");
+        info("Done clearing rustEnabled");
+      }
+      return rv;
+    };
+
+    Object.defineProperty(newTaskFn, "name", {
+      value: taskFn.name + (rustEnabled ? "_rustEnabled" : "_rustDisabled"),
+    });
+    let addTaskArgs = [...args];
+    addTaskArgs[taskFnIndex] = newTaskFn;
+    add_task(...addTaskArgs);
   }
 }

browser/components/urlbar/tests/quicksuggest/unit/head.js

@@ -95,6 +95,8 @@ function makeWikipediaResult({
       displayUrl: url.replace(/^https:\/\//, ""),
       isSponsored: false,
       qsSuggestion: keyword,
+      sponsoredAdvertiser: "Wikipedia",
+      sponsoredIabCategory: "5 - Education",
       helpUrl: QuickSuggest.HELP_URL,
       helpL10n: {
         id: "urlbar-result-menu-learn-more-about-firefox-suggest",

browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_merino.js

@@ -64,13 +64,13 @@ const EXPECTED_MERINO_URLBAR_RESULT = {
     telemetryType: "adm_sponsored",
     qsSuggestion: "full_keyword",
     title: "title",
-    url: "url",
+    url: "http://example.com/amp",
-    originalUrl: "url",
+    originalUrl: "http://example.com/amp",
     icon: null,
-    sponsoredImpressionUrl: "impression_url",
+    sponsoredImpressionUrl: "http://example.com/amp-impression",
-    sponsoredClickUrl: "click_url",
+    sponsoredClickUrl: "http://example.com/amp-click",
     sponsoredBlockId: 1,
-    sponsoredAdvertiser: "advertiser",
+    sponsoredAdvertiser: "amp",
     isSponsored: true,
     descriptionL10n: { id: "urlbar-result-action-sponsored" },
     helpUrl: QuickSuggest.HELP_URL,
@@ -81,7 +81,7 @@ const EXPECTED_MERINO_URLBAR_RESULT = {
     blockL10n: {
       id: "urlbar-result-menu-dismiss-firefox-suggest",
     },
-    displayUrl: "url",
+    displayUrl: "http://example.com/amp",
     requestId: "request_id",
     source: "merino",
     provider: "adm",