bug 1366294 - Part 5 - Send appropriately-structured Telemetry data to JS r?gfritzsche draft
authorChris H-C <chutten@mozilla.com>
Tue, 20 Jun 2017 15:03:10 -0400
changeset 610613 b54365078d30e4009da10154aa78ac3889b1b952
parent 610612 b146a1d500ed926f29e425adb7a11f0a617e2ad8
child 610614 b7e74b6ae8997dfc1664580a6a538b482173e7e2
push id68956
push userbmo:chutten@mozilla.com
push dateTue, 18 Jul 2017 15:29:20 +0000
reviewersgfritzsche
bugs1366294
milestone56.0a1
bug 1366294 - Part 5 - Send appropriately-structured Telemetry data to JS r?gfritzsche Previously we were doing bad string manipulation nonsense. Now when asked for snapshots C++ can return a properly-formated Object tree. MozReview-Commit-ID: HAvIbgzUvMU
toolkit/components/telemetry/TelemetryHistogram.cpp
toolkit/components/telemetry/TelemetrySession.jsm
toolkit/components/telemetry/nsITelemetry.idl
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -37,16 +37,17 @@ using mozilla::StaticMutexAutoLock;
 using mozilla::Telemetry::Accumulation;
 using mozilla::Telemetry::KeyedAccumulation;
 using mozilla::Telemetry::HistogramID;
 using mozilla::Telemetry::ProcessID;
 using mozilla::Telemetry::HistogramCount;
 using mozilla::Telemetry::Common::LogToBrowserConsole;
 using mozilla::Telemetry::Common::RecordedProcessType;
 using mozilla::Telemetry::Common::AutoHashtable;
+using mozilla::Telemetry::Common::GetNameForProcessID;
 using mozilla::Telemetry::Common::IsExpiredVersion;
 using mozilla::Telemetry::Common::CanRecordDataset;
 using mozilla::Telemetry::Common::IsInDataset;
 
 namespace TelemetryIPCAccumulator = mozilla::TelemetryIPCAccumulator;
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
@@ -1938,65 +1939,63 @@ TelemetryHistogram::CreateHistogramSnaps
       }
 
       mozilla::DebugOnly<Histogram*> h = nullptr;
       h = internal_GetHistogramById(HistogramID(histogramId), ProcessID(process), sessionType);
       MOZ_ASSERT(h);
     }
   }
 
-  // TODO: This won't get us data from different processes.
-  // We'll have to refactor this function to return {"parent": {...}, "content": {...}}.
-
-  // OK, now we can actually reflect things.
-  JS::Rooted<JSObject*> hobj(cx);
-  for (size_t i = 0; i < HistogramCount; ++i) {
-    const HistogramInfo& info = gHistogramInfos[i];
-    if (info.keyed) {
-      continue;
+  for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
+    JS::Rooted<JSObject*> processObject(cx, JS_NewPlainObject(cx));
+    if (!processObject) {
+      return NS_ERROR_FAILURE;
     }
-
-    HistogramID id = HistogramID(i);
-
-    // TODO: support multiple processes.
-    //for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
-
-    uint32_t process = uint32_t(ProcessID::Parent);
-    if (!CanRecordInProcess(info.record_in_processes, ProcessID(process)) ||
-      ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess)) {
-      continue;
-    }
-
-    Histogram* h = internal_GetHistogramById(id, ProcessID(process), sessionType, /* instantiate = */ false);
-    if (!h || internal_IsExpired(h) || !internal_ShouldReflectHistogram(h, id)) {
-      continue;
-    }
-
-    hobj = JS_NewPlainObject(cx);
-    if (!hobj) {
+    if (!JS_DefineProperty(cx, root_obj, GetNameForProcessID(ProcessID(process)),
+                           processObject, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
-    switch (internal_ReflectHistogramSnapshot(cx, hobj, h)) {
-    case REFLECT_FAILURE:
-      return NS_ERROR_FAILURE;
-    case REFLECT_OK:
-      if (!JS_DefineProperty(cx, root_obj, gHistogramInfos[id].name(),
-                             hobj, JSPROP_ENUMERATE)) {
+    for (size_t i = 0; i < HistogramCount; ++i) {
+      const HistogramInfo& info = gHistogramInfos[i];
+      if (info.keyed) {
+        continue;
+      }
+
+      HistogramID id = HistogramID(i);
+
+      if (!CanRecordInProcess(info.record_in_processes, ProcessID(process)) ||
+        ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess)) {
+        continue;
+      }
+
+      Histogram* h = internal_GetHistogramById(id, ProcessID(process), sessionType, /* instantiate = */ false);
+      if (!h || internal_IsExpired(h) || !internal_ShouldReflectHistogram(h, id)) {
+        continue;
+      }
+
+      JS::Rooted<JSObject*> hobj(cx, JS_NewPlainObject(cx));
+      if (!hobj) {
         return NS_ERROR_FAILURE;
       }
-    }
+      switch (internal_ReflectHistogramSnapshot(cx, hobj, h)) {
+      case REFLECT_FAILURE:
+        return NS_ERROR_FAILURE;
+      case REFLECT_OK:
+        if (!JS_DefineProperty(cx, processObject, gHistogramInfos[id].name(),
+                               hobj, JSPROP_ENUMERATE)) {
+          return NS_ERROR_FAILURE;
+        }
+      }
 
 #if !defined(MOZ_WIDGET_ANDROID)
-    if ((sessionType == SessionType::Subsession) && clearSubsession) {
-      h->Clear();
-    }
+      if ((sessionType == SessionType::Subsession) && clearSubsession) {
+        h->Clear();
+      }
 #endif
-
-    // TODO: support multiple processes.
-    // }
+    }
   }
   return NS_OK;
 }
 
 nsresult
 TelemetryHistogram::RegisteredHistograms(uint32_t aDataset, uint32_t *aCount,
                                          char*** aHistograms)
 {
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -931,23 +931,21 @@ var Impl = {
       registered = registered.filter(n => !n.startsWith("TELEMETRY_TEST_"));
     }
     registered = registered.concat(registered.map(n => "STARTUP_" + n));
 
     let hls = subsession ? Telemetry.snapshotSubsessionHistograms(clearSubsession)
                          : Telemetry.histogramSnapshots;
     let ret = {};
 
-    for (let name of registered) {
-      for (let suffix of Object.values(HISTOGRAM_SUFFIXES)) {
-        if (name + suffix in hls) {
-          if (!(suffix in ret)) {
-            ret[suffix] = {};
-          }
-          ret[suffix][name] = this.packHistogram(hls[name + suffix]);
+    for (let [process, histograms] of Object.entries(hls)) {
+      ret[process] = {};
+      for (let [name, value] of Object.entries(histograms)) {
+        if (registered.includes(name)) {
+          ret[process][name] = this.packHistogram(value);
         }
       }
     }
 
     return ret;
   },
 
   getKeyedHistograms(subsession, clearSubsession) {
@@ -1321,50 +1319,50 @@ var Impl = {
 
     // Additional payload for chrome process.
     let histograms = protect(() => this.getHistograms(isSubsession, clearSubsession), {});
     let keyedHistograms = protect(() => this.getKeyedHistograms(isSubsession, clearSubsession), {});
     let scalars = protect(() => this.getScalars(isSubsession, clearSubsession), {});
     let keyedScalars = protect(() => this.getScalars(isSubsession, clearSubsession, true), {});
     let events = protect(() => this.getEvents(isSubsession, clearSubsession))
 
-    payloadObj.histograms = histograms[HISTOGRAM_SUFFIXES.PARENT] || {};
-    payloadObj.keyedHistograms = keyedHistograms[HISTOGRAM_SUFFIXES.PARENT] || {};
+    payloadObj.histograms = histograms.parent || {};
+    payloadObj.keyedHistograms = keyedHistograms.parent || {};
     payloadObj.processes = {
       parent: {
         scalars: scalars["parent"] || {},
         keyedScalars: keyedScalars["parent"] || {},
         events: events["parent"] || [],
       },
       content: {
         scalars: scalars["content"],
         keyedScalars: keyedScalars["content"],
-        histograms: histograms[HISTOGRAM_SUFFIXES.CONTENT],
-        keyedHistograms: keyedHistograms[HISTOGRAM_SUFFIXES.CONTENT],
+        histograms: histograms["content"],
+        keyedHistograms: keyedHistograms["content"],
         events: events["content"] || [],
       },
       extension: {
         scalars: scalars["extension"],
         keyedScalars: keyedScalars["extension"],
-        histograms: histograms[HISTOGRAM_SUFFIXES.EXTENSION],
-        keyedHistograms: keyedHistograms[HISTOGRAM_SUFFIXES.EXTENSION],
+        histograms: histograms["extension"],
+        keyedHistograms: keyedHistograms["extension"],
         events: events["extension"] || [],
       },
     };
 
     // Only include the GPU process if we've accumulated data for it.
-    if (HISTOGRAM_SUFFIXES.GPU in histograms ||
-        HISTOGRAM_SUFFIXES.GPU in keyedHistograms ||
+    if ("gpu" in histograms ||
+        "gpu" in keyedHistograms ||
         "gpu" in scalars ||
         "gpu" in keyedScalars) {
       payloadObj.processes.gpu = {
         scalars: scalars["gpu"],
         keyedScalars: keyedScalars["gpu"],
-        histograms: histograms[HISTOGRAM_SUFFIXES.GPU],
-        keyedHistograms: keyedHistograms[HISTOGRAM_SUFFIXES.GPU],
+        histograms: histograms["gpu"],
+        keyedHistograms: keyedHistograms["gpu"],
         events: events["gpu"] || [],
       };
     }
 
     payloadObj.info = info;
 
     // Add extended set measurements for chrome process.
     if (Telemetry.canRecordExtended) {
--- a/toolkit/components/telemetry/nsITelemetry.idl
+++ b/toolkit/components/telemetry/nsITelemetry.idl
@@ -47,18 +47,18 @@ interface nsITelemetry : nsISupports
    * DATASET_RELEASE_CHANNEL_OPTIN - the extended dataset that is opt-in on release,
    *                                 opt-out on pre-release channels.
    */
   const unsigned long DATASET_RELEASE_CHANNEL_OPTOUT = 0;
   const unsigned long DATASET_RELEASE_CHANNEL_OPTIN = 1;
 
 
   /**
-   * An object containing a snapshot from all of the currently registered histograms.
-   * { name1: {data1}, name2:{data2}...}
+   * An object containing a snapshot from all of the currently registered histograms from all processes.
+   * { process1: {name1: {data1}, name2:{data2}...} }
    * where data is consists of the following properties:
    *   min - Minimal bucket size
    *   max - Maximum bucket size
    *   histogram_type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN
    *                    or HISTOGRAM_COUNT
    *   counts - array representing contents of the buckets in the histogram
    *   ranges -  an array with calculated bucket sizes
    *   sum - sum of the bucket contents