From 48d4b18c33d015aa8b0a2a7cb99563ecd734c1ac Mon Sep 17 00:00:00 2001 From: Gregory Szorc Date: Thu, 8 Nov 2012 15:32:49 -0800 Subject: [PATCH] Bug 809930 - Make metrics provider collection API more robust; r=rnewman --- services/metrics/collector.jsm | 34 +++++++++++++++-- services/metrics/dataprovider.jsm | 29 +++++++++++--- services/metrics/modules-testing/mocks.jsm | 20 +++++++++- .../test_metrics_collection_result.js | 14 ++++++- .../tests/xpcshell/test_metrics_collector.js | 38 +++++++++++++++++++ .../tests/xpcshell/test_metrics_provider.js | 3 +- 6 files changed, 124 insertions(+), 14 deletions(-) diff --git a/services/metrics/collector.jsm b/services/metrics/collector.jsm index f4ff9f4b28e4..aedeb15f2ab3 100644 --- a/services/metrics/collector.jsm +++ b/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; } diff --git a/services/metrics/dataprovider.jsm b/services/metrics/dataprovider.jsm index c82af71167c1..175db36c82e4 100644 --- a/services/metrics/dataprovider.jsm +++ b/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(); } diff --git a/services/metrics/modules-testing/mocks.jsm b/services/metrics/modules-testing/mocks.jsm index 05623a3297f2..9f65d4ebd0e4 100644 --- a/services/metrics/modules-testing/mocks.jsm +++ b/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; }, }; diff --git a/services/metrics/tests/xpcshell/test_metrics_collection_result.js b/services/metrics/tests/xpcshell/test_metrics_collection_result.js index 57bb9a4dd885..1331e1614660 100644 --- a/services/metrics/tests/xpcshell/test_metrics_collection_result.js +++ b/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(); }); + diff --git a/services/metrics/tests/xpcshell/test_metrics_collector.js b/services/metrics/tests/xpcshell/test_metrics_collector.js index cb63de34fd6b..9d615d9f5d1d 100644 --- a/services/metrics/tests/xpcshell/test_metrics_collector.js +++ b/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(); }); diff --git a/services/metrics/tests/xpcshell/test_metrics_provider.js b/services/metrics/tests/xpcshell/test_metrics_provider.js index b7a2f5d1ace0..2a969ae280f0 100644 --- a/services/metrics/tests/xpcshell/test_metrics_provider.js +++ b/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);