Bug 1299144 - Replace TelemetryHistogram::NewKeyedHistogram and replace with predefined histograms. r?chutten draft
authorAdamG2 <adamgj.wong@gmail.com>
Tue, 13 Sep 2016 01:04:39 -0400
changeset 412886 345b1dac51728fd5a52218b51b296a00259e32a3
parent 412489 cfdb7af3af2e92e95f71ca2f1672bf5433beeb89
child 531087 4cbe480570d435c910b400e72916b858bc5d59e4
push id29278
push userbmo:adamgj.wong@gmail.com
push dateTue, 13 Sep 2016 05:32:17 +0000
reviewerschutten
bugs1299144
milestone51.0a1
Bug 1299144 - Replace TelemetryHistogram::NewKeyedHistogram and replace with predefined histograms. r?chutten MozReview-Commit-ID: FLRyEswR6r7
toolkit/components/telemetry/Histograms.json
toolkit/components/telemetry/Telemetry.cpp
toolkit/components/telemetry/TelemetryHistogram.cpp
toolkit/components/telemetry/TelemetryHistogram.h
toolkit/components/telemetry/nsITelemetry.idl
toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -5508,16 +5508,24 @@
     "description": "a testing histogram; not meant to be touched"
   },
   "TELEMETRY_TEST_KEYED_COUNT": {
     "expires_in_version": "never",
     "kind": "count",
     "keyed": true,
     "description": "a testing histogram; not meant to be touched"
   },
+    "TELEMETRY_TEST_KEYED_BOOLEAN": {
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "boolean",
+    "keyed": true,
+    "bug_numbers": [1299144],
+    "description": "a testing histogram; not meant to be touched"
+  },
   "TELEMETRY_TEST_RELEASE_OPTOUT": {
     "expires_in_version": "never",
     "kind": "flag",
     "releaseChannelCollection": "opt-out",
     "description": "a testing histogram; not meant to be touched"
   },
   "TELEMETRY_TEST_RELEASE_OPTIN": {
     "expires_in_version": "never",
--- a/toolkit/components/telemetry/Telemetry.cpp
+++ b/toolkit/components/telemetry/Telemetry.cpp
@@ -1025,27 +1025,16 @@ TelemetryImpl::~TelemetryImpl() {
   UnregisterWeakMemoryReporter(this);
 }
 
 void
 TelemetryImpl::InitMemoryReporter() {
   RegisterWeakMemoryReporter(this);
 }
 
-NS_IMETHODIMP
-TelemetryImpl::NewKeyedHistogram(const nsACString &name, const nsACString &expiration, uint32_t histogramType,
-                            uint32_t min, uint32_t max, uint32_t bucketCount, JSContext *cx,
-                            uint8_t optArgCount, JS::MutableHandle<JS::Value> ret)
-{
-  return TelemetryHistogram::NewKeyedHistogram(name, expiration, histogramType,
-                                               min, max, bucketCount,
-                                               cx, optArgCount, ret);
-}
-
-
 bool
 TelemetryImpl::ReflectSQL(const SlowSQLEntryType *entry,
                           const Stat *stat,
                           JSContext *cx,
                           JS::Handle<JSObject*> obj)
 {
   if (stat->hitCount == 0)
     return true;
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -226,22 +226,16 @@ internal_CanRecordExtended() {
 
 bool
 internal_IsHistogramEnumId(mozilla::Telemetry::ID aID)
 {
   static_assert(((mozilla::Telemetry::ID)-1 > 0), "ID should be unsigned.");
   return aID < mozilla::Telemetry::HistogramCount;
 }
 
-bool
-internal_IsValidHistogramName(const nsACString& name)
-{
-  return !FindInReadable(NS_LITERAL_CSTRING(KEYED_HISTOGRAM_NAME_SEPARATOR), name);
-}
-
 // Note: this is completely unrelated to mozilla::IsEmpty.
 bool
 internal_IsEmpty(const Histogram *h)
 {
   Histogram::SampleSet ss;
   h->SnapshotSample(&ss);
   return ss.counts(0) == 0 && ss.sum() == 0;
 }
@@ -2008,52 +2002,16 @@ const char*
 TelemetryHistogram::GetHistogramName(mozilla::Telemetry::ID id)
 {
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
   const HistogramInfo& h = gHistograms[id];
   return h.id();
 }
 
 nsresult
-TelemetryHistogram::NewKeyedHistogram(const nsACString &name,
-                                      const nsACString &expiration,
-                                      uint32_t histogramType,
-                                      uint32_t min, uint32_t max,
-                                      uint32_t bucketCount, JSContext *cx,
-                                      uint8_t optArgCount,
-                                      JS::MutableHandle<JS::Value> ret)
-{
-  KeyedHistogram* keyed = nullptr;
-  {
-    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
-    if (!internal_IsValidHistogramName(name)) {
-      return NS_ERROR_INVALID_ARG;
-    }
-
-    nsresult rv
-      = internal_CheckHistogramArguments(histogramType, min, max,
-                                         bucketCount, optArgCount == 3);
-    if (NS_FAILED(rv)) {
-      return rv;
-    }
-
-    keyed = new KeyedHistogram(name, expiration, histogramType,
-                               min, max, bucketCount,
-                               nsITelemetry::DATASET_RELEASE_CHANNEL_OPTIN);
-    if (MOZ_UNLIKELY(!gKeyedHistograms.Put(name, keyed, mozilla::fallible))) {
-      delete keyed;
-      return NS_ERROR_OUT_OF_MEMORY;
-    }
-  }
-
-  // Runs without protection from |gTelemetryHistogramMutex|
-  return internal_WrapAndReturnKeyedHistogram(keyed, cx, ret);
-}
-
-nsresult
 TelemetryHistogram::HistogramFrom(const nsACString &name,
                                   const nsACString &existing_name,
                                   JSContext *cx,
                                   JS::MutableHandle<JS::Value> ret)
 {
   Histogram* clone = nullptr;
   {
     StaticMutexAutoLock locker(gTelemetryHistogramMutex);
--- a/toolkit/components/telemetry/TelemetryHistogram.h
+++ b/toolkit/components/telemetry/TelemetryHistogram.h
@@ -52,22 +52,16 @@ GetHistogramById(const nsACString &name,
 nsresult
 GetKeyedHistogramById(const nsACString &name, JSContext *cx,
                       JS::MutableHandle<JS::Value> ret);
 
 const char*
 GetHistogramName(mozilla::Telemetry::ID id);
 
 nsresult
-NewKeyedHistogram(const nsACString &name, const nsACString &expiration,
-                  uint32_t histogramType, uint32_t min, uint32_t max,
-                  uint32_t bucketCount, JSContext *cx,
-                  uint8_t optArgCount, JS::MutableHandle<JS::Value> ret);
-
-nsresult
 HistogramFrom(const nsACString &name, const nsACString &existing_name,
               JSContext *cx, JS::MutableHandle<JS::Value> ret);
 
 nsresult
 CreateHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret,
                          bool subsession, bool clearSubsession);
 
 nsresult
--- a/toolkit/components/telemetry/nsITelemetry.idl
+++ b/toolkit/components/telemetry/nsITelemetry.idl
@@ -210,53 +210,34 @@ interface nsITelemetry : nsISupports
    * An object containing a snapshot from all of the currently registered keyed histograms.
    * { name1: {histogramData1}, name2:{histogramData2}...}
    * where the histogramData is as described in histogramSnapshots.
    */
   [implicit_jscontext]
   readonly attribute jsval keyedHistogramSnapshots;
 
   /**
-   * Create and return a keyed histogram.  Parameters:
-   *
-   * @param name Unique histogram name
-   * @param expiration Expiration version
-   * @param type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN, HISTOGRAM_FLAG or HISTOGRAM_COUNT
-   * @param min - Minimal bucket size
-   * @param max - Maximum bucket size
-   * @param bucket_count - number of buckets in the histogram.
-   * The returned object has the following functions:
-   *   add(string key, [optional] int) - Add an int value to the histogram for that key. If no histogram for that key exists yet, it is created.
-   *   snapshot([optional] string key) - If key is provided, returns a snapshot for the histogram with that key or null. If key is not provided, returns the snapshots of all the registered keys in the form {key1: snapshot1, key2: snapshot2, ...}.
-   *   keys() - Returns an array with the string keys of the currently registered histograms
-   *   clear() - Clears the registered histograms from this.
-   *   dataset() - identifies what dataset this is in: DATASET_RELEASE_CHANNEL_OPTOUT or ...OPTIN
-   */
-  [implicit_jscontext, optional_argc]
-  jsval newKeyedHistogram(in ACString name,
-                          in ACString expiration,
-                          in unsigned long histogram_type,
-                          [optional] in uint32_t min,
-                          [optional] in uint32_t max,
-                          [optional] in uint32_t bucket_count);
-
-  /**
    * Returns an array whose values are the names of histograms defined
    * in Histograms.json.
    *
    * @param dataset - DATASET_RELEASE_CHANNEL_OPTOUT or ...OPTIN
    */
   void registeredKeyedHistograms(in uint32_t dataset, out uint32_t count,
                                  [retval, array, size_is(count)] out string histograms);
 
   /**
-   * Same as newKeyedHistogram above, but for histograms registered in TelemetryHistograms.h.
+   * Create and return a histogram registered in TelemetryHistograms.h.
    *
    * @param id - unique identifier from TelemetryHistograms.h
-   * The returned object has the same functions as a histogram returned from newKeyedHistogram.
+   * The returned object has the following functions:
+   *   add(string key, [optional] int) - Add an int value to the histogram for that key. If no histogram for that key exists yet, it is created.
+   *   snapshot([optional] string key) - If key is provided, returns a snapshot for the histogram with that key or null. If key is not provided, returns the snapshots of all the registered keys in the form {key1: snapshot1, key2: snapshot2, ...}.
+   *   keys() - Returns an array with the string keys of the currently registered histograms
+   *   clear() - Clears the registered histograms from this.
+   *   dataset() - identifies what dataset this is in: DATASET_RELEASE_CHANNEL_OPTOUT or ...OPTIN
    */
   [implicit_jscontext]
   jsval getKeyedHistogramById(in ACString id);
 
   /**
    * A flag indicating if Telemetry can record base data (FHR data). This is true if the
    * FHR data reporting service or self-support are enabled.
    *
--- a/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
+++ b/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ -457,50 +457,41 @@ add_task(function* test_expired_histogra
   do_check_eq(rh[test_expired_id], undefined);
 });
 
 add_task(function* test_keyed_histogram() {
   // Check that invalid names get rejected.
 
   let threw = false;
   try {
-    Telemetry.newKeyedHistogram("test::invalid # histogram", "never", Telemetry.HISTOGRAM_BOOLEAN);
-  } catch (e) {
-    // This should throw as we reject names with the # separator
-    threw = true;
-  }
-  Assert.ok(threw, "newKeyedHistogram should have thrown");
-
-  threw = false;
-  try {
     Telemetry.getKeyedHistogramById("test::unknown histogram", "never", Telemetry.HISTOGRAM_BOOLEAN);
   } catch (e) {
     // This should throw as it is an unknown ID
     threw = true;
   }
   Assert.ok(threw, "getKeyedHistogramById should have thrown");
 });
 
 add_task(function* test_keyed_boolean_histogram() {
-  const KEYED_ID = "test::keyed::boolean";
+  const KEYED_ID = "TELEMETRY_TEST_KEYED_BOOLEAN";
   let KEYS = numberRange(0, 2).map(i => "key" + (i + 1));
   KEYS.push("漢語");
   let histogramBase = {
     "min": 1,
     "max": 2,
     "histogram_type": 2,
     "sum": 1,
     "ranges": [0, 1, 2],
     "counts": [0, 1, 0]
   };
   let testHistograms = numberRange(0, 3).map(i => JSON.parse(JSON.stringify(histogramBase)));
   let testKeys = [];
   let testSnapShot = {};
 
-  let h = Telemetry.newKeyedHistogram(KEYED_ID, "never", Telemetry.HISTOGRAM_BOOLEAN);
+  let h = Telemetry.getKeyedHistogramById(KEYED_ID);
   for (let i=0; i<2; ++i) {
     let key = KEYS[i];
     h.add(key, true);
     testSnapShot[key] = testHistograms[i];
     testKeys.push(key);
 
     Assert.deepEqual(h.keys().sort(), testKeys);
     Assert.deepEqual(h.snapshot(), testSnapShot);
@@ -523,31 +514,31 @@ add_task(function* test_keyed_boolean_hi
   Assert.deepEqual(allSnapshots[KEYED_ID], testSnapShot);
 
   h.clear();
   Assert.deepEqual(h.keys(), []);
   Assert.deepEqual(h.snapshot(), {});
 });
 
 add_task(function* test_keyed_count_histogram() {
-  const KEYED_ID = "test::keyed::count";
+  const KEYED_ID = "TELEMETRY_TEST_KEYED_COUNT";
   const KEYS = numberRange(0, 5).map(i => "key" + (i + 1));
   let histogramBase = {
     "min": 1,
     "max": 2,
     "histogram_type": 4,
     "sum": 0,
     "ranges": [0, 1, 2],
     "counts": [1, 0, 0]
   };
   let testHistograms = numberRange(0, 5).map(i => JSON.parse(JSON.stringify(histogramBase)));
   let testKeys = [];
   let testSnapShot = {};
 
-  let h = Telemetry.newKeyedHistogram(KEYED_ID, "never", Telemetry.HISTOGRAM_COUNT);
+  let h = Telemetry.getKeyedHistogramById(KEYED_ID);
   for (let i=0; i<4; ++i) {
     let key = KEYS[i];
     let value = i*2 + 1;
 
     for (let k=0; k<value; ++k) {
       h.add(key);
     }
     testHistograms[i].counts[0] = value;
@@ -578,18 +569,18 @@ add_task(function* test_keyed_count_hist
   Assert.deepEqual(allSnapshots[KEYED_ID], testSnapShot);
 
   h.clear();
   Assert.deepEqual(h.keys(), []);
   Assert.deepEqual(h.snapshot(), {});
 });
 
 add_task(function* test_keyed_flag_histogram() {
-  const KEYED_ID = "test::keyed::flag";
-  let h = Telemetry.newKeyedHistogram(KEYED_ID, "never", Telemetry.HISTOGRAM_FLAG);
+  const KEYED_ID = "TELEMETRY_TEST_KEYED_FLAG";
+  let h = Telemetry.getKeyedHistogramById(KEYED_ID);
 
   const KEY = "default";
   h.add(KEY, true);
 
   let testSnapshot = {};
   testSnapshot[KEY] = {
     "min": 1,
     "max": 2,
@@ -629,22 +620,16 @@ add_task(function* test_keyed_histogram_
 
   // Extended set keyed histograms should not be recorded.
   h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_RELEASE_OPTIN");
   h.clear();
   h.add(TEST_KEY, 1);
   Assert.equal(h.snapshot(TEST_KEY).sum, 0,
                "The keyed histograms should not record any data.");
 
-  // Runtime created histograms should not be recorded.
-  h = Telemetry.newKeyedHistogram("test::runtime_keyed_boolean", "never", Telemetry.HISTOGRAM_BOOLEAN);
-  h.add(TEST_KEY, 1);
-  Assert.equal(h.snapshot(TEST_KEY).sum, 0,
-               "The keyed histogram should not record any data.");
-
   // Check that extended histograms are recorded when required.
   Telemetry.canRecordExtended = true;
 
   h.add(TEST_KEY, 1);
   Assert.equal(h.snapshot(TEST_KEY).sum, 1,
                   "The runtime keyed histogram should record the correct value.");
 
   h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_RELEASE_OPTIN");