Bug 1467929: Only collect browser JS errors from recent builds. draft
authorMichael Kelly <mkelly@mozilla.com>
Tue, 12 Jun 2018 21:49:58 -0700
changeset 807448 b546c0da58f944ef7da716386599d52d81954055
parent 806951 49efc43b14387db6c68d70bbf5e9b8195dd7f80f
push id113113
push userbmo:mkelly@mozilla.com
push dateThu, 14 Jun 2018 17:07:21 +0000
bugs1467929
milestone62.0a1
Bug 1467929: Only collect browser JS errors from recent builds. Telemetry data suggests between 13%-40% of errors being collected are from builds older than a week. Since Nightly updates twice a day, errors from builds that old don't reflect the current state of Nightly, so we can ignore them and save some bandwidth. A build is considered "recent" if the date encoded at the start of the appBuildId (YYYYMMDD) is within 7 days of the current date. Since this is mostly for preventing high load on the collection service, the check does not handle problems with the local clock being inaccurate in order to simplify the implementation. MozReview-Commit-ID: BbCO4kaBprL
browser/modules/BrowserErrorReporter.jsm
browser/modules/test/browser/browser_BrowserErrorReporter.js
--- a/browser/modules/BrowserErrorReporter.jsm
+++ b/browser/modules/BrowserErrorReporter.jsm
@@ -17,16 +17,17 @@ var EXPORTED_SYMBOLS = ["BrowserErrorRep
 const CONTEXT_LINES = 5;
 const ERROR_PREFIX_RE = /^[^\W]+:/m;
 const PREF_ENABLED = "browser.chrome.errorReporter.enabled";
 const PREF_LOG_LEVEL = "browser.chrome.errorReporter.logLevel";
 const PREF_PROJECT_ID = "browser.chrome.errorReporter.projectId";
 const PREF_PUBLIC_KEY = "browser.chrome.errorReporter.publicKey";
 const PREF_SAMPLE_RATE = "browser.chrome.errorReporter.sampleRate";
 const PREF_SUBMIT_URL = "browser.chrome.errorReporter.submitUrl";
+const RECENT_BUILD_AGE = 1000 * 60 * 60 * 24 * 7; // 7 days
 const SDK_NAME = "firefox-error-reporter";
 const SDK_VERSION = "1.0.0";
 const TELEMETRY_ERROR_COLLECTED = "browser.errors.collected_count";
 const TELEMETRY_ERROR_COLLECTED_FILENAME = "browser.errors.collected_count_by_filename";
 const TELEMETRY_ERROR_COLLECTED_STACK = "browser.errors.collected_with_stack_count";
 const TELEMETRY_ERROR_REPORTED = "browser.errors.reported_success_count";
 const TELEMETRY_ERROR_REPORTED_FAIL = "browser.errors.reported_failure_count";
 const TELEMETRY_ERROR_SAMPLE_RATE = "browser.errors.sample_rate";
@@ -78,27 +79,42 @@ const MODULE_SAMPLE_RATES = new Map([
  * The outgoing requests are designed to be compatible with Sentry. See
  * https://docs.sentry.io/clientdev/ for details on the data format that Sentry
  * expects.
  *
  * Errors may contain PII, such as in messages or local file paths in stack
  * traces; see bug 1426482 for privacy review and server-side mitigation.
  */
 class BrowserErrorReporter {
+  /**
+   * Generate a Date object corresponding to the date in the appBuildId.
+   */
+  static getAppBuildIdDate() {
+    const appBuildId = Services.appinfo.appBuildID;
+    const buildYear = Number.parseInt(appBuildId.slice(0, 4));
+    // Date constructor uses 0-indexed months
+    const buildMonth = Number.parseInt(appBuildId.slice(4, 6)) - 1;
+    const buildDay = Number.parseInt(appBuildId.slice(6, 8));
+    return new Date(buildYear, buildMonth, buildDay);
+  }
+
   constructor(options = {}) {
     // Test arguments for mocks and changing behavior
     this.fetch = options.fetch || defaultFetch;
+    this.now = options.now || null;
     this.chromeOnly = options.chromeOnly !== undefined ? options.chromeOnly : true;
     this.registerListener = (
       options.registerListener || (() => Services.console.registerListener(this))
     );
     this.unregisterListener = (
       options.unregisterListener || (() => Services.console.unregisterListener(this))
     );
 
+    XPCOMUtils.defineLazyGetter(this, "appBuildIdDate", BrowserErrorReporter.getAppBuildIdDate);
+
     // Values that don't change between error reports.
     this.requestBodyTemplate = {
       logger: "javascript",
       platform: "javascript",
       release: Services.appinfo.appBuildID,
       environment: UpdateUtils.getUpdateChannel(false),
       contexts: {
         os: {
@@ -193,16 +209,23 @@ class BrowserErrorReporter {
     // WebExtensions get grouped separately from other errors
     if (filename.startsWith("moz-extension://")) {
         return "MOZEXTENSION";
     }
 
     return "FILTERED";
   }
 
+  isRecentBuild() {
+    // The local clock is not reliable, but this method doesn't need to be
+    // perfect.
+    const now = this.now || new Date();
+    return (now - this.appBuildIdDate) <= RECENT_BUILD_AGE;
+  }
+
   observe(message) {
     if (message instanceof Ci.nsIScriptError) {
       ChromeUtils.idleDispatch(() => this.handleMessage(message));
     }
   }
 
   async handleMessage(message) {
     const isWarning = message.flags & message.warningFlag;
@@ -216,16 +239,21 @@ class BrowserErrorReporter {
     if (message.stack) {
       Services.telemetry.scalarAdd(TELEMETRY_ERROR_COLLECTED_STACK, 1);
     }
     if (message.sourceName) {
       const key = this.errorCollectedFilenameKey(message.sourceName);
       Services.telemetry.keyedScalarAdd(TELEMETRY_ERROR_COLLECTED_FILENAME, key.slice(0, 69), 1);
     }
 
+    // Old builds should not send errors to Sentry
+    if (!this.isRecentBuild()) {
+      return;
+    }
+
     // Sample the amount of errors we send out
     let sampleRate = Number.parseFloat(this.sampleRatePref);
     for (const [regex, rate] of MODULE_SAMPLE_RATES) {
       if (message.sourceName.match(regex)) {
         sampleRate = rate;
         break;
       }
     }
--- a/browser/modules/test/browser/browser_BrowserErrorReporter.js
+++ b/browser/modules/test/browser/browser_BrowserErrorReporter.js
@@ -110,16 +110,17 @@ add_task(async function testInitUninitPr
 });
 
 add_task(async function testInitPastMessages() {
   const fetchSpy = sinon.spy();
   const reporter = new BrowserErrorReporter({
     fetch: fetchSpy,
     registerListener: noop,
     unregisterListener: noop,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
   });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
   resetConsole();
   Services.console.logMessage(createScriptError({message: "Logged before init"}));
@@ -138,16 +139,17 @@ add_task(async function testEnabledPrefW
   let listening = false;
   const reporter = new BrowserErrorReporter({
     registerListener() {
       listening = true;
     },
     unregisterListener() {
       listening = false;
     },
+    now: BrowserErrorReporter.getAppBuildIdDate(),
   });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, false],
   ]});
 
   reporter.init();
   ok(!listening, "Reporter does not collect errors if the enable pref is false.");
 
@@ -155,17 +157,20 @@ add_task(async function testEnabledPrefW
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
   ]});
   ok(listening, "Reporter collects errors if the enabled pref switches to true.");
 });
 
 add_task(async function testNonErrorLogs() {
   const fetchSpy = sinon.spy();
-  const reporter = new BrowserErrorReporter({fetch: fetchSpy});
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchSpy,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
+  });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
   await reporter.handleMessage({message: "Not a scripterror instance."});
   ok(
     !fetchPassedError(fetchSpy, "Not a scripterror instance."),
@@ -194,17 +199,20 @@ add_task(async function testNonErrorLogs
   ok(
     fetchPassedError(fetchSpy, "Is error"),
     "Reporter collects error messages.",
   );
 });
 
 add_task(async function testSampling() {
   const fetchSpy = sinon.spy();
-  const reporter = new BrowserErrorReporter({fetch: fetchSpy});
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchSpy,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
+  });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
   await reporter.handleMessage(createScriptError({message: "Should log"}));
   ok(
     fetchPassedError(fetchSpy, "Should log"),
@@ -251,17 +259,20 @@ add_task(async function testSampling() {
   ok(
     !fetchPassedError(fetchSpy, "Also shouldn't log"),
     "An invalid sample rate will cause the reporter to never collect errors.",
   );
 });
 
 add_task(async function testNameMessage() {
   const fetchSpy = sinon.spy();
-  const reporter = new BrowserErrorReporter({fetch: fetchSpy});
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchSpy,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
+  });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
   await reporter.handleMessage(createScriptError({message: "No name"}));
   let call = fetchCallForMessage(fetchSpy, "No name");
   let body = JSON.parse(call.args[1].body);
@@ -300,19 +311,44 @@ add_task(async function testNameMessage(
   );
   is(
     body.exception.values[0].value,
     "Has :extra: colons",
     "Reporter uses error message as the value parameter.",
   );
 });
 
+add_task(async function testRecentBuild() {
+  // Create date that is guaranteed to be a month newer than the build date.
+  const nowDate = BrowserErrorReporter.getAppBuildIdDate();
+  nowDate.setMonth(nowDate.getMonth() + 1);
+
+  const fetchSpy = sinon.spy();
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchSpy,
+    now: nowDate,
+  });
+  await SpecialPowers.pushPrefEnv({set: [
+    [PREF_ENABLED, true],
+    [PREF_SAMPLE_RATE, "1.0"],
+  ]});
+
+  await reporter.handleMessage(createScriptError({message: "Is error"}));
+  ok(
+    !fetchPassedError(fetchSpy, "Is error"),
+    "Reporter does not collect errors from builds older than a week.",
+  );
+});
+
 add_task(async function testFetchArguments() {
   const fetchSpy = sinon.spy();
-  const reporter = new BrowserErrorReporter({fetch: fetchSpy});
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchSpy,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
+  });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
     [PREF_PROJECT_ID, "123"],
     [PREF_PUBLIC_KEY, "foobar"],
     [PREF_SUBMIT_URL, "https://errors.example.com/api/123/store/"],
   ]});
 
@@ -400,17 +436,21 @@ add_task(async function testFetchArgumen
 
   reporter.uninit();
 });
 
 add_task(async function testAddonIDMangle() {
   const fetchSpy = sinon.spy();
   // Passing false here disables category checks on errors, which would
   // otherwise block errors directly from extensions.
-  const reporter = new BrowserErrorReporter({fetch: fetchSpy, chromeOnly: false});
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchSpy,
+    chromeOnly: false,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
+  });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
   resetConsole();
   reporter.init();
 
   // Create and install test add-on
@@ -442,17 +482,21 @@ add_task(async function testAddonIDMangl
   await extension.unload();
   reporter.uninit();
 });
 
 add_task(async function testExtensionTag() {
   const fetchSpy = sinon.spy();
   // Passing false here disables category checks on errors, which would
   // otherwise block errors directly from extensions.
-  const reporter = new BrowserErrorReporter({fetch: fetchSpy, chromeOnly: false});
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchSpy,
+    chromeOnly: false,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
+  });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
   resetConsole();
   reporter.init();
 
   // Create and install test add-on
@@ -483,17 +527,20 @@ add_task(async function testExtensionTag
   await reporter.handleMessage(createScriptError({message: "testExtensionTag not from extension"}));
   call = fetchCallForMessage(fetchSpy, "testExtensionTag not from extension");
   body = JSON.parse(call.args[1].body);
   is(body.tags.isExtensionError, false, "Normal errors have an isExtensionError=false tag.");
 });
 
 add_task(async function testScalars() {
   const fetchStub = sinon.stub();
-  const reporter = new BrowserErrorReporter({fetch: fetchStub});
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchStub,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
+  });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
   Services.telemetry.clearScalars();
 
   const messages = [
@@ -569,17 +616,20 @@ add_task(async function testScalars() {
     `${TELEMETRY_ERROR_COLLECTED_FILENAME} is incremented when an error is collected.`,
   );
 
   resetConsole();
 });
 
 add_task(async function testCollectedFilenameScalar() {
   const fetchStub = sinon.stub();
-  const reporter = new BrowserErrorReporter(fetchStub);
+  const reporter = new BrowserErrorReporter({
+    fetch: fetchStub,
+    now: BrowserErrorReporter.getAppBuildIdDate(),
+  });
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
   const testCases = [
     ["chrome://unknown/module.jsm", false],
     ["resource://unknown/module.jsm", false],