bug 1460595 - Remove events from main pings and, thus, TelemetrySession r=Dexter
authorChris H-C <chutten@mozilla.com>
Wed, 06 Jun 2018 11:05:29 -0400
changeset 423723 378ddda2ffc6
parent 423722 6c93b28d0ab8
child 423724 e97d629e0b08
push id34191
push userrgurzau@mozilla.com
push dateTue, 26 Jun 2018 21:53:37 +0000
treeherdermozilla-central@1c235a552c32 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersDexter
bugs1460595
milestone63.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 1460595 - Remove events from main pings and, thus, TelemetrySession r=Dexter This requires we take unsent event records out of about:telemetry since its "Current Payload" view only looks at the "main" ping. MozReview-Commit-ID: GLs2EYvFaAF
toolkit/components/telemetry/TelemetrySession.jsm
toolkit/components/telemetry/tests/unit/test_ChildEvents.js
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
toolkit/content/aboutTelemetry.js
toolkit/content/aboutTelemetry.xhtml
toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -863,36 +863,16 @@ var Impl = {
         }
         ret[processName][name] = scalarsSnapshot[processName][name];
       }
     }
 
     return ret;
   },
 
-  getEvents(isSubsession, clearSubsession) {
-    if (!isSubsession) {
-      // We only support scalars for subsessions.
-      this._log.trace("getEvents - We only support events in subsessions.");
-      return [];
-    }
-
-    let snapshot = Telemetry.snapshotEvents(this.getDatasetType(),
-                                            clearSubsession);
-
-    // Don't return the test events outside of test environments.
-    if (!this._testing) {
-      for (let proc of Object.keys(snapshot)) {
-        snapshot[proc] = snapshot[proc].filter(e => !e[1].startsWith("telemetry.test"));
-      }
-    }
-
-    return snapshot;
-  },
-
   /**
    * Descriptive metadata
    *
    * @param  reason
    *         The reason for the telemetry ping, this will be included in the
    *         returned metadata,
    * @return The metadata as a JS object
    */
@@ -1139,17 +1119,16 @@ var Impl = {
     }
 
     // Additional payload for chrome process.
     let measurements = {
       histograms: protect(() => this.getHistograms(clearSubsession), {}),
       keyedHistograms: protect(() => this.getKeyedHistograms(clearSubsession), {}),
       scalars: protect(() => this.getScalars(isSubsession, clearSubsession), {}),
       keyedScalars: protect(() => this.getScalars(isSubsession, clearSubsession, true), {}),
-      events: protect(() => this.getEvents(isSubsession, clearSubsession)),
     };
 
     let measurementsContainGPU = Object
       .keys(measurements)
       .some(key => "gpu" in measurements[key]);
 
     payloadObj.processes = {};
     let processTypes = ["parent", "content", "extension", "dynamic"];
@@ -1163,24 +1142,23 @@ var Impl = {
       let processPayload = {};
 
       for (const key in measurements) {
         let payloadLoc = processPayload;
         // Parent histograms are added to the top-level payload object instead of the process payload.
         if (processType == "parent" && (key == "histograms" || key == "keyedHistograms")) {
           payloadLoc = payloadObj;
         }
-        // The Dynamic process only collects events and scalars.
-        if (processType == "dynamic" && !["events", "scalars"].includes(key)) {
+        // The Dynamic process only collects scalars.
+        if (processType == "dynamic" && key !== "scalars") {
           continue;
         }
 
         // Process measurements can be empty, set a default value.
-        let defaultValue = key == "events" ? [] : {};
-        payloadLoc[key] = measurements[key][processType] || defaultValue;
+        payloadLoc[key] = measurements[key][processType] || {};
       }
 
       // Add process measurements to payload.
       payloadObj.processes[processType] = processPayload;
     }
 
     payloadObj.info = info;
 
--- a/toolkit/components/telemetry/tests/unit/test_ChildEvents.js
+++ b/toolkit/components/telemetry/tests/unit/test_ChildEvents.js
@@ -107,60 +107,57 @@ add_task(async function() {
   // and batch send the data back to the parent process.
   await waitForContentEvents();
   const timestampAfterChildEvents = Telemetry.msSinceProcessStart();
 
   // Also record some events in the parent.
   RECORDED_PARENT_EVENTS.forEach(e => Telemetry.recordEvent(...e));
   UNRECORDED_PARENT_EVENTS.forEach(e => Telemetry.recordEvent(...e));
 
-  // Get an "environment-changed" ping rather than a "test-ping", as
-  // event measurements are only supported in subsession pings.
-  const payload = TelemetrySession.getPayload("environment-change");
+  let snapshot =
+    Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
 
-  // Validate the event data is present in the payload.
-  Assert.ok("processes" in payload, "Should have processes section");
-  Assert.ok("parent" in payload.processes, "Should have main process section");
-  Assert.ok("events" in payload.processes.parent, "Main process section should have events.");
-  Assert.ok("content" in payload.processes, "Should have child process section");
-  Assert.ok("events" in payload.processes.content, "Child process section should have events.");
-  Assert.ok("dynamic" in payload.processes, "Should have dynamic process section");
-  Assert.ok("events" in payload.processes.dynamic, "Dynamic process section should have events.");
+  Assert.ok("parent" in snapshot, "Should have main process section");
+  Assert.ok(snapshot.parent.length > 0, "Main process section should have events.");
+  Assert.ok("content" in snapshot, "Should have child process section");
+  Assert.ok(snapshot.content.length > 0, "Child process section should have events.");
+  Assert.ok("dynamic" in snapshot, "Should have dynamic process section");
+  Assert.ok(snapshot.dynamic.length > 0, "Dynamic process section should have events.");
 
   // Check that the expected events are present from the content process.
-  let contentEvents = payload.processes.content.events.map(e => e.slice(1));
+  let contentEvents = snapshot.content.map(e => e.slice(1));
   Assert.equal(contentEvents.length, RECORDED_CONTENT_EVENTS.length, "Should match expected event count.");
   for (let i = 0; i < RECORDED_CONTENT_EVENTS.length; ++i) {
     Assert.deepEqual(contentEvents[i], RECORDED_CONTENT_EVENTS[i], "Should have recorded expected event.");
   }
 
   // Check that the expected events are present from the parent process.
-  let parentEvents = payload.processes.parent.events.map(e => e.slice(1));
+  let parentEvents = snapshot.parent.map(e => e.slice(1));
   Assert.equal(parentEvents.length, RECORDED_PARENT_EVENTS.length, "Should match expected event count.");
   for (let i = 0; i < RECORDED_PARENT_EVENTS.length; ++i) {
     Assert.deepEqual(parentEvents[i], RECORDED_PARENT_EVENTS[i], "Should have recorded expected event.");
   }
 
   // Check that the expected dynamic events are present.
-  let dynamicEvents = payload.processes.dynamic.events.map(e => e.slice(1));
+  let dynamicEvents = snapshot.dynamic.map(e => e.slice(1));
   Assert.equal(dynamicEvents.length, RECORDED_DYNAMIC_EVENTS.length, "Should match expected event count.");
   for (let i = 0; i < RECORDED_DYNAMIC_EVENTS.length; ++i) {
     Assert.deepEqual(dynamicEvents[i], RECORDED_DYNAMIC_EVENTS[i], "Should have recorded expected event.");
   }
 
   // Check that the event timestamps are in the expected ranges.
-  let contentTimestamps = payload.processes.content.events.map(e => e[0]);
-  let parentTimestamps = payload.processes.parent.events.map(e => e[0]);
+  let contentTimestamps = snapshot.content.map(e => e[0]);
+  let parentTimestamps = snapshot.parent.map(e => e[0]);
 
   Assert.ok(contentTimestamps.every(ts => (ts > Math.floor(timestampBeforeChildEvents)) &&
                                           (ts < timestampAfterChildEvents)),
             "All content event timestamps should be in the expected time range.");
   Assert.ok(parentTimestamps.every(ts => (ts >= Math.floor(timestampAfterChildEvents))),
             "All parent event timestamps should be in the expected time range.");
 
   // Make sure all events are cleared from storage properly.
-  let snapshot =
+  snapshot =
       Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true);
   Assert.greaterOrEqual(Object.keys(snapshot).length, 2, "Should have events from at least two processes.");
   snapshot =
       Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true);
   Assert.equal(Object.keys(snapshot).length, 0, "Should have cleared all events from storage.");
 });
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -270,63 +270,16 @@ function checkScalars(processes) {
     for (let key in keyedScalars[name]) {
       Assert.equal(typeof key, "string", "Keyed scalar keys must be strings.");
       Assert.ok(key.length <= 70, "Keyed scalar keys can't have more than 70 characters.");
       checkScalar(scalars[name][key]);
     }
   }
 }
 
-function checkEvents(processes) {
-  // Check that the events section is available in the ping payload.
-  const parent = processes.parent;
-  Assert.ok("events" in parent, "The events section must be available in the parent process.");
-
-  // Check that the events section has the right format.
-  Assert.ok(Array.isArray(parent.events), "The events entry must be an array.");
-  for (let [ts, category, method, object, value, extra] of parent.events) {
-    Assert.equal(typeof(ts), "number", "Timestamp field should be a number.");
-    Assert.greaterOrEqual(ts, 0, "Timestamp should be >= 0.");
-
-    Assert.equal(typeof(category), "string", "Category should have the right type.");
-    Assert.lessOrEqual(category.length, 100, "Category should have the right string length.");
-
-    Assert.equal(typeof(method), "string", "Method should have the right type.");
-    Assert.lessOrEqual(method.length, 40, "Method should have the right string length.");
-
-    Assert.equal(typeof(object), "string", "Object should have the right type.");
-    Assert.lessOrEqual(object.length, 40, "Object should have the right string length.");
-
-    Assert.ok(value === null || typeof(value) === "string",
-              "Value should be null or a string.");
-    if (value) {
-      Assert.lessOrEqual(value.length, 100, "Value should have the right string length.");
-    }
-
-    Assert.ok(extra === null || typeof(extra) === "object",
-              "Extra should be null or an object.");
-    if (extra) {
-      let keys = Object.keys(extra);
-      let keyTypes = keys.map(k => typeof(k));
-      Assert.lessOrEqual(keys.length, 20, "Should not have too many extra keys.");
-      Assert.ok(keyTypes.every(t => t === "string"),
-                "All extra keys should be strings.");
-      Assert.ok(keys.every(k => k.length <= 20),
-                "All extra keys should have the right string length.");
-
-      let values = Object.values(extra);
-      let valueTypes = values.map(v => typeof(v));
-      Assert.ok(valueTypes.every(t => t === "string"),
-                "All extra values should be strings.");
-      Assert.ok(values.every(v => v.length <= 100),
-                "All extra values should have the right string length.");
-    }
-  }
-}
-
 function checkPayload(payload, reason, successfulPings, savedPings) {
   Assert.ok("info" in payload, "Payload must contain an info section.");
   checkPayloadInfo(payload.info, reason);
 
   Assert.ok(payload.simpleMeasurements.totalTime >= 0);
   Assert.ok(payload.simpleMeasurements.uptime >= 0);
   Assert.equal(payload.simpleMeasurements.startupInterrupted, 1);
   Assert.equal(payload.simpleMeasurements.shutdownDuration, SHUTDOWN_TIME);
@@ -447,17 +400,16 @@ function checkPayload(payload, reason, s
       sum: 1,
     },
   };
   Assert.deepEqual(expected_keyed_count, keyedHistograms[TELEMETRY_TEST_KEYED_COUNT]);
 
   Assert.ok("processes" in payload, "The payload must have a processes section.");
   Assert.ok("parent" in payload.processes, "There must be at least a parent process.");
   checkScalars(payload.processes);
-  checkEvents(payload.processes);
 }
 
 function writeStringToFile(file, contents) {
   let ostream = Cc["@mozilla.org/network/safe-file-output-stream;1"]
                 .createInstance(Ci.nsIFileOutputStream);
   ostream.init(file, PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
                RW_OWNER, ostream.DEFER_OPEN);
   ostream.write(contents, contents.length);
@@ -690,73 +642,16 @@ add_task(async function test_checkSubses
   Assert.equal(subsession.processes.parent.scalars[UINT_SCALAR], expectedUint,
                UINT_SCALAR + " must contain the expected value.");
   Assert.equal(subsession.processes.parent.scalars[STRING_SCALAR], expectedString,
                STRING_SCALAR + " must contain the expected value.");
 
   await TelemetryController.testShutdown();
 });
 
-add_task(async function test_checkSubsessionEvents() {
-  if (gIsAndroid) {
-    // We don't support subsessions yet on Android.
-    return;
-  }
-
-  // Clear the events.
-  Telemetry.clearEvents();
-  await TelemetryController.testReset();
-
-  // Enable recording for the test events.
-  Telemetry.setEventRecordingEnabled("telemetry.test", true);
-
-  // Record some events.
-  let expected = [
-    ["telemetry.test", "test1", "object1", "a", null],
-    ["telemetry.test", "test1", "object1", null, {key1: "value"}],
-  ];
-  for (let event of expected) {
-    Telemetry.recordEvent(...event);
-  }
-
-  // Strip off trailing null values to match the serialized events.
-  for (let e of expected) {
-    while ((e.length >= 3) && (e[e.length - 1] === null)) {
-      e.pop();
-    }
-  }
-
-  // Check that events are not available in classic pings but are in subsession
-  // pings. Also clear the subsession.
-  let classic = TelemetrySession.getPayload();
-  let subsession = TelemetrySession.getPayload("environment-change", true);
-
-  Assert.ok("events" in classic.processes.parent, "Should have an events field in classic payload.");
-  Assert.ok("events" in subsession.processes.parent, "Should have an events field in subsession payload.");
-
-  // They should be empty in the classic payload.
-  Assert.deepEqual(classic.processes.parent.events, [], "Events in classic payload should be empty.");
-
-  // In the subsession payload, they should contain the recorded test events.
-  let events = subsession.processes.parent.events.filter(e => e[1] === "telemetry.test");
-  Assert.equal(events.length, expected.length, "Should have the right amount of events in the payload.");
-  for (let i = 0; i < expected.length; ++i) {
-    Assert.deepEqual(events[i].slice(1), expected[i],
-                     "Should have the right event data in the ping.");
-  }
-
-  // As we cleared the subsession above, the events entry should now be empty.
-  subsession = TelemetrySession.getPayload("environment-change", false);
-  Assert.ok("events" in subsession.processes.parent, "Should have an events field in subsession payload.");
-  events = subsession.processes.parent.events.filter(e => e[1] === "telemetry.test");
-  Assert.equal(events.length, 0, "Should have no test events in the subsession payload now.");
-
-  await TelemetryController.testShutdown();
-});
-
 add_task(async function test_dailyCollection() {
   if (gIsAndroid) {
     // We don't do daily collections yet on Android.
     return;
   }
 
   let now = new Date(2030, 1, 1, 12, 0, 0);
   let nowHour = new Date(2030, 1, 1, 12, 0, 0);
--- a/toolkit/content/aboutTelemetry.js
+++ b/toolkit/content/aboutTelemetry.js
@@ -1703,57 +1703,16 @@ var KeyedScalars = {
       // Populate the section with the key-value pairs from the scalar.
       const table = GenericTable.render(explodeObject(keyedScalars[scalar]), headings);
       container.appendChild(table);
       scalarsSection.appendChild(container);
     }
   },
 };
 
-var Events = {
-  /**
-   * Render the event data - if present - from the payload in a simple table.
-   * @param aPayload A payload object to render the data from.
-   */
-  render(aPayload) {
-    let eventsSection = document.getElementById("events");
-    removeAllChildNodes(eventsSection);
-
-    let processesSelect = document.getElementById("processes");
-    let selectedProcess = processesSelect.selectedOptions.item(0).getAttribute("value");
-
-    if (!aPayload.processes ||
-        !selectedProcess ||
-        !(selectedProcess in aPayload.processes)) {
-      return;
-    }
-
-    let events = aPayload.processes[selectedProcess].events || {};
-    let hasData = Array.from(processesSelect.options).some((option) => {
-      let value = option.getAttribute("value");
-      let evts = aPayload.processes[value].events;
-      return evts && Object.keys(evts).length > 0;
-    });
-    setHasData("events-section", hasData);
-    if (Object.keys(events).length > 0) {
-      const headings = [
-        "timestampHeader",
-        "categoryHeader",
-        "methodHeader",
-        "objectHeader",
-        "valuesHeader",
-        "extraHeader",
-      ].map(h => bundle.GetStringFromName(h));
-
-      const table = GenericTable.render(events, headings);
-      eventsSection.appendChild(table);
-    }
-  },
-};
-
 /**
  * Helper function for showing either the toggle element or "No data collected" message for a section
  *
  * @param aSectionID ID of the section element that needs to be changed
  * @param aHasData true (default) indicates that toggle should be displayed
  */
 function setHasData(aSectionID, aHasData) {
   let sectionElement = document.getElementById(aSectionID);
@@ -2342,19 +2301,16 @@ function displayRichPingData(ping, updat
   KeyedScalars.render(payload);
 
   // Show histogram data
   HistogramSection.render(payload);
 
   // Show keyed histogram data
   KeyedHistogramSection.render(payload);
 
-  // Show event data.
-  Events.render(payload);
-
   // Show captured stacks.
   CapturedStacks.render(payload);
 
   LateWritesSingleton.renderLateWrites(payload.lateWrites);
 
   // Show simple measurements
   SimpleMeasurements.render(payload);
 
--- a/toolkit/content/aboutTelemetry.xhtml
+++ b/toolkit/content/aboutTelemetry.xhtml
@@ -52,19 +52,16 @@
         <span class="category-name">&aboutTelemetry.keyedScalarsSection;</span>
       </div>
       <div class="category" value="histograms-section">
         <span class="category-name">&aboutTelemetry.histogramsSection;</span>
       </div>
       <div class="category" value="keyed-histograms-section">
         <span class="category-name">&aboutTelemetry.keyedHistogramsSection;</span>
       </div>
-      <div class="category" value="events-section">
-        <span class="category-name">&aboutTelemetry.eventsSection;</span>
-      </div>
       <div class="category" value="simple-measurements-section">
         <span class="category-name">&aboutTelemetry.simpleMeasurementsSection;</span>
       </div>
       <div class="category" value="slow-sql-section">
         <span class="category-name">&aboutTelemetry.slowSqlSection;</span>
       </div>
       <div class="category" value="addon-details-section">
         <span class="category-name">&aboutTelemetry.addonDetailsSection;</span>
--- a/toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
+++ b/toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
@@ -29,17 +29,16 @@
 <!ENTITY aboutTelemetry.homeSection "Home">
 <!ENTITY aboutTelemetry.generalDataSection "General Data">
 <!ENTITY aboutTelemetry.environmentDataSection "Environment Data">
 <!ENTITY aboutTelemetry.sessionInfoSection "Session Information">
 <!ENTITY aboutTelemetry.scalarsSection "Scalars">
 <!ENTITY aboutTelemetry.keyedScalarsSection "Keyed Scalars">
 <!ENTITY aboutTelemetry.histogramsSection "Histograms">
 <!ENTITY aboutTelemetry.keyedHistogramsSection "Keyed Histograms">
-<!ENTITY aboutTelemetry.eventsSection "Events">
 <!ENTITY aboutTelemetry.simpleMeasurementsSection "Simple Measurements">
 <!ENTITY aboutTelemetry.slowSqlSection "Slow SQL Statements">
 <!ENTITY aboutTelemetry.addonDetailsSection "Add-on Details">
 <!ENTITY aboutTelemetry.capturedStacksSection "Captured Stacks">
 <!ENTITY aboutTelemetry.lateWritesSection "Late Writes">
 <!ENTITY aboutTelemetry.rawPayloadSection "Raw Payload">
 <!ENTITY aboutTelemetry.raw "Raw JSON">
 
--- a/toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
+++ b/toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ -89,13 +89,8 @@ captured-stacks-title = %1$S (capture co
 # - %1$S is replaced by the number of the late write
 late-writes-title = Late Write #%1$S
 
 stackTitle = Stack:
 memoryMapTitle = Memory map:
 
 errorFetchingSymbols = An error occurred while fetching symbols. Check that you are connected to the Internet and try again.
 
-timestampHeader = timestamp
-categoryHeader = category
-methodHeader = method
-objectHeader = object
-extraHeader = extra