bug 1460595 - Remove events from main pings and, thus, TelemetrySession r?Dexter draft
authorChris H-C <chutten@mozilla.com>
Wed, 06 Jun 2018 11:05:29 -0400
changeset 810759 8e7d806bc2f9
parent 810758 4d903b78b6bb
push id114089
push userbmo:chutten@mozilla.com
push dateTue, 26 Jun 2018 12:37:14 +0000
reviewersDexter
bugs1460595
milestone63.0a1
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