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

--HG--
extra : rebase_source : 63ffa636213bdcdc437e3768b4d449b7cb73ead4

toolkit/components/telemetry/TelemetrySession.jsm

@@ -868,26 +868,6 @@ var Impl = {
     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
    *
@@ -1144,7 +1124,6 @@ var Impl = {
       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
@@ -1168,14 +1147,13 @@ var Impl = {
         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.

toolkit/components/telemetry/tests/unit/test_ChildEvents.js

@@ -112,43 +112,40 @@ add_task(async function() {
   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)),
@@ -157,7 +154,7 @@ add_task(async function() {
             "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 =

toolkit/components/telemetry/tests/unit/test_TelemetrySession.js

@@ -275,53 +275,6 @@ function checkScalars(processes) {
   }
 }
 
-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);
@@ -452,7 +405,6 @@ function checkPayload(payload, reason, successfulPings, savedPings) {
   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) {
@@ -695,63 +647,6 @@ add_task(async function test_checkSubsessionScalars() {
   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.

toolkit/content/aboutTelemetry.js

@@ -1708,47 +1708,6 @@ var KeyedScalars = {
   },
 };
 
-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
  *
@@ -2347,9 +2306,6 @@ function displayRichPingData(ping, updatePayloadList) {
   // Show keyed histogram data
   KeyedHistogramSection.render(payload);
 
-  // Show event data.
-  Events.render(payload);
-
   // Show captured stacks.
   CapturedStacks.render(payload);
 

toolkit/content/aboutTelemetry.xhtml

@@ -57,9 +57,6 @@
       <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>

toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd

@@ -34,7 +34,6 @@
 <!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">

toolkit/locales/en-US/chrome/global/aboutTelemetry.properties

@@ -94,8 +94,3 @@ 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