Bug 1664849 - Extend ad impression/click telemetry to organic SERPS. r=daleharvey

Differential Revision: https://phabricator.services.mozilla.com/D97493
2020-11-20 10:25:48 +00:00 · 2020-11-20 10:25:48 +00:00 · 3d69c205ed
--- a/browser/components/search/SearchTelemetry.jsm
+++ b/browser/components/search/SearchTelemetry.jsm
@ -199,19 +199,16 @@ class TelemetryHandler {

    this._reportSerpPage(info, url);

-    // If we have a code, then we also track this for potential ad clicks.
-    if (info.code) {
-      let item = this._browserInfoByURL.get(url);
-      if (item) {
-        item.browsers.add(browser);
-        item.count++;
-      } else {
-        this._browserInfoByURL.set(url, {
-          browsers: new WeakSet([browser]),
-          info,
-          count: 1,
-        });
-      }
+    let item = this._browserInfoByURL.get(url);
+    if (item) {
+      item.browsers.add(browser);
+      item.count++;
+    } else {
+      this._browserInfoByURL.set(url, {
+        browsers: new WeakSet([browser]),
+        info,
+        count: 1,
+      });
    }
  }

@ -699,13 +696,13 @@ class ContentHandler {
      }

      let originURL = channel.originURI && channel.originURI.spec;
-      let info = this._findBrowserItemForURL(originURL);
-      if (!originURL || !info) {
+      let item = this._findBrowserItemForURL(originURL);
+      if (!originURL || !item) {
        return;
      }

      let URL = channel.finalURL;
-      info = this._getProviderInfoForURL(URL, true);
+      let info = this._getProviderInfoForURL(URL, true);
      if (!info) {
        return;
      }
@ -713,7 +710,7 @@ class ContentHandler {
      try {
        Services.telemetry.keyedScalarAdd(
          SEARCH_AD_CLICKS_SCALAR,
-          info.telemetryId,
+          `${info.telemetryId}:${item.info.code ? "sap" : "organic"}`,
          1
        );
        channel._adClickRecorded = true;
@ -748,12 +745,12 @@ class ContentHandler {
      return;
    }

+    logConsole.debug("Counting ads in page for", item.info.provider, info.url);
    Services.telemetry.keyedScalarAdd(
      SEARCH_WITH_ADS_SCALAR,
-      item.info.provider,
+      `${item.info.provider}:${item.info.code ? "sap" : "organic"}`,
      1
    );
-    logConsole.debug("Counting ads in page for", item.info.provider, info.url);
  }
 }

--- a/browser/components/search/docs/index.rst
+++ b/browser/components/search/docs/index.rst
@ -0,0 +1,21 @@
+Search
+======
+
+This document describes the implementation of parts of Firefox's search interfaces.
+
+The search area covers:
+
+  * Search bar on the toolbar
+  * In-content search
+  * One-off search buttons on both the search and address bars
+
+Search Engine handling is taken care of with the `toolkit Search Service`_.
+
+Most of the search code lives in `browser/components/search`_.
+
+.. toctree::
+
+   telemetry
+
+.. _toolkit Search Service: /toolkit/search/index.html
+.. _browser/components/search: https://searchfox.org/mozilla-central/source/browser/components/search
--- a/browser/components/search/docs/telemetry.rst
+++ b/browser/components/search/docs/telemetry.rst
@ -0,0 +1,42 @@
+Telemetry
+=========
+
+This section describes existing telemetry probes measuring interaction with
+search engines.
+
+Note: Some search related probes are also documented on the `address bar telemetry`_
+page.
+
+.. toctree::
+   :caption: Table of Contents
+
+   telemetry
+
+Search probes relevant to front-end searches
+--------------------------------------------
+
+Definitions:
+
+  * ``organic`` is a search that a user performs by visiting a search engine
+    directly.
+  * ``sap`` (search access point) is a search that a user performs by visiting
+    via one of Firefox's access points, using the associated partner codes.
+  * ``sap-follow-on`` is a SAP search where the user has first accessed the site
+    via a SAP, and then performed an additional search.
+  * ``SERP`` refers to a search engine result page.
+
+SEARCH_COUNTS
+  This histogram records search counts for visits to SERP in-content pages. It
+  also stores other items - see `address bar telemetry`_. For in-content
+  searches, the format is
+  ``<provider>.in-content:[sap|sap-follow-on|organic]:[code|none]``.
+
+browser.search.with_ads
+  This keyed scalar records counts of SERP pages with adverts displayed.
+  The key format is ``<provider>:<sap|organic>``.
+
+browser.search.ad_clicks
+  Records clicks of adverts on SERP pages. The key format is
+  ``<provider>:<sap|organic>``.
+
+.. _address bar telemetry: /browser/urlbar/telemetry.html
--- a/browser/components/search/moz.build
+++ b/browser/components/search/moz.build
@ -21,5 +21,7 @@ XPCSHELL_TESTS_MANIFESTS += ["test/unit/xpcshell.ini"]

 JAR_MANIFESTS += ["jar.mn"]

+SPHINX_TREES["/browser/search"] = "docs"
+
 with Files("**"):
    BUG_COMPONENT = ("Firefox", "Search")
--- a/browser/components/search/schema/search-telemetry-schema.json
+++ b/browser/components/search/schema/search-telemetry-schema.json
@ -47,7 +47,7 @@
    },
    "followOnCookies": {
      "type": "array",
-      "title": "Follow-on Cookes",
+      "title": "Follow-on Cookies",
      "description": "An array of cookie details that are used to identify follow-on searches.",
      "items": {
        "type": "object",
--- a/browser/components/search/test/browser/browser_searchTelemetry.js
+++ b/browser/components/search/test/browser/browser_searchTelemetry.js
@ -37,8 +37,8 @@ function getPageUrl(useExample = false, useAdPage = false) {
  return `http://${server}/browser/browser/components/search/test/browser/${page}`;
 }

-function getSERPUrl(page) {
-  return page + "?s=test&abc=ff";
+function getSERPUrl(page, organic = false) {
+  return `${page}?s=test${organic ? "" : "&abc=ff"}`;
 }

 function getSERPFollowOnUrl(page) {
@ -202,7 +202,26 @@ add_task(async function test_track_ad() {
  await assertTelemetry(
    { "example.in-content:sap:ff": 1 },
    {
-      "browser.search.with_ads": { example: 1 },
+      "browser.search.with_ads": { "example:sap": 1 },
+    }
+  );
+
+  BrowserTestUtils.removeTab(tab);
+});
+
+add_task(async function test_track_ad_organic() {
+  Services.telemetry.clearScalars();
+  searchCounts.clear();
+
+  let tab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    getSERPUrl(getPageUrl(false, true), true)
+  );
+
+  await assertTelemetry(
+    { "example.in-content:organic:none": 1 },
+    {
+      "browser.search.with_ads": { "example:organic": 1 },
    }
  );

@ -226,7 +245,7 @@ add_task(async function test_track_ad_new_window() {
  await assertTelemetry(
    { "example.in-content:sap:ff": 1 },
    {
-      "browser.search.with_ads": { example: 1 },
+      "browser.search.with_ads": { "example:sap": 1 },
    }
  );

@ -256,7 +275,7 @@ add_task(async function test_track_ad_pages_without_ads() {
  await assertTelemetry(
    { "example.in-content:sap:ff": 2 },
    {
-      "browser.search.with_ads": { example: 1 },
+      "browser.search.with_ads": { "example:sap": 1 },
    }
  );

@ -265,20 +284,25 @@ add_task(async function test_track_ad_pages_without_ads() {
  }
 });

-add_task(async function test_track_ad_click() {
+async function track_ad_click(testOrganic) {
  // Note: the above tests have already checked a page with no ad-urls.
  searchCounts.clear();
  Services.telemetry.clearScalars();

+  let expectedScalarKey = `example:${testOrganic ? "organic" : "sap"}`;
+  let expectedHistogramKey = `example.in-content:${
+    testOrganic ? "organic:none" : "sap:ff"
+  }`;
+
  let tab = await BrowserTestUtils.openNewForegroundTab(
    gBrowser,
-    getSERPUrl(getPageUrl(false, true))
+    getSERPUrl(getPageUrl(false, true), testOrganic)
  );

  await assertTelemetry(
-    { "example.in-content:sap:ff": 1 },
+    { [expectedHistogramKey]: 1 },
    {
-      "browser.search.with_ads": { example: 1 },
+      "browser.search.with_ads": { [expectedScalarKey]: 1 },
    }
  );

@ -291,10 +315,10 @@ add_task(async function test_track_ad_click() {
  await new Promise(resolve => setTimeout(resolve, ADLINK_CHECK_TIMEOUT_MS));

  await assertTelemetry(
-    { "example.in-content:sap:ff": 1 },
+    { [expectedHistogramKey]: 1 },
    {
-      "browser.search.with_ads": { example: 1 },
-      "browser.search.ad_clicks": { example: 1 },
+      "browser.search.with_ads": { [expectedScalarKey]: 1 },
+      "browser.search.ad_clicks": { [expectedScalarKey]: 1 },
    }
  );

@ -307,10 +331,10 @@ add_task(async function test_track_ad_click() {

  // We've gone back, so we register an extra display & if it is with ads or not.
  await assertTelemetry(
-    { "example.in-content:sap:ff": 2 },
+    { [expectedHistogramKey]: 2 },
    {
-      "browser.search.with_ads": { example: 2 },
-      "browser.search.ad_clicks": { example: 1 },
+      "browser.search.with_ads": { [expectedScalarKey]: 2 },
+      "browser.search.ad_clicks": { [expectedScalarKey]: 1 },
    }
  );

@ -323,14 +347,22 @@ add_task(async function test_track_ad_click() {
  await new Promise(resolve => setTimeout(resolve, ADLINK_CHECK_TIMEOUT_MS));

  await assertTelemetry(
-    { "example.in-content:sap:ff": 2 },
+    { [expectedHistogramKey]: 2 },
    {
-      "browser.search.with_ads": { example: 2 },
-      "browser.search.ad_clicks": { example: 2 },
+      "browser.search.with_ads": { [expectedScalarKey]: 2 },
+      "browser.search.ad_clicks": { [expectedScalarKey]: 2 },
    }
  );

  BrowserTestUtils.removeTab(tab);
+}
+
+add_task(async function test_track_ad_click() {
+  await track_ad_click(false);
+});
+
+add_task(async function test_track_ad_click_organic() {
+  await track_ad_click(true);
 });

 add_task(async function test_track_ad_click_with_location_change_other_tab() {
@ -342,7 +374,7 @@ add_task(async function test_track_ad_click_with_location_change_other_tab() {
  await assertTelemetry(
    { "example.in-content:sap:ff": 1 },
    {
-      "browser.search.with_ads": { example: 1 },
+      "browser.search.with_ads": { "example:sap": 1 },
    }
  );

@ -362,8 +394,8 @@ add_task(async function test_track_ad_click_with_location_change_other_tab() {
  await assertTelemetry(
    { "example.in-content:sap:ff": 1 },
    {
-      "browser.search.with_ads": { example: 1 },
-      "browser.search.ad_clicks": { example: 1 },
+      "browser.search.with_ads": { "example:sap": 1 },
+      "browser.search.ad_clicks": { "example:sap": 1 },
    }
  );

--- a/browser/docs/index.rst
+++ b/browser/docs/index.rst
@ -15,9 +15,9 @@ This is the nascent documentation of the Firefox front-end code.
   installer/windows/installer/index
   /toolkit/mozapps/defaultagent/default-browser-agent/index
   components/newtab/content-src/asrouter/docs/index
+   search/index
   base/sslerrorreport/index
   base/tabbrowser/index
   touchbar/index
   components/uitour/docs/index
   components/payments/docs/index
-
--- a/toolkit/components/telemetry/Scalars.yaml
+++ b/toolkit/components/telemetry/Scalars.yaml
@ -4749,14 +4749,15 @@ browser.search:
    bug_numbers:
      - 1495548
      - 1505411
+      - 1664849
    description: >
-      Records counts of SERP pages with adverts displayed. The key format is ‘<engine-name>’.
+      Records counts of SERP pages with adverts displayed. The key format is ‘<provider>:<sap|organic>’.
    expires: never
    keyed: true
    kind: uint
    notification_emails:
      - fx-search@mozilla.com
-      - adw@mozilla.com
+      - teon@mozilla.com
    release_channel_collection: opt-out
    products:
      - 'firefox'
@ -4768,14 +4769,15 @@ browser.search:
    bug_numbers:
      - 1495548
      - 1505411
+      - 1664849
    description: >
-      Records clicks of adverts on SERP pages. The key format is ‘<engine-name>’.
+      Records clicks of adverts on SERP pages. The key format is ‘<provider>:<sap|organic>’.
    expires: never
    keyed: true
    kind: uint
    notification_emails:
      - fx-search@mozilla.com
-      - adw@mozilla.com
+      - teon@mozilla.com
    release_channel_collection: opt-out
    products:
      - 'firefox'