Bug 1069953 - Part 1: Make min/max/bucket_count optional for nsITelemetry newHistogram(). r=froydnj, ba=lmandel
authorGeorg Fritzsche <georg.fritzsche@googlemail.com>
Tue, 28 Oct 2014 12:47:38 +0100
changeset 225906 56b3e37832b9
parent 225905 a4db8f39f372
child 225907 3fe1e43c97b8
push id4063
push usergeorg.fritzsche@googlemail.com
push date2014-11-02 23:54 +0000
treeherdermozilla-beta@1ca39da5df9d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1069953
milestone34.0
Bug 1069953 - Part 1: Make min/max/bucket_count optional for nsITelemetry newHistogram(). r=froydnj, ba=lmandel
toolkit/components/telemetry/Telemetry.cpp
toolkit/components/telemetry/nsITelemetry.idl
toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
--- a/toolkit/components/telemetry/Telemetry.cpp
+++ b/toolkit/components/telemetry/Telemetry.cpp
@@ -717,23 +717,32 @@ IsExpired(const char *expiration){
   return strcmp(expiration, "never") && (mozilla::Version(expiration) <= current_version);
 }
 
 bool
 IsExpired(const Histogram *histogram){
   return histogram->histogram_name() == EXPIRED_ID;
 }
 
+/*
+ * min, max & bucketCount are optional for boolean, flag & count histograms.
+ * haveOptArgs has to be set if the caller provides them.
+ */
 nsresult
-HistogramGet(const char *name, const char *expiration, uint32_t min, uint32_t max,
-             uint32_t bucketCount, uint32_t histogramType, Histogram **result)
+HistogramGet(const char *name, const char *expiration, uint32_t histogramType,
+             uint32_t min, uint32_t max, uint32_t bucketCount, bool haveOptArgs,
+             Histogram **result)
 {
   if (histogramType != nsITelemetry::HISTOGRAM_BOOLEAN
       && histogramType != nsITelemetry::HISTOGRAM_FLAG
       && histogramType != nsITelemetry::HISTOGRAM_COUNT) {
+    // The min, max & bucketCount arguments are not optional for this type.
+    if (!haveOptArgs)
+      return NS_ERROR_ILLEGAL_VALUE;
+
     // Sanity checks for histogram parameters.
     if (min >= max)
       return NS_ERROR_ILLEGAL_VALUE;
 
     if (bucketCount <= 2)
       return NS_ERROR_ILLEGAL_VALUE;
 
     if (min < 1)
@@ -777,17 +786,18 @@ GetHistogramByEnumId(Telemetry::ID id, H
   static Histogram* knownHistograms[Telemetry::HistogramCount] = {0};
   Histogram *h = knownHistograms[id];
   if (h) {
     *ret = h;
     return NS_OK;
   }
 
   const TelemetryHistogram &p = gHistograms[id];
-  nsresult rv = HistogramGet(p.id(), p.expiration(), p.min, p.max, p.bucketCount, p.histogramType, &h);
+  nsresult rv = HistogramGet(p.id(), p.expiration(), p.histogramType,
+                             p.min, p.max, p.bucketCount, true, &h);
   if (NS_FAILED(rv))
     return rv;
 
 #ifdef DEBUG
   // Check that the C++ Histogram code computes the same ranges as the
   // Python histogram code.
   if (!IsExpired(p.expiration())) {
     const struct bounds &b = gBucketLowerBoundIndex[id];
@@ -1275,23 +1285,23 @@ TelemetryImpl::~TelemetryImpl() {
 }
 
 void
 TelemetryImpl::InitMemoryReporter() {
   RegisterWeakMemoryReporter(this);
 }
 
 NS_IMETHODIMP
-TelemetryImpl::NewHistogram(const nsACString &name, const nsACString &expiration, uint32_t min,
-                            uint32_t max, uint32_t bucketCount, uint32_t histogramType,
-                            JSContext *cx, JS::MutableHandle<JS::Value> ret)
+TelemetryImpl::NewHistogram(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)
 {
   Histogram *h;
   nsresult rv = HistogramGet(PromiseFlatCString(name).get(), PromiseFlatCString(expiration).get(),
-                             min, max, bucketCount, histogramType, &h);
+                             histogramType, min, max, bucketCount, optArgCount == 3, &h);
   if (NS_FAILED(rv))
     return rv;
   h->ClearFlags(Histogram::kUmaTargetedHistogramFlag);
   h->SetFlags(Histogram::kExtendedStatisticsFlag);
   return WrapAndReturnHistogram(h, cx, ret);
 }
 
 bool
@@ -1409,18 +1419,19 @@ TelemetryImpl::HistogramFrom(const nsACS
   Histogram *existing;
   rv = GetHistogramByEnumId(id, &existing);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   Histogram *clone;
   rv = HistogramGet(PromiseFlatCString(name).get(), p.expiration(),
-                    existing->declared_min(), existing->declared_max(),
-                    existing->bucket_count(), p.histogramType, &clone);
+                    p.histogramType, existing->declared_min(),
+                    existing->declared_max(), existing->bucket_count(),
+                    true, &clone);
   if (NS_FAILED(rv))
     return rv;
 
   Histogram::SampleSet ss;
   existing->SnapshotSample(&ss);
   clone->AddSampleSet(ss);
   return WrapAndReturnHistogram(clone, cx, ret);
 }
@@ -1644,18 +1655,18 @@ TelemetryImpl::GetHistogramSnapshots(JSC
 }
 
 bool
 TelemetryImpl::CreateHistogramForAddon(const nsACString &name,
                                        AddonHistogramInfo &info)
 {
   Histogram *h;
   nsresult rv = HistogramGet(PromiseFlatCString(name).get(), "never",
-                             info.min, info.max, info.bucketCount,
-                             info.histogramType, &h);
+                             info.histogramType, info.min, info.max,
+                             info.bucketCount, true, &h);
   if (NS_FAILED(rv)) {
     return false;
   }
   // Don't let this histogram be reported via the normal means
   // (e.g. Telemetry.registeredHistograms); we'll make it available in
   // other ways.
   h->ClearFlags(Histogram::kUmaTargetedHistogramFlag);
   info.h = h;
--- a/toolkit/components/telemetry/nsITelemetry.idl
+++ b/toolkit/components/telemetry/nsITelemetry.idl
@@ -7,17 +7,17 @@
 #include "nsIFile.idl"
 
 [scriptable,function, uuid(3d3b9075-5549-4244-9c08-b64fefa1dd60)]
 interface nsIFetchTelemetryDataCallback : nsISupports
 {
   void complete();
 };
 
-[scriptable, uuid(df355bbf-437d-4332-80e6-a8a54db959f3)]
+[scriptable, uuid(4c7ba1fc-8253-4bb6-b434-325e51c26109)]
 interface nsITelemetry : nsISupports
 {
   /**
    * Histogram types:
    * HISTOGRAM_EXPONENTIAL - buckets increase exponentially
    * HISTOGRAM_LINEAR - buckets increase linearly
    * HISTOGRAM_BOOLEAN - For storing 0/1 values
    * HISTOGRAM_FLAG - For storing a single value; its count is always == 1.
@@ -133,32 +133,36 @@ interface nsITelemetry : nsISupports
 
   /**
    * Returns an array whose values are the names of histograms defined
    * in Histograms.json.
    */
   void registeredHistograms(out uint32_t count,
                             [retval, array, size_is(count)] out string histograms);
 
-  /** 
+  /**
    * Create and return a histogram.  Parameters:
    *
    * @param name Unique histogram name
    * @param expiration Expiration version
+   * @param type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR or HISTOGRAM_BOOLEAN
    * @param min - Minimal bucket size
    * @param max - Maximum bucket size
    * @param bucket_count - number of buckets in the histogram.
-   * @param type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN or HISTOGRAM_COUNT
    * The returned object has the following functions:
    *   add(int) - Adds an int value to the appropriate bucket
    *   snapshot() - Returns a snapshot of the histogram with the same data fields as in histogramSnapshots()
    *   clear() - Zeros out the histogram's buckets and sum
    */
-  [implicit_jscontext]
-  jsval newHistogram(in ACString name, in ACString expiration, in uint32_t min, in uint32_t max, in uint32_t bucket_count, in unsigned long histogram_type);
+  [implicit_jscontext, optional_argc]
+  jsval newHistogram(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);
 
   /**
    * Create a histogram using the current state of an existing histogram.  The
    * existing histogram must be registered in TelemetryHistograms.h.
    *
    * @param name Unique histogram name
    * @param existing_name Existing histogram name
    * The returned object has the same functions as a histogram returned from newHistogram.
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ -80,17 +80,17 @@ function wrapWithExceptionHandler(f) {
 }
 
 function registerPingHandler(handler) {
   gHttpServer.registerPrefixHandler("/submit/telemetry/",
 				   wrapWithExceptionHandler(handler));
 }
 
 function setupTestData() {
-  Telemetry.newHistogram(IGNORE_HISTOGRAM, "never", 1, 2, 3, Telemetry.HISTOGRAM_BOOLEAN);
+  Telemetry.newHistogram(IGNORE_HISTOGRAM, "never", Telemetry.HISTOGRAM_BOOLEAN);
   Telemetry.histogramFrom(IGNORE_CLONED_HISTOGRAM, IGNORE_HISTOGRAM_TO_CLONE);
   Services.startup.interrupted = true;
   Telemetry.registerAddonHistogram(ADDON_NAME, ADDON_HISTOGRAM, 1, 5, 6,
                                    Telemetry.HISTOGRAM_LINEAR);
   let h1 = Telemetry.getAddonHistogram(ADDON_NAME, ADDON_HISTOGRAM);
   h1.add(1);
   let h2 = Telemetry.getHistogramById("TELEMETRY_TEST_COUNT");
   h2.add();
@@ -464,17 +464,17 @@ add_task(function* test_overwritePing() 
   yield TelemetryFile.savePing(ping, true);
   yield TelemetryFile.savePing(ping, false);
   yield TelemetryFile.cleanupPingFile(ping);
 });
 
 // Ensures that expired histograms are not part of the payload.
 add_task(function* test_expiredHistogram() {
   let histogram_id = "FOOBAR";
-  let dummy = Telemetry.newHistogram(histogram_id, "30", 1, 2, 3, Telemetry.HISTOGRAM_EXPONENTIAL);
+  let dummy = Telemetry.newHistogram(histogram_id, "30", Telemetry.HISTOGRAM_EXPONENTIAL, 1, 2, 3);
 
   dummy.add(1);
 
   do_check_eq(TelemetryPing.getPayload()["histograms"][histogram_id], undefined);
   do_check_eq(TelemetryPing.getPayload()["histograms"]["TELEMETRY_TEST_EXPIRED"], undefined);
 });
 
 // Checks that an invalid histogram file is deleted if TelemetryFile fails to parse it.
--- a/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
+++ b/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ -8,32 +8,32 @@ const INT_MAX = 0x7FFFFFFF;
 
 const Telemetry = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);
 Cu.import("resource://gre/modules/Services.jsm", this);
 
 function test_expired_histogram() {
   var histogram_id = "FOOBAR";
   var test_expired_id = "TELEMETRY_TEST_EXPIRED";
   var clone_id = "ExpiredClone";
-  var dummy = Telemetry.newHistogram(histogram_id, "28.0a1", 1, 2, 3, Telemetry.HISTOGRAM_EXPONENTIAL);
+  var dummy = Telemetry.newHistogram(histogram_id, "28.0a1", Telemetry.HISTOGRAM_EXPONENTIAL, 1, 2, 3);
   var dummy_clone = Telemetry.histogramFrom(clone_id, test_expired_id);
   var rh = Telemetry.registeredHistograms([]);
 
   dummy.add(1);
   dummy_clone.add(1);
 
   do_check_eq(Telemetry.histogramSnapshots["__expired__"], undefined);
   do_check_eq(Telemetry.histogramSnapshots[histogram_id], undefined);
   do_check_eq(Telemetry.histogramSnapshots[test_expired_id], undefined);
   do_check_eq(Telemetry.histogramSnapshots[clone_id], undefined);
   do_check_eq(rh[test_expired_id], undefined);
 }
 
 function test_histogram(histogram_type, name, min, max, bucket_count) {
-  var h = Telemetry.newHistogram(name, "never", min, max, bucket_count, histogram_type);
+  var h = Telemetry.newHistogram(name, "never", histogram_type, min, max, bucket_count);
   var r = h.snapshot().ranges;
   var sum = 0;
   var log_sum = 0;
   var log_sum_squares = 0;
   for(var i=0;i<r.length;i++) {
     var v = r[i];
     sum += v;
     if (histogram_type == Telemetry.HISTOGRAM_EXPONENTIAL) {
@@ -121,17 +121,17 @@ function expect_success(f) {
   } catch (e) {
     succeeded = false;
   }
   do_check_true(succeeded);
 }
 
 function test_boolean_histogram()
 {
-  var h = Telemetry.newHistogram("test::boolean histogram", "never", 99,1,4, Telemetry.HISTOGRAM_BOOLEAN);
+  var h = Telemetry.newHistogram("test::boolean histogram", "never", Telemetry.HISTOGRAM_BOOLEAN);
   var r = h.snapshot().ranges;
   // boolean histograms ignore numeric parameters
   do_check_eq(uneval(r), uneval([0, 1, 2]))
   var sum = 0
   for(var i=0;i<r.length;i++) {
     var v = r[i];
     sum += v;
     h.add(v);
@@ -143,17 +143,17 @@ function test_boolean_histogram()
   // last bucket should always be 0 since .add parameters are normalized to either 0 or 1
   do_check_eq(s.counts[2], 0);
   do_check_eq(s.sum, 3);
   do_check_eq(s.counts[0], 2);
 }
 
 function test_flag_histogram()
 {
-  var h = Telemetry.newHistogram("test::flag histogram", "never", 130, 4, 5, Telemetry.HISTOGRAM_FLAG);
+  var h = Telemetry.newHistogram("test::flag histogram", "never", Telemetry.HISTOGRAM_FLAG);
   var r = h.snapshot().ranges;
   // Flag histograms ignore numeric parameters.
   do_check_eq(uneval(r), uneval([0, 1, 2]))
   // Should already have a 0 counted.
   var c = h.snapshot().counts;
   var s = h.snapshot().sum;
   do_check_eq(uneval(c), uneval([1, 0, 0]));
   do_check_eq(s, 1);
@@ -169,17 +169,17 @@ function test_flag_histogram()
   var s3 = h.snapshot().sum;
   do_check_eq(uneval(c3), uneval([0, 1, 0]));
   do_check_eq(s3, 1);
   do_check_eq(h.snapshot().histogram_type, Telemetry.FLAG_HISTOGRAM);
 }
 
 function test_count_histogram()
 {
-  let h = Telemetry.newHistogram("test::count histogram", "never", 1, 2, 3, Telemetry.HISTOGRAM_COUNT);
+  let h = Telemetry.newHistogram("test::count histogram", "never", Telemetry.HISTOGRAM_COUNT, 1, 2, 3);
   let s = h.snapshot();
   do_check_eq(uneval(s.ranges), uneval([0, 1, 2]));
   do_check_eq(uneval(s.counts), uneval([0, 0, 0]));
   do_check_eq(s.sum, 0);
   h.add();
   s = h.snapshot();
   do_check_eq(uneval(s.counts), uneval([1, 0, 0]));
   do_check_eq(s.sum, 1);
@@ -347,17 +347,17 @@ function test_addons() {
   snapshots = Telemetry.addonHistogramSnapshots;
   do_check_false(addon_id in snapshots);
   // Make sure other addons are unaffected.
   do_check_true(extra_addon in snapshots);
 }
 
 // Check that telemetry doesn't record in private mode
 function test_privateMode() {
-  var h = Telemetry.newHistogram("test::private_mode_boolean", "never", 1,2,3, Telemetry.HISTOGRAM_BOOLEAN);
+  var h = Telemetry.newHistogram("test::private_mode_boolean", "never", Telemetry.HISTOGRAM_BOOLEAN);
   var orig = h.snapshot();
   Telemetry.canRecord = false;
   h.add(1);
   do_check_eq(uneval(orig), uneval(h.snapshot()));
   Telemetry.canRecord = true;
   h.add(1);
   do_check_neq(uneval(orig), uneval(h.snapshot()));
 }
@@ -386,18 +386,18 @@ function generateUUID() {
 function run_test()
 {
   let kinds = [Telemetry.HISTOGRAM_EXPONENTIAL, Telemetry.HISTOGRAM_LINEAR]
   for each (let histogram_type in kinds) {
     let [min, max, bucket_count] = [1, INT_MAX - 1, 10]
     test_histogram(histogram_type, "test::"+histogram_type, min, max, bucket_count);
 
     const nh = Telemetry.newHistogram;
-    expect_fail(function () nh("test::min", "never", 0, max, bucket_count, histogram_type));
-    expect_fail(function () nh("test::bucket_count", "never", min, max, 1, histogram_type));
+    expect_fail(function () nh("test::min", "never", histogram_type, 0, max, bucket_count));
+    expect_fail(function () nh("test::bucket_count", "never", histogram_type, min, max, 1));
   }
 
   // Instantiate the storage for this histogram and make sure it doesn't
   // get reflected into JS, as it has no interesting data in it.
   let h = Telemetry.getHistogramById("NEWTAB_PAGE_PINNED_SITES_COUNT");
   do_check_false("NEWTAB_PAGE_PINNED_SITES_COUNT" in Telemetry.histogramSnapshots);
 
   test_boolean_histogram();