Bug 1347216 - Fix categorical keyed histograms. r=Dexter

This also fixes an existing bug where we checked for the wrong type in JSKeyedHistogram_Add().
KeyedHistogram::GetHistogramType() returns one of the histogram types from nsITelemetry.idl, not one of the chromium types (base::*).

I extended the test coverage accordingly, as it seemed to be lacking.

toolkit/components/telemetry/Histograms.json

@@ -6516,7 +6516,7 @@
     "keyed": true,
     "description": "a testing histogram; not meant to be touched"
   },
-    "TELEMETRY_TEST_KEYED_BOOLEAN": {
+  "TELEMETRY_TEST_KEYED_BOOLEAN": {
     "alert_emails": ["telemetry-client-dev@mozilla.com"],
     "expires_in_version": "never",
     "kind": "boolean",
@@ -6524,6 +6524,41 @@
     "bug_numbers": [1299144],
     "description": "a testing histogram; not meant to be touched"
   },
+  "TELEMETRY_TEST_KEYED_EXPONENTIAL": {
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "exponential",
+    "low": 1,
+    "high": 40000,
+    "n_buckets": 10,
+    "keyed": true,
+    "bug_numbers": [1347216],
+    "description": "a testing histogram; not meant to be touched"
+  },
+  "TELEMETRY_TEST_KEYED_LINEAR": {
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "linear",
+    "low": 1,
+    "high": 250000,
+    "n_buckets": 10,
+    "keyed": true,
+    "bug_numbers": [1347216],
+    "description": "a testing histogram; not meant to be touched"
+  },
+  "TELEMETRY_TEST_KEYED_CATEGORICAL": {
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "categorical",
+    "keyed": true,
+    "labels": [
+      "CommonLabel",
+      "Label2",
+      "Label3"
+    ],
+    "bug_numbers": [1347216],
+    "description": "a testing histogram; not meant to be touched"
+  },
   "TELEMETRY_TEST_RELEASE_OPTOUT": {
     "alert_emails": ["telemetry-client-dev@mozilla.com"],
     "expires_in_version": "never",

toolkit/components/telemetry/TelemetryHistogram.cpp

@@ -1482,6 +1482,7 @@ internal_JSHistogram_Add(JSContext *cx, unsigned argc, JS::Value *vp)
       return true;
     }
 
+    // Get label id value.
     nsresult rv = gHistograms[id].label_id(NS_ConvertUTF16toUTF8(label).get(), &value);
     if (NS_FAILED(rv)) {
       LogToBrowserConsole(nsIScriptError::errorFlag,
@@ -1728,22 +1729,47 @@ internal_JSKeyedHistogram_Add(JSContext *cx, unsigned argc, JS::Value *vp)
 
   // If we don't have an argument for the count histogram, assume an increment of 1.
   // Otherwise, make sure to run some sanity checks on the argument.
-  int32_t value = 1;
-  if ((type != base::CountHistogram::COUNT_HISTOGRAM) || (args.length() == 2)) {
+  uint32_t value = 1;
+  if ((type != nsITelemetry::HISTOGRAM_COUNT) || (args.length() == 2)) {
     if (args.length() < 2) {
       LogToBrowserConsole(nsIScriptError::errorFlag,
                           NS_LITERAL_STRING("Expected two arguments for this histogram type"));
       return true;
     }
 
-    if (!(args[1].isNumber() || args[1].isBoolean())) {
-      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a number"));
-      return true;
-    }
+    if (type == nsITelemetry::HISTOGRAM_CATEGORICAL && args[1].isString()) {
+      // For categorical histograms we allow passing a string argument that specifies the label.
+      mozilla::Telemetry::HistogramID id;
+      if (NS_FAILED(keyed->GetEnumId(id))) {
+        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed to get histogram id."));
+        return true;
+      }
 
-    if (!JS::ToInt32(cx, args[1], &value)) {
-      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed to convert argument"));
-      return true;
+      // Get label string.
+      nsAutoJSString label;
+      if (!label.init(cx, args[1])) {
+        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid string parameter"));
+        return true;
+      }
+
+      // Get label id value.
+      nsresult rv = gHistograms[id].label_id(NS_ConvertUTF16toUTF8(label).get(), &value);
+      if (NS_FAILED(rv)) {
+        LogToBrowserConsole(nsIScriptError::errorFlag,
+                            NS_LITERAL_STRING("Unknown label for categorical histogram"));
+        return true;
+      }
+    } else {
+      // All other accumulations expect one numerical argument.
+      if (!(args[1].isNumber() || args[1].isBoolean())) {
+        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a number"));
+        return true;
+      }
+
+      if (!JS::ToUint32(cx, args[1], &value)) {
+        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed to convert argument"));
+        return true;
+      }
     }
   }
 

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

@@ -132,6 +132,41 @@ add_task(function* test_parameterChecks() {
   }
 });
 
+add_task(function* test_parameterCounts() {
+  let histogramIds = [
+    "TELEMETRY_TEST_EXPONENTIAL",
+    "TELEMETRY_TEST_LINEAR",
+    "TELEMETRY_TEST_FLAG",
+    "TELEMETRY_TEST_CATEGORICAL",
+    "TELEMETRY_TEST_BOOLEAN",
+  ];
+
+  for (let id of histogramIds) {
+    let h = Telemetry.getHistogramById(id);
+    h.clear();
+    h.add();
+    Assert.equal(h.snapshot().sum, 0, "Calling add() without a value should only log an error.");
+    h.clear();
+  }
+});
+
+add_task(function* test_parameterCountsKeyed() {
+  let histogramIds = [
+    "TELEMETRY_TEST_KEYED_FLAG",
+    "TELEMETRY_TEST_KEYED_BOOLEAN",
+    "TELEMETRY_TEST_KEYED_EXPONENTIAL",
+    "TELEMETRY_TEST_KEYED_LINEAR",
+  ];
+
+  for (let id of histogramIds) {
+    let h = Telemetry.getKeyedHistogramById(id);
+    h.clear();
+    h.add("key");
+    Assert.equal(h.snapshot("key").sum, 0, "Calling add('key') without a value should only log an error.");
+    h.clear();
+  }
+});
+
 add_task(function* test_noSerialization() {
   // Instantiate the storage for this histogram and make sure it doesn't
   // get reflected into JS, as it has no interesting data in it.
@@ -608,9 +643,54 @@ add_task(function* test_keyed_count_histogram() {
   let allSnapshots = Telemetry.keyedHistogramSnapshots;
   Assert.deepEqual(allSnapshots[KEYED_ID], testSnapShot);
 
+  // Test clearing categorical histogram.
   h.clear();
   Assert.deepEqual(h.keys(), []);
   Assert.deepEqual(h.snapshot(), {});
+
+  // Test leaving out the value argument. That should increment by 1.
+  h.add("key");
+  Assert.equal(h.snapshot("key").sum, 1);
+});
+
+add_task(function* test_keyed_categorical_histogram() {
+  const KEYED_ID = "TELEMETRY_TEST_KEYED_CATEGORICAL";
+  const KEYS = numberRange(0, 5).map(i => "key" + (i + 1));
+
+  let h = Telemetry.getKeyedHistogramById(KEYED_ID);
+
+  for (let k of KEYS) {
+    // Test adding both per label and index.
+    for (let v of ["CommonLabel", "Label2", "Label3", "Label3", 0, 0, 1]) {
+      h.add(k, v);
+    }
+
+    // The |add| method should not throw for unexpected values, but rather
+    // print an error message in the console.
+    for (let s of ["", "Label4", "1234"]) {
+      h.add(k, s);
+    }
+  }
+
+  // Categorical histograms default to 50 linear buckets.
+  let expectedRanges = [];
+  for (let i = 0; i < 51; ++i) {
+    expectedRanges.push(i);
+  }
+
+  // Check that the set of keys in the snapshot is what we expect.
+  let snapshot = h.snapshot();
+  let snapshotKeys = Object.keys(snapshot);
+  Assert.equal(KEYS.length, snapshotKeys.length);
+  Assert.ok(KEYS.every(k => snapshotKeys.includes(k)));
+
+  // Check the snapshot values.
+  for (let k of KEYS) {
+    Assert.ok(k in snapshot);
+    Assert.equal(snapshot[k].sum, 6);
+    Assert.deepEqual(snapshot[k].ranges, expectedRanges);
+    Assert.deepEqual(snapshot[k].counts.slice(0, 4), [3, 2, 2, 0]);
+  }
 });
 
 add_task(function* test_keyed_flag_histogram() {