Backed out changeset aaee4ba7af2e (bug 1664849) for failures on test_urlTelemetry.js. CLOSED TREE

2020-11-20 14:32:42 +02:00 · 2020-11-20 14:32:42 +02:00 · 1fe4bfc1b7
--- a/browser/components/search/SearchTelemetry.jsm
+++ b/browser/components/search/SearchTelemetry.jsm
@ -199,16 +199,19 @@ class TelemetryHandler {

    this._reportSerpPage(info, url);

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

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

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

      let URL = channel.finalURL;
-      let info = this._getProviderInfoForURL(URL, true);
+      info = this._getProviderInfoForURL(URL, true);
      if (!info) {
        return;
      }
@ -710,7 +713,7 @@ class ContentHandler {
      try {
        Services.telemetry.keyedScalarAdd(
          SEARCH_AD_CLICKS_SCALAR,
-          `${info.telemetryId}:${item.info.code ? "sap" : "organic"}`,
+          info.telemetryId,
          1
        );
        channel._adClickRecorded = true;
@ -745,12 +748,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.code ? "sap" : "organic"}`,
+      item.info.provider,
      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
@ -1,21 +0,0 @@
-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
@ -1,42 +0,0 @@
-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,7 +21,5 @@ 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 Cookies",
+      "title": "Follow-on Cookes",
      "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, organic = false) {
-  return `${page}?s=test${organic ? "" : "&abc=ff"}`;
+function getSERPUrl(page) {
+  return page + "?s=test&abc=ff";
 }

 function getSERPFollowOnUrl(page) {
@ -202,26 +202,7 @@ add_task(async function test_track_ad() {
  await assertTelemetry(
    { "example.in-content:sap:ff": 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 },
+      "browser.search.with_ads": { example: 1 },
    }
  );

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

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

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

-async function track_ad_click(testOrganic) {
+add_task(async function test_track_ad_click() {
  // 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), testOrganic)
+    getSERPUrl(getPageUrl(false, true))
  );

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

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

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

@ -331,10 +307,10 @@ async function track_ad_click(testOrganic) {

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

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

  await assertTelemetry(
-    { [expectedHistogramKey]: 2 },
+    { "example.in-content:sap:ff": 2 },
    {
-      "browser.search.with_ads": { [expectedScalarKey]: 2 },
-      "browser.search.ad_clicks": { [expectedScalarKey]: 2 },
+      "browser.search.with_ads": { example: 2 },
+      "browser.search.ad_clicks": { example: 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() {
@ -374,7 +342,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:sap": 1 },
+      "browser.search.with_ads": { example: 1 },
    }
  );

@ -394,8 +362,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:sap": 1 },
-      "browser.search.ad_clicks": { "example:sap": 1 },
+      "browser.search.with_ads": { example: 1 },
+      "browser.search.ad_clicks": { example: 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,15 +4749,14 @@ browser.search:
    bug_numbers:
      - 1495548
      - 1505411
-      - 1664849
    description: >
-      Records counts of SERP pages with adverts displayed. The key format is ‘<provider>:<sap|organic>’.
+      Records counts of SERP pages with adverts displayed. The key format is ‘<engine-name>’.
    expires: never
    keyed: true
    kind: uint
    notification_emails:
      - fx-search@mozilla.com
-      - teon@mozilla.com
+      - adw@mozilla.com
    release_channel_collection: opt-out
    products:
      - 'firefox'
@ -4769,15 +4768,14 @@ browser.search:
    bug_numbers:
      - 1495548
      - 1505411
-      - 1664849
    description: >
-      Records clicks of adverts on SERP pages. The key format is ‘<provider>:<sap|organic>’.
+      Records clicks of adverts on SERP pages. The key format is ‘<engine-name>’.
    expires: never
    keyed: true
    kind: uint
    notification_emails:
      - fx-search@mozilla.com
-      - teon@mozilla.com
+      - adw@mozilla.com
    release_channel_collection: opt-out
    products:
      - 'firefox'