Bug 1343855 - Part 2 - Add support for key whitelists in keyed histograms. r=gfritzsche, data-review=francois

MozReview-Commit-ID: H2pE3VivWIX

toolkit/components/telemetry/Histograms.json

@@ -7421,6 +7421,20 @@
     "keyed": true,
     "description": "a testing histogram; not meant to be touched"
   },
+  "TELEMETRY_TEST_KEYED_KEYS": {
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "record_in_processes": ["main", "content"],
+    "bug_numbers": [1343855],
+    "expires_in_version": "never",
+    "kind": "boolean",
+    "keyed": true,
+    "keys": [
+      "testkey",
+      "CommonKey",
+      "thirdKey"
+    ],
+    "description": "a testing histogram; not meant to be touched"
+  },
   "TELEMETRY_TEST_KEYED_BOOLEAN": {
     "record_in_processes": ["main", "content"],
     "alert_emails": ["telemetry-client-dev@mozilla.com"],

toolkit/components/telemetry/Scalars.yaml

@@ -690,6 +690,23 @@ navigator.storage:
       - 'main'
       - 'content'
 
+telemetry:
+  accumulate_unknown_histogram_keys:
+    bug_numbers:
+      - 1343855
+    description: >
+      The count of attempted accumulations to unknown histogram keys for
+      histograms that restrict the set of allowed keys ('keys' property).
+      The names of the offending histograms are used as keys in this probe.
+    expires: never
+    kind: uint
+    keyed: true
+    notification_emails:
+      - telemetry-client-dev@mozilla.com
+    release_channel_collection: opt-out
+    record_in_processes:
+      - all
+
 telemetry.discarded:
   accumulations:
     bug_numbers:

toolkit/components/telemetry/TelemetryHistogram.cpp

@@ -13,6 +13,7 @@
 #include "nsBaseHashtable.h"
 #include "nsClassHashtable.h"
 #include "nsITelemetry.h"
+#include "nsPrintfCString.h"
 
 #include "mozilla/dom/ToJSValue.h"
 #include "mozilla/gfx/GPUProcessManager.h"
@@ -23,6 +24,7 @@
 
 #include "TelemetryCommon.h"
 #include "TelemetryHistogram.h"
+#include "TelemetryScalar.h"
 #include "ipc/TelemetryIPCAccumulator.h"
 
 #include "base/histogram.h"
@@ -120,12 +122,15 @@ struct HistogramInfo {
   uint32_t dataset;
   uint32_t label_index;
   uint32_t label_count;
+  uint32_t key_index;
+  uint32_t key_count;
   RecordedProcessType record_in_processes;
   bool keyed;
 
   const char *name() const;
   const char *expiration() const;
   nsresult label_id(const char* label, uint32_t* labelId) const;
+  bool allows_key(const nsACString& key) const;
 };
 
 enum reflectStatus {
@@ -423,6 +428,32 @@ HistogramInfo::label_id(const char* label, uint32_t* labelId) const
   return NS_ERROR_FAILURE;
 }
 
+bool
+HistogramInfo::allows_key(const nsACString& key) const
+{
+  MOZ_ASSERT(this->keyed);
+
+  // If we didn't specify a list of allowed keys, just return true.
+  if (this->key_count == 0) {
+    return true;
+  }
+
+  // Otherwise, check if |key| is in the list of allowed keys.
+  for (uint32_t i = 0; i < this->key_count; ++i) {
+    // gHistogramKeyTable contains the indices of the key strings in the
+    // gHistogramStringTable. They are stored in-order and consecutively,
+    // from the offset key_index to (key_index + key_count).
+    uint32_t string_offset = gHistogramKeyTable[this->key_index + i];
+    const char* const str = &gHistogramStringTable[string_offset];
+    if (key.EqualsASCII(str)) {
+      return true;
+    }
+  }
+
+  // |key| was not found.
+  return false;
+}
+
 } // namespace
 
 
@@ -1391,6 +1422,18 @@ internal_JSKeyedHistogram_Add(JSContext *cx, unsigned argc, JS::Value *vp)
     return true;
   }
 
+  // Check if we're allowed to record in the provided key, for this histogram.
+  if (!gHistogramInfos[id].allows_key(NS_ConvertUTF16toUTF8(key))) {
+    nsPrintfCString msg("%s - key '%s' not allowed for this keyed histogram",
+                        gHistogramInfos[id].name(),
+                        NS_ConvertUTF16toUTF8(key).get());
+    LogToBrowserConsole(nsIScriptError::errorFlag, NS_ConvertUTF8toUTF16(msg));
+    TelemetryScalar::Add(
+      mozilla::Telemetry::ScalarID::TELEMETRY_ACCUMULATE_UNKNOWN_HISTOGRAM_KEYS,
+      NS_ConvertASCIItoUTF16(gHistogramInfos[id].name()), 1);
+    return true;
+  }
+
   const uint32_t type = gHistogramInfos[id].histogramType;
 
   // If we don't have an argument for the count histogram, assume an increment of 1.
@@ -1794,6 +1837,18 @@ TelemetryHistogram::Accumulate(HistogramID aID,
     return;
   }
 
+  // Check if we're allowed to record in the provided key, for this histogram.
+  if (!gHistogramInfos[aID].allows_key(aKey)) {
+    nsPrintfCString msg("%s - key '%s' not allowed for this keyed histogram",
+                        gHistogramInfos[aID].name(),
+                        aKey.get());
+    LogToBrowserConsole(nsIScriptError::errorFlag, NS_ConvertUTF8toUTF16(msg));
+    TelemetryScalar::Add(
+      mozilla::Telemetry::ScalarID::TELEMETRY_ACCUMULATE_UNKNOWN_HISTOGRAM_KEYS,
+      NS_ConvertASCIItoUTF16(gHistogramInfos[aID].name()), 1);
+    return;
+  }
+
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
   internal_Accumulate(aID, aKey, aSample);
 }
@@ -1817,14 +1872,33 @@ void
 TelemetryHistogram::Accumulate(const char* name,
                                const nsCString& key, uint32_t sample)
 {
-  StaticMutexAutoLock locker(gTelemetryHistogramMutex);
-  if (!internal_CanRecordBase()) {
-    return;
-  }
-  HistogramID id;
-  nsresult rv = internal_GetHistogramIdByName(nsDependentCString(name), &id);
-  if (NS_SUCCEEDED(rv)) {
-    internal_Accumulate(id, key, sample);
+  bool keyNotAllowed = false;
+
+  {
+    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+    if (!internal_CanRecordBase()) {
+      return;
+    }
+    HistogramID id;
+    nsresult rv = internal_GetHistogramIdByName(nsDependentCString(name), &id);
+    if (NS_SUCCEEDED(rv)) {
+      // Check if we're allowed to record in the provided key, for this histogram.
+      if (gHistogramInfos[id].allows_key(key)) {
+        internal_Accumulate(id, key, sample);
+        return;
+      }
+      // We're holding |gTelemetryHistogramMutex|, so we can't print a message
+      // here.
+      keyNotAllowed = true;
+    }
+   }
+
+  if (keyNotAllowed) {
+    LogToBrowserConsole(nsIScriptError::errorFlag,
+                        NS_LITERAL_STRING("Key not allowed for this keyed histogram"));
+    TelemetryScalar::Add(
+      mozilla::Telemetry::ScalarID::TELEMETRY_ACCUMULATE_UNKNOWN_HISTOGRAM_KEYS,
+      NS_ConvertASCIItoUTF16(name), 1);
   }
 }
 

toolkit/components/telemetry/docs/collection/histograms.rst

@@ -149,6 +149,10 @@ Required. One of the histogram types described in the previous section. Differen
 ---------
 Optional, boolean, defaults to ``false``. Determines whether this is a *keyed histogram*.
 
+``keys``
+---------
+Optional, list of strings. Only valid for *keyed histograms*. Defines a case sensitive list of allowed keys that can be used for this histogram. The list is limited to 30 keys with a maximum length of 20 characters. When using a key that is not in the list, the accumulation is discarded and a warning is printed to the browser console.
+
 ``low``
 -------
 Optional, the default value is ``1``. This field represents the minimum value expected in the histogram. Note that all histograms automatically get a bucket with label ``0`` for counting values below the ``low`` value. If a histogram does not specify a ``low`` value, it will always have a ``"0"`` bucket (for negative or zero values) and a ``"1"`` bucket (for values between ``1`` and the next bucket).

toolkit/components/telemetry/gen-histogram-data.py

@@ -16,11 +16,12 @@ banner = """/* This file is auto-generated, see gen-histogram-data.py.  */
 """
 
 
-def print_array_entry(output, histogram, name_index, exp_index, label_index, label_count):
+def print_array_entry(output, histogram, name_index, exp_index, label_index,
+                      label_count, key_index, key_count):
     cpp_guard = histogram.cpp_guard()
     if cpp_guard:
         print("#if defined(%s)" % cpp_guard, file=output)
-    print("  { %s, %s, %s, %s, %d, %d, %s, %d, %d, %s, %s },"
+    print("  { %s, %s, %s, %s, %d, %d, %s, %d, %d, %d, %d, %s, %s },"
           % (histogram.low(),
              histogram.high(),
              histogram.n_buckets(),
@@ -30,6 +31,8 @@ def print_array_entry(output, histogram, name_index, exp_index, label_index, lab
              histogram.dataset(),
              label_index,
              label_count,
+             key_index,
+             key_count,
              " | ".join(histogram.record_in_processes_enum()),
              "true" if histogram.keyed() else "false"), file=output)
     if cpp_guard:
@@ -40,6 +43,8 @@ def write_histogram_table(output, histograms):
     string_table = StringTable()
     label_table = []
     label_count = 0
+    keys_table = []
+    keys_count = 0
 
     print("constexpr HistogramInfo gHistogramInfos[] = {", file=output)
     for histogram in histograms:
@@ -53,9 +58,15 @@ def write_histogram_table(output, histograms):
             label_table.append((histogram.name(), string_table.stringIndexes(labels)))
             label_count += len(labels)
 
-        print_array_entry(output, histogram,
-                          name_index, exp_index,
-                          label_index, len(labels))
+        keys = histogram.keys()
+        key_index = 0
+        if len(keys) > 0:
+            key_index = keys_count
+            keys_table.append((histogram.name(), string_table.stringIndexes(keys)))
+            keys_count += len(keys)
+
+        print_array_entry(output, histogram, name_index, exp_index,
+                          label_index, len(labels), key_index, len(keys))
     print("};\n", file=output)
 
     strtab_name = "gHistogramStringTable"
@@ -68,6 +79,11 @@ def write_histogram_table(output, histograms):
         print("/* %s */ %s," % (name, ", ".join(map(str, indexes))), file=output)
     print("};", file=output)
 
+    print("\nconst uint32_t gHistogramKeyTable[] = {", file=output)
+    for name, indexes in keys_table:
+        print("/* %s */ %s," % (name, ", ".join(map(str, indexes))), file=output)
+    print("};", file=output)
+
 
 # Write out static asserts for histogram data.  We'd prefer to perform
 # these checks in this script itself, but since several histograms

toolkit/components/telemetry/tests/gtest/TestHistograms.cpp

@@ -120,3 +120,54 @@ TEST_F(TelemetryTestFixture, AccumulateKeyedCountHistogram)
   JS::ToUint32(cx.GetJSContext(), sum, &uSum);
   ASSERT_EQ(uSum, kExpectedValue) << "The histogram is not returning expected sum";
 }
+
+TEST_F(TelemetryTestFixture, TestKeyedKeysHistogram)
+{
+  AutoJSContextWithGlobal cx(mCleanGlobal);
+
+  JS::RootedValue testHistogram(cx.GetJSContext());
+  JS::RootedValue rval(cx.GetJSContext());
+
+  GetAndClearHistogram(cx.GetJSContext(), mTelemetry,
+                       NS_LITERAL_CSTRING("TELEMETRY_TEST_KEYED_KEYS"), true);
+
+  // Test the accumulation on both the allowed and unallowed keys, using
+  // the API that accepts histogram IDs.
+  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_KEYED_KEYS, NS_LITERAL_CSTRING("not-allowed"), 1);
+  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_KEYED_KEYS, NS_LITERAL_CSTRING("testkey"), 0);
+  // Do the same, using the API that accepts the histogram name as a string.
+  Telemetry::Accumulate("TELEMETRY_TEST_KEYED_KEYS", NS_LITERAL_CSTRING("not-allowed"), 1);
+  Telemetry::Accumulate("TELEMETRY_TEST_KEYED_KEYS", NS_LITERAL_CSTRING("CommonKey"), 1);
+
+  // Get a snapshot for all the histograms
+  JS::RootedValue snapshot(cx.GetJSContext());
+  GetSnapshots(cx.GetJSContext(), mTelemetry, "TELEMETRY_TEST_KEYED_KEYS", &snapshot, true);
+
+  // Get the histogram from the snapshot
+  JS::RootedValue histogram(cx.GetJSContext());
+  GetProperty(cx.GetJSContext(), "TELEMETRY_TEST_KEYED_KEYS", snapshot, &histogram);
+
+  // Get "testkey" property from histogram and check that it stores the correct
+  // data.
+  JS::RootedValue expectedKeyData(cx.GetJSContext());
+  GetProperty(cx.GetJSContext(), "testkey", histogram,  &expectedKeyData);
+  ASSERT_TRUE(!expectedKeyData.isUndefined())
+    << "Cannot find the expected key in the histogram data";
+  JS::RootedValue sum(cx.GetJSContext());
+  GetProperty(cx.GetJSContext(), "sum", expectedKeyData,  &sum);
+  uint32_t uSum = 0;
+  JS::ToUint32(cx.GetJSContext(), sum, &uSum);
+  ASSERT_EQ(uSum, 0U) << "The histogram is not returning expected sum for 'testkey'";
+
+  // Do the same for the "CommonKey" property.
+  GetProperty(cx.GetJSContext(), "CommonKey", histogram,  &expectedKeyData);
+  ASSERT_TRUE(!expectedKeyData.isUndefined())
+    << "Cannot find the expected key in the histogram data";
+  GetProperty(cx.GetJSContext(), "sum", expectedKeyData,  &sum);
+  JS::ToUint32(cx.GetJSContext(), sum, &uSum);
+  ASSERT_EQ(uSum, 1U) << "The histogram is not returning expected sum for 'CommonKey'";
+
+  GetProperty(cx.GetJSContext(), "not-allowed", histogram,  &expectedKeyData);
+  ASSERT_TRUE(expectedKeyData.isUndefined())
+    << "Unallowed keys must not be recorded in the histogram data";
+}

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

@@ -947,3 +947,37 @@ function test_keyed_subsession() {
   Assert.ok(!(KEY in subsession));
   Assert.equal(h.subsessionSnapshot(KEY).sum, 0);
 });
+
+add_task(function* test_keyed_keys() {
+  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_KEYS");
+  h.clear();
+  Telemetry.clearScalars();
+
+  // The |add| method should not throw for keys that are not allowed.
+  h.add("testkey", true);
+  h.add("thirdKey", false);
+  h.add("not-allowed", true);
+
+  // Check that we have the expected keys.
+  let snap = h.snapshot();
+  Assert.ok(Object.keys(snap).length, 2, "Only 2 keys must be recorded.");
+  Assert.ok("testkey" in snap, "'testkey' must be recorded.");
+  Assert.ok("thirdKey" in snap, "'thirdKey' must be recorded.");
+  Assert.deepEqual(snap.testkey.counts, [0, 1, 0],
+                   "'testkey' must contain the correct value.");
+  Assert.deepEqual(snap.thirdKey.counts, [1, 0, 0],
+                   "'thirdKey' must contain the correct value.");
+
+  // Keys that are not allowed must not be recorded.
+  Assert.ok(!("not-allowed" in snap), "'not-allowed' must not be recorded.");
+
+  // Check that these failures were correctly tracked.
+  const parentScalars =
+    Telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false).parent;
+  const scalarName = "telemetry.accumulate_unknown_histogram_keys";
+  Assert.ok(scalarName in parentScalars, "Accumulation to unallowed keys must be reported.");
+  Assert.ok("TELEMETRY_TEST_KEYED_KEYS" in parentScalars[scalarName],
+            "Accumulation to unallowed keys must be recorded with the correct key.");
+  Assert.equal(parentScalars[scalarName].TELEMETRY_TEST_KEYED_KEYS, 1,
+               "Accumulation to unallowed keys must report the correct value.");
+});