Bug 809930 - Make metrics provider collection API more robust; r=rnewman

services/metrics/collector.jsm

@@ -10,6 +10,7 @@ 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");
 
 
@@ -24,6 +25,7 @@ this.MetricsCollector = function MetricsCollector() {
 
   this._providers = [];
   this.collectionResults = new Map();
+  this.providerErrors = new Map();
 }
 
 MetricsCollector.prototype = {
@@ -51,6 +53,8 @@ MetricsCollector.prototype = {
       provider: provider,
       constantsCollected: false,
     });
+
+    this.providerErrors.set(provider.name, []);
   },
 
   /**
@@ -65,16 +69,38 @@ MetricsCollector.prototype = {
     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;
+      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;
       }
 
-      let result = provider.provider.collectConstantMeasurements();
       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;
       }
 

services/metrics/dataprovider.jsm

@@ -195,9 +195,18 @@ Object.freeze(MetricsMeasurement.prototype);
  * 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
@@ -264,9 +273,12 @@ Object.freeze(MetricsProvider.prototype);
  * 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
@@ -290,6 +302,11 @@ this.MetricsCollectionResult = function MetricsCollectionResult(name) {
   this.expectedMeasurements = new Set();
   this.errors = [];
 
+  this.populate = function populate() {
+    throw new Error("populate() must be defined on MetricsCollectionResult " +
+                    "instance.");
+  };
+
   this._deferred = Promise.defer();
 }
 

services/metrics/modules-testing/mocks.jsm

@@ -37,6 +37,8 @@ this.DummyProvider = function DummyProvider(name="DummyProvider") {
 
   this.constantMeasurementName = "DummyMeasurement";
   this.collectConstantCount = 0;
+  this.throwDuringCollectConstantMeasurements = null;
+  this.throwDuringConstantPopulate = null;
 }
 DummyProvider.prototype = {
   __proto__: MetricsProvider.prototype,
@@ -46,13 +48,27 @@ DummyProvider.prototype = {
 
     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;
   },
 };

services/metrics/tests/xpcshell/test_metrics_collection_result.js

@@ -20,11 +20,22 @@ add_test(function test_constructor() {
   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();
@@ -229,3 +240,4 @@ add_test(function test_finish() {
 
   result.finish();
 });
+

services/metrics/tests/xpcshell/test_metrics_collector.js

@@ -28,6 +28,7 @@ add_test(function test_register_provider() {
   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 {
@@ -59,6 +60,43 @@ add_test(function test_collect_constant_measurements() {
     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();
   });

services/metrics/tests/xpcshell/test_metrics_provider.js

@@ -45,11 +45,12 @@ add_test(function test_default_collectors() {
   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);