Bug 1468761 - Always serialize histograms in packed format r=chutten
☠☠ backed out by a67cc39bf78a ☠ ☠
authorJan-Erik Rediger <jrediger@mozilla.com>
Mon, 22 Oct 2018 15:21:45 +0000
changeset 490755 65bfae07e0f4fdcc22bbf909ffbe92f7224f546b
parent 490698 6c8bbe5f25a750a29707105cc1d57871af9e1fae
child 490756 9ee8406cf1d3585cb4090101ec48aa7fc696bc97
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewerschutten
bugs1468761
milestone64.0a1
Bug 1468761 - Always serialize histograms in packed format r=chutten Differential Revision: https://phabricator.services.mozilla.com/D9235
toolkit/components/telemetry/app/TelemetryUtils.jsm
toolkit/components/telemetry/core/TelemetryHistogram.cpp
toolkit/components/telemetry/core/nsITelemetry.idl
--- a/toolkit/components/telemetry/app/TelemetryUtils.jsm
+++ b/toolkit/components/telemetry/app/TelemetryUtils.jsm
@@ -21,67 +21,16 @@ const PREF_TELEMETRY_ENABLED = "toolkit.
 const IS_CONTENT_PROCESS = (function() {
   // We cannot use Services.appinfo here because in telemetry xpcshell tests,
   // appinfo is initially unavailable, and becomes available only later on.
   // eslint-disable-next-line mozilla/use-services
   let runtime = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
   return runtime.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT;
 })();
 
-/**
- * When reflecting a histogram into JS, Telemetry hands us an object
- * with the following properties:
- *
- * - min, max, histogram_type, sum, sum_squares_{lo,hi}: simple integers;
- * - counts: array of counts for histogram buckets;
- * - ranges: array of calculated bucket sizes.
- *
- * This format is not straightforward to read and potentially bulky
- * with lots of zeros in the counts array.  Packing histograms makes
- * raw histograms easier to read and compresses the data a little bit.
- *
- * Returns an object:
- * { range: [min, max], bucket_count: <number of buckets>,
- *   histogram_type: <histogram_type>, sum: <sum>,
- *   values: { bucket1: count1, bucket2: count2, ... } }
- */
-function packHistogram(hgram) {
-  let r = hgram.ranges;
-  let c = hgram.counts;
-  let retgram = {
-    range: [r[1], r[r.length - 1]],
-    bucket_count: r.length,
-    histogram_type: hgram.histogram_type,
-    values: {},
-    sum: hgram.sum,
-  };
-
-  let first = true;
-  let last = 0;
-
-  for (let i = 0; i < c.length; i++) {
-    let value = c[i];
-    if (!value)
-      continue;
-
-    // add a lower bound
-    if (i && first) {
-      retgram.values[r[i - 1]] = 0;
-    }
-    first = false;
-    last = i + 1;
-    retgram.values[r[i]] = value;
-  }
-
-  // add an upper bound
-  if (last && last < c.length)
-    retgram.values[r[last]] = 0;
-  return retgram;
-}
-
 var TelemetryUtils = {
   Preferences: Object.freeze({
     // General Preferences
     ArchiveEnabled: "toolkit.telemetry.archive.enabled",
     CachedClientId: "toolkit.telemetry.cachedClientID",
     FirstRun: "toolkit.telemetry.reportingpolicy.firstRun",
     FirstShutdownPingEnabled: "toolkit.telemetry.firstShutdownPing.enabled",
     HealthPingEnabled: "toolkit.telemetry.healthping.enabled",
@@ -319,17 +268,17 @@ var TelemetryUtils = {
    */
   packHistograms(snapshot, testingMode = false) {
     let ret = {};
 
     for (let [process, histograms] of Object.entries(snapshot)) {
       ret[process] = {};
       for (let [name, value] of Object.entries(histograms)) {
         if (testingMode || !name.startsWith("TELEMETRY_TEST_")) {
-          ret[process][name] = packHistogram(value);
+          ret[process][name] = value;
         }
       }
     }
 
     return ret;
   },
 
   /**
@@ -368,17 +317,17 @@ var TelemetryUtils = {
         if (testingMode || !name.startsWith("TELEMETRY_TEST_")) {
           let keys = Object.keys(value);
           if (keys.length == 0) {
             // Skip empty keyed histogram
             continue;
           }
           ret[process][name] = {};
           for (let [key, hgram] of Object.entries(value)) {
-            ret[process][name][key] = packHistogram(hgram);
+            ret[process][name][key] = hgram;
           }
         }
       }
     }
 
     return ret;
   },
 
--- a/toolkit/components/telemetry/core/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/core/TelemetryHistogram.cpp
@@ -742,63 +742,97 @@ internal_GetHistogramAndSamples(const St
   }
 
   // Finally, save the |sum|. We don't need to reflect declared_min, declared_max and
   // histogram_type as they are in gHistogramInfo.
   aSnapshot.mSampleSum = ss.sum();
   return NS_OK;
 }
 
+/**
+ * Reflect a histogram snapshot into a JavaScript object.
+ * The returned histogram object will have the following properties:
+ *
+ *   bucket_count - Number of buckets of this histogram
+ *   histogram_type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN,
+ *                    HISTOGRAM_FLAG, HISTOGRAM_COUNT, or HISTOGRAM_CATEGORICAL
+ *   sum - sum of the bucket contents
+ *   range - A 2-item array of minimum and maximum bucket size
+ *   values - Map from bucket start to the bucket's count
+ */
 nsresult
 internal_ReflectHistogramAndSamples(JSContext *cx,
                                     JS::Handle<JSObject*> obj,
                                     const HistogramInfo& aHistogramInfo,
                                     const HistogramSnapshotData& aSnapshot)
 {
-  if (!(JS_DefineProperty(cx, obj, "min",
-                          aHistogramInfo.min, JSPROP_ENUMERATE)
-        && JS_DefineProperty(cx, obj, "max",
-                             aHistogramInfo.max, JSPROP_ENUMERATE)
+  if (!(JS_DefineProperty(cx, obj, "bucket_count",
+                             aHistogramInfo.bucketCount, JSPROP_ENUMERATE)
         && JS_DefineProperty(cx, obj, "histogram_type",
                              aHistogramInfo.histogramType, JSPROP_ENUMERATE)
         && JS_DefineProperty(cx, obj, "sum",
                              double(aSnapshot.mSampleSum), JSPROP_ENUMERATE))) {
     return NS_ERROR_FAILURE;
   }
 
   // Don't rely on the bucket counts from "aHistogramInfo": it may
   // differ from the length of aSnapshot.mBucketCounts due to expired
   // histograms.
   const size_t count = aSnapshot.mBucketCounts.Length();
   MOZ_ASSERT(count == aSnapshot.mBucketRanges.Length(),
              "The number of buckets and the number of counts must match.");
 
-  // Create the "ranges" property and add it to the final object.
-  JS::Rooted<JSObject*> rarray(cx, JS_NewArrayObject(cx, count));
-  if (!rarray
-      || !JS_DefineProperty(cx, obj, "ranges", rarray, JSPROP_ENUMERATE)) {
+  // Create the "range" property and add it to the final object.
+  JS::Rooted<JSObject*> rarray(cx, JS_NewArrayObject(cx, 2));
+  if (rarray == nullptr
+      || !JS_DefineProperty(cx, obj, "range", rarray, JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+  // Add [min, max] into the range array
+  if (!JS_DefineElement(cx, rarray, 0, aHistogramInfo.min, JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+  if (!JS_DefineElement(cx, rarray, 1, aHistogramInfo.max, JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+
+  JS::Rooted<JSObject*> values(cx, JS_NewPlainObject(cx));
+  if (values == nullptr
+      || !JS_DefineProperty(cx, obj, "values", values, JSPROP_ENUMERATE)) {
     return NS_ERROR_FAILURE;
   }
 
-  // Fill the "ranges" property.
+  bool first = true;
+  size_t last = 0;
+
   for (size_t i = 0; i < count; i++) {
-    if (!JS_DefineElement(cx, rarray, i, aSnapshot.mBucketRanges[i], JSPROP_ENUMERATE)) {
+    auto value = aSnapshot.mBucketCounts[i];
+    if (value == 0) {
+      continue;
+    }
+
+    if (i > 0 && first) {
+      auto range = aSnapshot.mBucketRanges[i - 1];
+      if (!JS_DefineProperty(cx, values, nsPrintfCString("%d", range).get(), 0, JSPROP_ENUMERATE)) {
+        return NS_ERROR_FAILURE;
+      }
+    }
+
+    first = false;
+    last = i + 1;
+
+    auto range = aSnapshot.mBucketRanges[i];
+    if (!JS_DefineProperty(cx, values, nsPrintfCString("%d", range).get(), value, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
   }
 
-  JS::Rooted<JSObject*> counts_array(cx, JS_NewArrayObject(cx, count));
-  if (!counts_array
-      || !JS_DefineProperty(cx, obj, "counts", counts_array, JSPROP_ENUMERATE)) {
-    return NS_ERROR_FAILURE;
-  }
-
-  // Fill the "counts" property.
-  for (size_t i = 0; i < count; i++) {
-    if (!JS_DefineElement(cx, counts_array, i, aSnapshot.mBucketCounts[i], JSPROP_ENUMERATE)) {
+  if (last > 0 && last < count) {
+    auto range = aSnapshot.mBucketRanges[last];
+    if (!JS_DefineProperty(cx, values, nsPrintfCString("%d", range).get(), 0, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
   }
 
   return NS_OK;
 }
 
 bool
--- a/toolkit/components/telemetry/core/nsITelemetry.idl
+++ b/toolkit/components/telemetry/core/nsITelemetry.idl
@@ -51,25 +51,23 @@ interface nsITelemetry : nsISupports
   const unsigned long DATASET_RELEASE_CHANNEL_OPTIN = 1;
 
 
   /**
    * Serializes the histograms from the given store to a JSON-style object.
    * The returned structure looks like:
    *   { "process": { "name1": histogramData1, "name2": histogramData2 }, ... }
    *
-   * Where histogramDataN has the following properties:
-   *   min - minimum bucket size
-   *   max - maximum bucket size
+   * Each histogram is represented in a packed format and has the following properties:
+   *   bucket_count - Number of buckets of this histogram
    *   histogram_type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN,
    *                    HISTOGRAM_FLAG, HISTOGRAM_COUNT, or HISTOGRAM_CATEGORICAL
-   *   counts - array representing contents of the buckets in the histogram
-   *   ranges - array with calculated bucket sizes
    *   sum - sum of the bucket contents
-   * TODO(bug 1468761): Return packed histograms.
+   *   range - A 2-item array of minimum and maximum bucket size
+   *   values - Map from bucket to the bucket's count
    *
    * @param aStoreName The name of the store to snapshot. Ignored at the moment.
    * @param aClearStore Whether to clear out the histograms in the named store after snapshotting.
    */
   [implicit_jscontext]
   jsval getSnapshotForHistograms(in ACString aStoreName, in boolean aClearStore);
 
   /**
@@ -103,26 +101,25 @@ interface nsITelemetry : nsISupports
    * @param aClearStore Whether to clear out the keyed scalars in the named store after snapshotting.
    */
   [implicit_jscontext]
   jsval getSnapshotForKeyedScalars(in ACString aStoreName, in boolean aClearStore);
 
   /**
    * Serializes the histograms from the given dataset to a JSON-style object.
    * The returned structure looks like:
-   *   { process1: {name1: {histogramData1}, name2:{histogramData2}...}}
+   *   { "process": { "name1": histogramData1, "name2": histogramData2 }, ... }
    *
-   * Where histogramDataN has the following properties:
-   *   min - minimum bucket size
-   *   max - maximum bucket size
+   * Each histogram is represented in a packed format and has the following properties:
+   *   bucket_count - Number of buckets of this histogram
    *   histogram_type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN,
    *                    HISTOGRAM_FLAG, HISTOGRAM_COUNT, or HISTOGRAM_CATEGORICAL
-   *   counts - array representing contents of the buckets in the histogram
-   *   ranges - array with calculated bucket sizes
    *   sum - sum of the bucket contents
+   *   range - A 2-item array of minimum and maximum bucket size
+   *   values - Map from bucket to the bucket's count
    *
    * @param aDataset DATASET_RELEASE_CHANNEL_OPTOUT or DATASET_RELEASE_CHANNEL_OPTIN.
    * @param aClear Whether to clear out the histograms after snapshotting
    */
   [implicit_jscontext]
   jsval snapshotHistograms(in uint32_t aDataset, in boolean aClear);
 
   /**