bug 1553550 - removing expiring intermediate preloading telemetry r=jcj
authorDana Keeler <dkeeler@mozilla.com>
Wed, 03 Jul 2019 16:49:18 +0000
changeset 540794 17183959c3a91f478dd3426df4439bb1d714f28a
parent 540793 be232271fcde16cffc84ef2f24a8e849302c5991
child 540795 98eb0b753429a9da45154a18c3779e7840c72619
push id11529
push userarchaeopteryx@coole-files.de
push dateThu, 04 Jul 2019 15:22:33 +0000
treeherdermozilla-beta@ebb510a784b8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs1553550
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1553550 - removing expiring intermediate preloading telemetry r=jcj Differential Revision: https://phabricator.services.mozilla.com/D36516
security/manager/ssl/RemoteSecuritySettings.jsm
security/manager/ssl/tests/unit/test_intermediate_preloads.js
toolkit/components/telemetry/Histograms.json
toolkit/components/telemetry/Scalars.yaml
--- a/security/manager/ssl/RemoteSecuritySettings.jsm
+++ b/security/manager/ssl/RemoteSecuritySettings.jsm
@@ -18,21 +18,16 @@ const INTERMEDIATES_BUCKET_PREF         
 const INTERMEDIATES_CHECKED_SECONDS_PREF = "security.remote_settings.intermediates.checked";
 const INTERMEDIATES_COLLECTION_PREF      = "security.remote_settings.intermediates.collection";
 const INTERMEDIATES_DL_PER_POLL_PREF     = "security.remote_settings.intermediates.downloads_per_poll";
 const INTERMEDIATES_DL_PARALLEL_REQUESTS = "security.remote_settings.intermediates.parallel_downloads";
 const INTERMEDIATES_ENABLED_PREF         = "security.remote_settings.intermediates.enabled";
 const INTERMEDIATES_SIGNER_PREF          = "security.remote_settings.intermediates.signer";
 const LOGLEVEL_PREF                      = "browser.policies.loglevel";
 
-const INTERMEDIATES_ERRORS_TELEMETRY     = "INTERMEDIATE_PRELOADING_ERRORS";
-const INTERMEDIATES_PENDING_TELEMETRY    = "security.intermediate_preloading_num_pending";
-const INTERMEDIATES_PRELOADED_TELEMETRY  = "security.intermediate_preloading_num_preloaded";
-const INTERMEDIATES_UPDATE_MS_TELEMETRY  = "INTERMEDIATE_PRELOADING_UPDATE_TIME_MS";
-
 const ONECRL_BUCKET_PREF     = "services.settings.security.onecrl.bucket";
 const ONECRL_COLLECTION_PREF = "services.settings.security.onecrl.collection";
 const ONECRL_SIGNER_PREF     = "services.settings.security.onecrl.signer";
 const ONECRL_CHECKED_PREF    = "services.settings.security.onecrl.checked";
 
 const PINNING_ENABLED_PREF         = "services.blocklist.pinning.enabled";
 const PINNING_BUCKET_PREF          = "services.blocklist.pinning.bucket";
 const PINNING_COLLECTION_PREF      = "services.blocklist.pinning.collection";
@@ -215,19 +210,17 @@ const updateCertBlocklist = AppConstants
         if (item.issuerName && item.serialNumber) {
           certList.revokeCertByIssuerAndSerial(item.issuerName,
             item.serialNumber);
         } else if (item.subject && item.pubKeyHash) {
           certList.revokeCertBySubjectAndPubKey(item.subject,
             item.pubKeyHash);
         }
       } catch (e) {
-        // prevent errors relating to individual blocklist entries from
-        // causing sync to fail. We will accumulate telemetry on these failures in
-        // bug 1254099.
+        // Prevent errors relating to individual blocklist entries from causing sync to fail.
         Cu.reportError(e);
       }
     }
     certList.saveEntries();
   };
 
 /**
  * Modify the appropriate security pins based on records from the remote
@@ -259,19 +252,17 @@ async function updatePinningList({ data:
         }
         if (pinType == "STSPin") {
           siteSecurityService.setHSTSPreload(item.hostName,
             item.includeSubdomains,
             item.expires);
         }
       }
     } catch (e) {
-      // prevent errors relating to individual preload entries from causing
-      // sync to fail. We will accumulate telemetry for such failures in bug
-      // 1254099.
+      // Prevent errors relating to individual preload entries from causing sync to fail.
       Cu.reportError(e);
     }
   }
 }
 
 var RemoteSecuritySettings = {
   /**
    * Initialize the clients (cheap instantiation) and setup their sync event.
@@ -371,18 +362,16 @@ class IntermediatePreloads {
 
     log.debug(`There are ${waiting.length} intermediates awaiting download.`);
     if (waiting.length == 0) {
       // Nothing to do.
       Services.obs.notifyObservers(null, "remote-security-settings:intermediates-updated", "success");
       return;
     }
 
-    TelemetryStopwatch.start(INTERMEDIATES_UPDATE_MS_TELEMETRY);
-
     let toDownload = waiting.slice(0, maxDownloadsPerRun);
     let recordsCertsAndSubjects = [];
     for (let i = 0; i < toDownload.length; i += parallelDownloads) {
       const chunk = toDownload.slice(i, i + parallelDownloads);
       const downloaded = await Promise.all(chunk.map(record => this.maybeDownloadAttachment(record)));
       recordsCertsAndSubjects = recordsCertsAndSubjects.concat(downloaded);
     }
 
@@ -394,49 +383,35 @@ class IntermediatePreloads {
         recordsToUpdate.push(record);
       }
     }
     let result = await new Promise((resolve) => {
       certStorage.addCerts(certInfos, resolve);
     }).catch((err) => err);
     if (result != Cr.NS_OK) {
       Cu.reportError(`certStorage.addCerts failed: ${result}`);
-      Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("failedToUpdateDB");
       return;
     }
     await col.db.execute(transaction => {
       recordsToUpdate.forEach((record) => {
         transaction.update({ ...record, cert_import_complete: true });
       });
     });
-    const { data: finalCurrent } = await col.list();
-    const finalWaiting = finalCurrent.filter(record => !record.cert_import_complete);
-    const countPreloaded = finalCurrent.length - finalWaiting.length;
-
-    TelemetryStopwatch.finish(INTERMEDIATES_UPDATE_MS_TELEMETRY);
-    Services.telemetry.scalarSet(INTERMEDIATES_PRELOADED_TELEMETRY,
-                                  countPreloaded);
-    Services.telemetry.scalarSet(INTERMEDIATES_PENDING_TELEMETRY,
-                                  finalWaiting.length);
 
     Services.obs.notifyObservers(null, "remote-security-settings:intermediates-updated",
                                   "success");
   }
 
   async onObservePollEnd(subject, topic, data) {
     log.debug(`onObservePollEnd ${subject} ${topic}`);
 
     try {
       await this.updatePreloadedIntermediates();
     } catch (err) {
       log.warn(`Unable to update intermediate preloads: ${err}`);
-
-      Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("failedToObserve");
     }
   }
 
   // This method returns a promise to RemoteSettingsClient.maybeSync method.
   async onSync({ data: { current, created, updated, deleted } }) {
     if (!Services.prefs.getBoolPref(INTERMEDIATES_ENABLED_PREF, true)) {
       log.debug("Intermediate Preloading is disabled");
       return;
@@ -487,20 +462,16 @@ class IntermediatePreloads {
 
     return fetch(remoteFilePath, {
       headers,
       credentials: "omit",
     }).then(resp => {
       log.debug(`Download fetch completed: ${resp.ok} ${resp.status}`);
       if (!resp.ok) {
         Cu.reportError(`Failed to fetch ${remoteFilePath}: ${resp.status}`);
-
-        Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-          .add("failedToFetch");
-
         return Promise.reject();
       }
       return resp.arrayBuffer();
     })
     .then(buffer => new Uint8Array(buffer));
   }
 
   /**
@@ -519,50 +490,36 @@ class IntermediatePreloads {
     const {attachment: {hash, size}} = record;
     let result = { record, cert: null, subject: null };
 
     let attachmentData;
     try {
       attachmentData = await this._downloadAttachmentBytes(record);
     } catch (err) {
       Cu.reportError(`Failed to download attachment: ${err}`);
-      Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("failedToDownloadMisc");
       return result;
     }
 
     if (!attachmentData || attachmentData.length == 0) {
       // Bug 1519273 - Log telemetry for these rejections
       log.debug(`Empty attachment. Hash=${hash}`);
-
-      Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("emptyAttachment");
-
       return result;
     }
 
     // check the length
     if (attachmentData.length !== size) {
       log.debug(`Unexpected attachment length. Hash=${hash} Lengths ${attachmentData.length} != ${size}`);
-
-      Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("unexpectedLength");
-
       return result;
     }
 
     // check the hash
     let dataAsString = gTextDecoder.decode(attachmentData);
     let calculatedHash = getHash(dataAsString);
     if (calculatedHash !== hash) {
       log.warn(`Invalid hash. CalculatedHash=${calculatedHash}, Hash=${hash}, data=${dataAsString}`);
-
-      Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("unexpectedHash");
-
       return result;
     }
     log.debug(`downloaded cert with hash=${hash}, size=${size}`);
 
     let certBase64;
     let subjectBase64;
     try {
       // split off the header and footer
@@ -571,22 +528,16 @@ class IntermediatePreloads {
       let certBytes = stringToBytes(atob(certBase64));
       let cert = new X509.Certificate();
       cert.parse(certBytes);
       // get the DER-encoded subject and get a base64-encoded string from it
       // TODO(bug 1542028): add getters for _der and _bytes
       subjectBase64 = btoa(bytesToString(cert.tbsCertificate.subject._der._bytes));
     } catch (err) {
       Cu.reportError(`Failed to decode cert: ${err}`);
-
-      // Re-purpose the "failedToUpdateNSS" telemetry tag as "failed to
-      // decode preloaded intermediate certificate"
-      Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("failedToUpdateNSS");
-
       return result;
     }
     result.cert = certBase64;
     result.subject = subjectBase64;
     return result;
   }
 
   async maybeSync(expectedTimestamp, options) {
@@ -596,13 +547,11 @@ class IntermediatePreloads {
   async removeCerts(recordsToRemove) {
     let certStorage = Cc["@mozilla.org/security/certstorage;1"].getService(Ci.nsICertStorage);
     let hashes = recordsToRemove.map(record => record.derHash);
     let result = await new Promise((resolve) => {
         certStorage.removeCertsByHashes(hashes, resolve);
     }).catch((err) => err);
     if (result != Cr.NS_OK) {
       Cu.reportError(`Failed to remove some intermediate certificates`);
-      Services.telemetry.getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("failedToRemove");
     }
   }
 }
--- a/security/manager/ssl/tests/unit/test_intermediate_preloads.js
+++ b/security/manager/ssl/tests/unit/test_intermediate_preloads.js
@@ -5,17 +5,16 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 "use strict";
 do_get_profile(); // must be called before getting nsIX509CertDB
 
 const {RemoteSettings} = ChromeUtils.import("resource://services-settings/remote-settings.js");
 const {RemoteSecuritySettings} = ChromeUtils.import("resource://gre/modules/psm/RemoteSecuritySettings.jsm");
 const {TestUtils} = ChromeUtils.import("resource://testing-common/TestUtils.jsm");
-const {TelemetryTestUtils} = ChromeUtils.import("resource://testing-common/TelemetryTestUtils.jsm");
 const {X509} = ChromeUtils.import("resource://gre/modules/psm/X509.jsm");
 
 const {IntermediatePreloadsClient} = RemoteSecuritySettings.init();
 
 let server;
 
 let intermediate1Data;
 let intermediate2Data;
@@ -33,32 +32,16 @@ function getHashCommon(aStr, useBase64) 
   return hasher.finish(useBase64);
 }
 
 // Get a hexified SHA-256 hash of the given string.
 function getHash(aStr) {
   return hexify(getHashCommon(aStr, false));
 }
 
-function countTelemetryReports(histogram) {
-  let count = 0;
-  for (let x in histogram.values) {
-    count += histogram.values[x];
-  }
-  return count;
-}
-
-function clearTelemetry() {
-  Services.telemetry.getHistogramById("INTERMEDIATE_PRELOADING_ERRORS")
-    .clear();
-  Services.telemetry.getHistogramById("INTERMEDIATE_PRELOADING_UPDATE_TIME_MS")
-    .clear();
-  Services.telemetry.clearScalars();
-}
-
 function getSubjectBytes(certDERString) {
   let bytes = stringToArray(certDERString);
   let cert = new X509.Certificate();
   cert.parse(bytes);
   return arrayToString(cert.tbsCertificate.subject._der._bytes);
 }
 
 function getSPKIBytes(certDERString) {
@@ -171,30 +154,21 @@ add_task({
 });
 
 add_task({
     skip_if: () => !AppConstants.MOZ_NEW_CERT_STORAGE,
   }, async function test_preload_invalid_hash() {
   Services.prefs.setBoolPref(INTERMEDIATES_ENABLED_PREF, true);
   const invalidHash = "6e340b9cffb37a989ca544e6bb780a2c78901d3fb33738768511a30617afa01d";
 
-  clearTelemetry();
-
   const result = await syncAndDownload(["int.pem"], {
     hashFunc: () => invalidHash,
   });
   equal(result, "success", "Preloading update should have run");
 
-  let errors_histogram = Services.telemetry
-                          .getHistogramById("INTERMEDIATE_PRELOADING_ERRORS")
-                          .snapshot();
-
-  equal(countTelemetryReports(errors_histogram), 1, "There should be one error report");
-  equal(errors_histogram.values[7], 1, "There should be one invalid hash error");
-
   equal((await locallyDownloaded()).length, 0, "There should be no local entry");
 
   let certDB = Cc["@mozilla.org/security/x509certdb;1"]
                .getService(Ci.nsIX509CertDB);
 
   // load the first root and end entity, ignore the initial intermediate
   addCertFromFile(certDB, "test_intermediate_preloads/ca.pem", "CTu,,");
 
@@ -206,30 +180,21 @@ add_task({
                               certificateUsageSSLServer);
 });
 
 add_task({
     skip_if: () => !AppConstants.MOZ_NEW_CERT_STORAGE,
   }, async function test_preload_invalid_length() {
   Services.prefs.setBoolPref(INTERMEDIATES_ENABLED_PREF, true);
 
-  clearTelemetry();
-
   const result = await syncAndDownload(["int.pem"], {
     lengthFunc: () => 42,
   });
   equal(result, "success", "Preloading update should have run");
 
-  let errors_histogram = Services.telemetry
-                          .getHistogramById("INTERMEDIATE_PRELOADING_ERRORS")
-                          .snapshot();
-
-  equal(countTelemetryReports(errors_histogram), 1, "There should be only one error report");
-  equal(errors_histogram.values[8], 1, "There should be one invalid length error");
-
   equal((await locallyDownloaded()).length, 0, "There should be no local entry");
 
   let certDB = Cc["@mozilla.org/security/x509certdb;1"]
                .getService(Ci.nsIX509CertDB);
 
   // load the first root and end entity, ignore the initial intermediate
   addCertFromFile(certDB, "test_intermediate_preloads/ca.pem", "CTu,,");
 
@@ -318,38 +283,21 @@ add_task({
   Services.prefs.setBoolPref(INTERMEDIATES_ENABLED_PREF, true);
   Services.prefs.setIntPref(INTERMEDIATES_DL_PER_POLL_PREF, 100);
 
   const files = [];
   for (let i = 0; i < 200; i++) {
     files.push(["int.pem", "int2.pem"][i % 2]);
   }
 
-  clearTelemetry();
-
   let result = await syncAndDownload(files);
   equal(result, "success", "Preloading update should have run");
 
   equal((await locallyDownloaded()).length, 100, "There should have been only 100 downloaded");
 
-  const scalars = TelemetryTestUtils.getProcessScalars("parent");
-  TelemetryTestUtils.assertScalar(scalars, "security.intermediate_preloading_num_preloaded",
-                                  100, "Should have preloaded 100 certs");
-  TelemetryTestUtils.assertScalar(scalars, "security.intermediate_preloading_num_pending",
-                                  100, "Should report 100 pending");
-
-  let time_histogram = Services.telemetry
-                         .getHistogramById("INTERMEDIATE_PRELOADING_UPDATE_TIME_MS")
-                         .snapshot();
-  let errors_histogram = Services.telemetry
-                           .getHistogramById("INTERMEDIATE_PRELOADING_ERRORS")
-                           .snapshot();
-  equal(countTelemetryReports(time_histogram), 1, "Should report time once");
-  equal(countTelemetryReports(errors_histogram), 0, "There should be no error reports");
-
   // Re-run
   result = await syncAndDownload([], { clear: false });
   equal(result, "success", "Preloading update should have run");
 
   equal((await locallyDownloaded()).length, 200, "There should have been 200 downloaded");
 });
 
 add_task({
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -13614,43 +13614,16 @@
     "alert_emails": ["wpan@mozilla.com"],
     "expires_in_version": "60",
     "kind": "exponential",
     "high": 5000,
     "n_buckets": 100,
     "bug_numbers": [1341531],
     "description": "Time (ms) for the APZ handled wheel event to dispatch, but before handlers executing."
   },
-  "INTERMEDIATE_PRELOADING_ERRORS": {
-    "record_in_processes": ["main"],
-    "alert_emails": ["seceng-telemetry@mozilla.com", "jjones@mozilla.com"],
-    "expires_in_version": "70",
-    "kind": "categorical",
-    "labels": ["emptyAttachment",
-               "failedToDownloadMisc",
-               "failedToFetch",
-               "failedToObserve",
-               "failedToRemove",
-               "failedToUpdateNSS",
-               "failedToUpdateDB",
-               "unexpectedHash",
-               "unexpectedLength"],
-    "bug_numbers": [1519273],
-    "description": "Errors occurred during intermediate certificate preloading."
-  },
-  "INTERMEDIATE_PRELOADING_UPDATE_TIME_MS": {
-    "record_in_processes": ["main"],
-    "alert_emails": ["seceng-telemetry@mozilla.com", "jjones@mozilla.com"],
-    "expires_in_version": "70",
-    "kind": "exponential",
-    "high": 180000,
-    "n_buckets": 50,
-    "bug_numbers": [1519273],
-    "description": "Number of milliseconds to update the pre-loaded data, if updated."
-  },
   "IPC_SYNC_MESSAGE_MANAGER_LATENCY_MS": {
     "record_in_processes": ["main", "content"],
     "alert_emails": ["nika@thelayzells.com"],
     "bug_numbers": [1348113],
     "expires_in_version": "70",
     "kind": "exponential",
     "low": 32,
     "high": 750,
--- a/toolkit/components/telemetry/Scalars.yaml
+++ b/toolkit/components/telemetry/Scalars.yaml
@@ -465,40 +465,16 @@ security:
     kind: uint
     keyed: true
     notification_emails:
       - seceng-telemetry@mozilla.com
       - jjones@mozilla.com
     release_channel_collection: opt-out
     record_in_processes:
       - main
-  intermediate_preloading_num_preloaded:
-    bug_numbers:
-      - 1519273
-    description: >
-      Number of intermediate certificates pre-loaded.
-    expires: "70"
-    kind: uint
-    notification_emails:
-      - seceng-telemetry@mozilla.com
-      - jjones@mozilla.com
-    record_in_processes:
-      - main
-  intermediate_preloading_num_pending:
-    bug_numbers:
-      - 1519273
-    description: >
-      Number of intermediate certificates pending pre-load.
-    expires: "70"
-    kind: uint
-    notification_emails:
-      - seceng-telemetry@mozilla.com
-      - jjones@mozilla.com
-    record_in_processes:
-      - main
   client_cert:
     bug_numbers:
       - 1255120
     description: >
       Counts of events related to client certificates this session. Currently:
       "requested" (a server requested a client certificate), "sent" (a client
       certificate was sent), "compat" (a compatibility workaround was required).
     expires: "66"