Bug 809930 - Make metrics provider collection API more robust; r=rnewman
authorGregory Szorc <gps@mozilla.com>
Thu, 08 Nov 2012 15:32:49 -0800
changeset 113826 5901554be0faae6973827bd41752badb03bc1950
parent 113825 6b0dc316e003dbf93b7eaad59fd5548b862e6f65
child 113827 6b2eb103766a470894c40f432acd567ee9f0a884
push id23890
push userryanvm@gmail.com
push dateWed, 21 Nov 2012 02:43:32 +0000
treeherdermozilla-central@4f19e7fd8bea [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs809930
milestone20.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 809930 - Make metrics provider collection API more robust; r=rnewman
services/metrics/collector.jsm
services/metrics/dataprovider.jsm
services/metrics/modules-testing/mocks.jsm
services/metrics/tests/xpcshell/test_metrics_collection_result.js
services/metrics/tests/xpcshell/test_metrics_collector.js
services/metrics/tests/xpcshell/test_metrics_provider.js
--- a/services/metrics/collector.jsm
+++ b/services/metrics/collector.jsm
@@ -5,30 +5,32 @@
 "use strict";
 
 this.EXPORTED_SYMBOLS = ["MetricsCollector"];
 
 const {utils: Cu} = Components;
 
 Cu.import("resource://gre/modules/commonjs/promise/core.js");
 Cu.import("resource://services-common/log4moz.js");
+Cu.import("resource://services-common/utils.js");
 Cu.import("resource://gre/modules/services/metrics/dataprovider.jsm");
 
 
 /**
  * Handles and coordinates the collection of metrics data from providers.
  *
  * This provides an interface for managing `MetricsProvider` instances. It
  * provides APIs for bulk collection of data.
  */
 this.MetricsCollector = function MetricsCollector() {
   this._log = Log4Moz.repository.getLogger("Metrics.MetricsCollector");
 
   this._providers = [];
   this.collectionResults = new Map();
+  this.providerErrors = new Map();
 }
 
 MetricsCollector.prototype = {
   /**
    * Registers a `MetricsProvider` with this collector.
    *
    * Once a `MetricsProvider` is registered, data will be collected from it
    * whenever we collect data.
@@ -46,40 +48,64 @@ MetricsCollector.prototype = {
         return;
       }
     }
 
     this._providers.push({
       provider: provider,
       constantsCollected: false,
     });
+
+    this.providerErrors.set(provider.name, []);
   },
 
   /**
    * Collects all constant measurements from all providers.
    *
    * Returns a Promise that will be fulfilled once all data providers have
    * provided their constant data. A side-effect of this promise fulfillment
    * is that the collector is populated with the obtained collection results.
    * The resolved value to the promise is this `MetricsCollector` instance.
    */
   collectConstantMeasurements: function collectConstantMeasurements() {
     let promises = [];
 
     for (let provider of this._providers) {
+      let name = provider.provider.name;
+
       if (provider.constantsCollected) {
         this._log.trace("Provider has already provided constant data: " +
-                        provider.name);
+                        name);
         continue;
       }
 
-      let result = provider.provider.collectConstantMeasurements();
+      let result;
+      try {
+        result = provider.provider.collectConstantMeasurements();
+      } catch (ex) {
+        this._log.warn("Exception when calling " + name +
+                       ".collectConstantMeasurements: " +
+                       CommonUtils.exceptionStr(ex));
+        this.providerErrors.get(name).push(ex);
+        continue;
+      }
+
       if (!result) {
-        this._log.trace("Provider does not provide constant data: " +
-                        provider.name);
+        this._log.trace("Provider does not provide constant data: " + name);
+        continue;
+      }
+
+      try {
+        this._log.debug("Populating constant measurements: " + name);
+        result.populate(result);
+      } catch (ex) {
+        this._log.warn("Exception when calling " + name + ".populate(): " +
+                       CommonUtils.exceptionStr(ex));
+        result.addError(ex);
+        promises.push(Promise.resolve(result));
         continue;
       }
 
       // Chain an invisible promise that updates state.
       let promise = result.onFinished(function onFinished(result) {
         provider.constantsCollected = true;
 
         return Promise.resolve(result);
--- a/services/metrics/dataprovider.jsm
+++ b/services/metrics/dataprovider.jsm
@@ -190,19 +190,28 @@ Object.freeze(MetricsMeasurement.prototy
  *
  * This type consists of various collect* functions. These functions are called
  * by the metrics collector at different points during the application's
  * lifetime. These functions return a `MetricsCollectionResult` instance.
  * This type behaves a lot like a promise. It has a `onFinished()` that can chain
  * deferred events until after the result is populated.
  *
  * Implementations of collect* functions should call `createResult()` to create
- * a new `MetricsCollectionResult` instance. When called, they should
- * initiate population of this instance. Once population has finished (perhaps
- * asynchronously), they should call `finish()` on the instance.
+ * a new `MetricsCollectionResult` instance. They should then register
+ * expected measurements with this instance, define a `populate` function on
+ * it, then return the instance.
+ *
+ * It is important for the collect* functions to just create the empty
+ * `MetricsCollectionResult` and nothing more. This is to enable the callee
+ * to handle errors gracefully. If the collect* function were to raise, the
+ * callee may not receive a `MetricsCollectionResult` instance and it would not
+ * know what data is missing.
+ *
+ * See the documentation for `MetricsCollectionResult` for details on how
+ * to perform population.
  *
  * Receivers of created `MetricsCollectionResult` instances should wait
  * until population has finished. They can do this by chaining on to the
  * promise inside that instance by calling `onFinished()`.
  *
  * The collect* functions can return null to signify that they will never
  * provide any data. This is the default implementation. An implemented
  * collect* function should *never* return null. Instead, it should return
@@ -259,19 +268,22 @@ Object.freeze(MetricsProvider.prototype)
  * This type is essentially a container for `MetricsMeasurement` instances that
  * provides some smarts useful for capturing state.
  *
  * The first things consumers of new instances should do is define the set of
  * expected measurements this result will contain via `expectMeasurement`. If
  * population of this instance is aborted or times out, downstream consumers
  * will know there is missing data.
  *
- * Next, they should add empty `MetricsMeasurement` instances to it via
- * `addMeasurement`. Finally, they should populate these measurements with
- * `setValue`.
+ * Next, they should define the `populate` property to a function that
+ * populates the instance.
+ *
+ * The `populate` function implementation should add empty `MetricsMeasurement`
+ * instances to the result via `addMeasurement`. Then, it should populate these
+ * measurements via `setValue`.
  *
  * It is preferred to populate via this type instead of directly on
  * `MetricsMeasurement` instances so errors with data population can be
  * captured and reported.
  *
  * Once population has finished, `finish()` must be called.
  *
  * @param name
@@ -285,16 +297,21 @@ this.MetricsCollectionResult = function 
   this._log = Log4Moz.repository.getLogger("Services.Metrics.MetricsCollectionResult");
 
   this.name = name;
 
   this.measurements = new Map();
   this.expectedMeasurements = new Set();
   this.errors = [];
 
+  this.populate = function populate() {
+    throw new Error("populate() must be defined on MetricsCollectionResult " +
+                    "instance.");
+  };
+
   this._deferred = Promise.defer();
 }
 
 MetricsCollectionResult.prototype = {
   /**
    * The Set of `MetricsMeasurement` names currently missing from this result.
    */
   get missingMeasurements() {
--- a/services/metrics/modules-testing/mocks.jsm
+++ b/services/metrics/modules-testing/mocks.jsm
@@ -32,27 +32,43 @@ DummyMeasurement.prototype = {
 };
 
 
 this.DummyProvider = function DummyProvider(name="DummyProvider") {
   MetricsProvider.call(this, name);
 
   this.constantMeasurementName = "DummyMeasurement";
   this.collectConstantCount = 0;
+  this.throwDuringCollectConstantMeasurements = null;
+  this.throwDuringConstantPopulate = null;
 }
 DummyProvider.prototype = {
   __proto__: MetricsProvider.prototype,
 
   collectConstantMeasurements: function collectConstantMeasurements() {
     this.collectConstantCount++;
 
     let result = this.createResult();
     result.expectMeasurement(this.constantMeasurementName);
+
+    result.populate = this._populateConstantResult.bind(this);
+
+    if (this.throwDuringCollectConstantMeasurements) {
+      throw new Error(this.throwDuringCollectConstantMeasurements);
+    }
+
+    return result;
+  },
+
+  _populateConstantResult: function _populateConstantResult(result) {
+    if (this.throwDuringConstantPopulate) {
+      throw new Error(this.throwDuringConstantPopulate);
+    }
+
+    this._log.debug("Populating constant measurement in DummyProvider.");
     result.addMeasurement(new DummyMeasurement(this.constantMeasurementName));
 
     result.setValue(this.constantMeasurementName, "string", "foo");
     result.setValue(this.constantMeasurementName, "uint32", 24);
 
     result.finish();
-
-    return result;
   },
 };
--- a/services/metrics/tests/xpcshell/test_metrics_collection_result.js
+++ b/services/metrics/tests/xpcshell/test_metrics_collection_result.js
@@ -15,21 +15,32 @@ function run_test() {
 
 add_test(function test_constructor() {
   let result = new MetricsCollectionResult("foo");
   do_check_eq(result.name, "foo");
 
   let failed = false;
   try {
     new MetricsCollectionResult();
-  } catch(ex) {
+  } catch (ex) {
     do_check_true(ex.message.startsWith("Must provide name argument to Metrics"));
     failed = true;
   } finally {
     do_check_true(failed);
+    failed = false;
+  }
+
+  try {
+    result.populate();
+  } catch (ex) {
+    do_check_true(ex.message.startsWith("populate() must be defined"));
+    failed = true;
+  } finally {
+    do_check_true(failed);
+    failed = false;
   }
 
   run_next_test();
 });
 
 add_test(function test_expected_measurements() {
   let result = new MetricsCollectionResult("foo");
   do_check_eq(result.missingMeasurements.size0);
@@ -224,8 +235,9 @@ add_test(function test_finish() {
   result.onFinished(function onFinished(result2) {
     do_check_eq(result, result2);
 
     run_next_test();
   });
 
   result.finish();
 });
+
--- a/services/metrics/tests/xpcshell/test_metrics_collector.js
+++ b/services/metrics/tests/xpcshell/test_metrics_collector.js
@@ -23,16 +23,17 @@ add_test(function test_constructor() {
 add_test(function test_register_provider() {
   let collector = new MetricsCollector();
   let dummy = new DummyProvider();
 
   collector.registerProvider(dummy);
   do_check_eq(collector._providers.length, 1);
   collector.registerProvider(dummy);
   do_check_eq(collector._providers.length, 1);
+  do_check_eq(collector.providerErrors.size, 1);
 
   let failed = false;
   try {
     collector.registerProvider({});
   } catch (ex) {
     do_check_true(ex.message.startsWith("argument must be a MetricsProvider"));
     failed = true;
   } finally {
@@ -54,16 +55,53 @@ add_test(function test_collect_constant_
     do_check_eq(provider.collectConstantCount, 1);
     do_check_eq(collector.collectionResults.size, 1);
     do_check_true(collector.collectionResults.has("DummyProvider"));
 
     let result = collector.collectionResults.get("DummyProvider");
     do_check_true(result instanceof MetricsCollectionResult);
 
     do_check_true(collector._providers[0].constantsCollected);
+    do_check_eq(collector.providerErrors.get("DummyProvider").length, 0);
+
+    run_next_test();
+  });
+});
+
+add_test(function test_collect_constant_throws() {
+  let collector = new MetricsCollector();
+  let provider = new DummyProvider();
+  provider.throwDuringCollectConstantMeasurements = "Fake error during collect";
+  collector.registerProvider(provider);
+
+  collector.collectConstantMeasurements().then(function onResult() {
+    do_check_eq(collector.providerErrors.get("DummyProvider").length, 1);
+    do_check_eq(collector.providerErrors.get("DummyProvider")[0].message,
+                provider.throwDuringCollectConstantMeasurements);
+
+    run_next_test();
+  });
+});
+
+add_test(function test_collect_constant_populate_throws() {
+  let collector = new MetricsCollector();
+  let provider = new DummyProvider();
+  provider.throwDuringConstantPopulate = "Fake error during constant populate";
+  collector.registerProvider(provider);
+
+  collector.collectConstantMeasurements().then(function onResult() {
+    do_check_eq(collector.collectionResults.size, 1);
+    do_check_true(collector.collectionResults.has("DummyProvider"));
+
+    let result = collector.collectionResults.get("DummyProvider");
+    do_check_eq(result.errors.length, 1);
+    do_check_eq(result.errors[0].message, provider.throwDuringConstantPopulate);
+
+    do_check_false(collector._providers[0].constantsCollected);
+    do_check_eq(collector.providerErrors.get("DummyProvider").length, 0);
 
     run_next_test();
   });
 });
 
 add_test(function test_collect_constant_onetime() {
   let collector = new MetricsCollector();
   let provider = new DummyProvider();
--- a/services/metrics/tests/xpcshell/test_metrics_provider.js
+++ b/services/metrics/tests/xpcshell/test_metrics_provider.js
@@ -40,21 +40,22 @@ add_test(function test_default_collector
 
     let result = provider[property]();
     do_check_null(result);
   }
 
   run_next_test();
 });
 
-add_test(function test_collect_synchronous() {
+add_test(function test_collect_constant_synchronous() {
   let provider = new DummyProvider();
 
   let result = provider.collectConstantMeasurements();
   do_check_true(result instanceof MetricsCollectionResult);
+  result.populate(result);
 
   result.onFinished(function onResult(res2) {
     do_check_eq(result, res2);
 
     let m = result.measurements.get("DummyMeasurement");
     do_check_eq(m.getValue("uint32"), 24);
 
     run_next_test();