From 06e743e0505787b38f2429891da51d1325676691 Mon Sep 17 00:00:00 2001 From: Dominic Kramer Date: Thu, 15 Jun 2017 10:31:19 -0700 Subject: [PATCH] errors-reporting: Unhandled rejections are reported (#2360) --- packages/error-reporting/README.md | 13 +- packages/error-reporting/src/configuration.js | 24 +++ packages/error-reporting/src/index.js | 11 ++ .../system-test/testAuthClient.js | 144 ++++++++++++++---- .../test/unit/configuration.js | 23 +++ 5 files changed, 181 insertions(+), 34 deletions(-) diff --git a/packages/error-reporting/README.md b/packages/error-reporting/README.md index a2dc5b51ecf..a7c4bb137c7 100644 --- a/packages/error-reporting/README.md +++ b/packages/error-reporting/README.md @@ -64,9 +64,17 @@ errors.report(new Error('Something broke!')); Open Stackdriver Error Reporting at https://console.cloud.google.com/errors to view the reported errors. +## Unhandled Rejections + +Unhandled Rejections are not reported by default. The reporting of unhandled rejections can be enabled using the `reportUnhandledRejections` configuration option. See the [Configuration](#configuration) section for more details. + +If unhandled rejections are set to be reported, then, when an unhandled rejection occurs, a message is printed to standard out indicated that an unhandled rejection had occurred and is being reported, and the value causing the rejection is reported to the error-reporting console. + ## Catching and Reporting Application-wide Uncaught Errors -*It is recommended to catch `uncaughtExceptions` for production-deployed applications.* +Uncaught exceptions are not reported by default. *It is recommended to process `uncaughtException`s for production-deployed applications.* + +Note that uncaught exceptions are not reported by default because to do so would require adding a listener to the `uncaughtException` event. Adding such a listener without knowledge of other `uncaughtException` listeners can cause interference between the event handlers or prevent the process from terminating cleanly. As such, it is necessary for `uncaughtException`s to be reported manually. ```js var errors = require('@google-cloud/error-reporting')(); @@ -156,6 +164,9 @@ var errors = require('@google-cloud/error-reporting')({ // should be reported // defaults to 2 (warnings) logLevel: 2, + // determines whether or not unhandled rejections are reported to the + // error-reporting console + reportUnhandledRejections: true, serviceContext: { service: 'my-service', version: 'my-service-version' diff --git a/packages/error-reporting/src/configuration.js b/packages/error-reporting/src/configuration.js index 03d77768964..33be4fc5f99 100644 --- a/packages/error-reporting/src/configuration.js +++ b/packages/error-reporting/src/configuration.js @@ -134,6 +134,14 @@ var Configuration = function(givenConfig, logger) { * @type {Object} */ this._serviceContext = {service: 'nodejs', version: ''}; + /** + * The _reportUnhandledRejections property is meant to specify whether or + * not unhandled rejections should be reported to the error-reporting console. + * @memberof Configuration + * @private + * @type {Boolean} + */ + this._reportUnhandledRejections = false; /** * The _givenConfiguration property holds a ConfigurationOptions object * which, if valid, will be merged against by the values taken from the meta- @@ -257,6 +265,12 @@ Configuration.prototype._gatherLocalConfiguration = function() { } else if (has(this._givenConfiguration, 'credentials')) { throw new Error('config.credentials must be a valid credentials object'); } + if (isBoolean(this._givenConfiguration.reportUnhandledRejections)) { + this._reportUnhandledRejections = + this._givenConfiguration.reportUnhandledRejections; + } else if (has(this._givenConfiguration, 'reportUnhandledRejections')) { + throw new Error('config.reportUnhandledRejections must be a boolean'); + } }; /** * The _checkLocalProjectId function is responsible for determing whether the @@ -354,4 +368,14 @@ Configuration.prototype.getCredentials = function() { Configuration.prototype.getServiceContext = function() { return this._serviceContext; }; +/** + * Returns the _reportUnhandledRejections property on the instance. + * @memberof Configuration + * @public + * @function getReportUnhandledRejections + * @returns {Boolean} - returns the _reportUnhandledRejections property + */ +Configuration.prototype.getReportUnhandledRejections = function() { + return this._reportUnhandledRejections; +}; module.exports = Configuration; diff --git a/packages/error-reporting/src/index.js b/packages/error-reporting/src/index.js index b2038350e05..3fec7a6e052 100644 --- a/packages/error-reporting/src/index.js +++ b/packages/error-reporting/src/index.js @@ -154,6 +154,17 @@ function Errors(initConfiguration) { * app.use(errors.koa); */ this.koa = koa(this._client, this._config); + + if (this._config.getReportUnhandledRejections()) { + var that = this; + process.on('unhandledRejection', function(reason) { + console.log('UnhandledPromiseRejectionWarning: ' + + 'Unhandled promise rejection: ' + reason + + '. This rejection has been reported to the ' + + 'Google Cloud Platform error-reporting console.'); + that.report(reason); + }); + } } module.exports = Errors; diff --git a/packages/error-reporting/system-test/testAuthClient.js b/packages/error-reporting/system-test/testAuthClient.js index 8678a2b0d3f..a97e2198842 100644 --- a/packages/error-reporting/system-test/testAuthClient.js +++ b/packages/error-reporting/system-test/testAuthClient.js @@ -31,9 +31,10 @@ var forEach = require('lodash.foreach'); var assign = require('lodash.assign'); var pick = require('lodash.pick'); var omitBy = require('lodash.omitby'); +var util = require('util'); const ERR_TOKEN = '_@google_STACKDRIVER_INTEGRATION_TEST_ERROR__'; -const TIMEOUT = 20000; +const TIMEOUT = 30000; const envKeys = ['GOOGLE_APPLICATION_CREDENTIALS', 'GCLOUD_PROJECT', 'NODE_ENV']; @@ -371,54 +372,91 @@ describe('error-reporting', function() { var errors; var transport; + var oldLogger; + var logOutput = ''; before(function() { - errors = require('../src/index.js')({ + // This test assumes that only the error-reporting library will be + // adding listeners to the 'unhandledRejection' event. Thus we need to + // make sure that no listeners for that event exist. If this check + // fails, then the reinitialize() method below will need to updated to + // more carefully reinitialize the error-reporting library without + // interfering with existing listeners of the 'unhandledRejection' event. + assert.strictEqual(process.listenerCount('unhandledRejection'), 0); + oldLogger = console.log; + console.log = function() { + var text = util.format.apply(null, arguments); + oldLogger(text); + logOutput += text; + }; + reinitialize(); + }); + + function reinitialize(extraConfig) { + process.removeAllListeners('unhandledRejection'); + var config = Object.assign({ ignoreEnvironmentCheck: true, serviceContext: { service: SERVICE_NAME, version: SERVICE_VERSION } - }); + }, extraConfig || {}); + errors = require('../src/index.js')(config); transport = new ErrorsApiTransport(errors._config, errors._logger); - }); + } after(function(done) { - transport.deleteAllEvents(function(err) { - assert.ifError(err); - done(); - }); + console.log = oldLogger; + if (transport) { + transport.deleteAllEvents(function(err) { + assert.ifError(err); + done(); + }); + } + }); + + afterEach(function() { + logOutput = ''; }); + function verifyAllGroups(messageTest, timeout, cb) { + setTimeout(function() { + transport.getAllGroups(function(err, groups) { + assert.ifError(err); + assert.ok(groups); + + var matchedErrors = groups.filter(function(errItem) { + return errItem && errItem.representative && + messageTest(errItem.representative.message); + }); + + cb(matchedErrors); + }); + }, timeout); + } + + function verifyServerResponse(messageTest, timeout, cb) { + verifyAllGroups(messageTest, timeout, function(matchedErrors) { + // The error should have been reported exactly once + assert.strictEqual(matchedErrors.length, 1); + var errItem = matchedErrors[0]; + assert.ok(errItem); + assert.equal(errItem.count, 1); + var rep = errItem.representative; + assert.ok(rep); + var context = rep.serviceContext; + assert.ok(context); + assert.strictEqual(context.service, SERVICE_NAME); + assert.strictEqual(context.version, SERVICE_VERSION); + cb(); + }); + } + function verifyReporting(errOb, messageTest, timeout, cb) { errors.report(errOb, function(err, response, body) { assert.ifError(err); assert(isObject(response)); assert.deepEqual(body, {}); - - setTimeout(function() { - transport.getAllGroups(function(err, groups) { - assert.ifError(err); - assert.ok(groups); - - var matchedErrors = groups.filter(function(errItem) { - return errItem && errItem.representative && - messageTest(errItem.representative.message); - }); - - // The error should have been reported exactly once - assert.strictEqual(matchedErrors.length, 1); - var errItem = matchedErrors[0]; - assert.ok(errItem); - assert.equal(errItem.count, 1); - var rep = errItem.representative; - assert.ok(rep); - var context = rep.serviceContext; - assert.ok(context); - assert.strictEqual(context.service, SERVICE_NAME); - assert.strictEqual(context.version, SERVICE_VERSION); - cb(); - }); - }, timeout); + verifyServerResponse(messageTest, timeout, cb); }); } @@ -465,4 +503,44 @@ describe('error-reporting', function() { }, TIMEOUT, done); })(); }); + + it('Should report unhandledRejections if enabled', function(done) { + this.timeout(TIMEOUT * 2); + reinitialize({ reportUnhandledRejections: true }); + var rejectValue = buildName('promise-rejection'); + Promise.reject(rejectValue); + setImmediate(function() { + var expected = 'UnhandledPromiseRejectionWarning: Unhandled ' + + 'promise rejection: ' + rejectValue + + '. This rejection has been reported to the ' + + 'Google Cloud Platform error-reporting console.'; + assert.notStrictEqual(logOutput.indexOf(expected), -1); + verifyServerResponse(function(message) { + return message.startsWith(rejectValue); + }, TIMEOUT, done); + }); + }); + + it('Should not report unhandledRejections if disabled', function(done) { + this.timeout(TIMEOUT * 2); + reinitialize({ reportUnhandledRejections: false }); + var rejectValue = buildName('promise-rejection'); + Promise.reject(rejectValue); + setImmediate(function() { + var notExpected = 'UnhandledPromiseRejectionWarning: Unhandled ' + + 'promise rejection: ' + rejectValue + + '. This rejection has been reported to the error-reporting console.'; + assert.strictEqual(logOutput.indexOf(notExpected), -1); + // Get all groups that that start with the rejection value and hence all + // of the groups corresponding to the above rejection (Since the + // buildName() creates a string unique enough to single out only the + // above rejection.) and verify that there are no such groups reported. + verifyAllGroups(function(message) { + return message.startsWith(rejectValue); + }, TIMEOUT, function(matchedErrors) { + assert.strictEqual(matchedErrors.length, 0); + done(); + }); + }); + }); }); \ No newline at end of file diff --git a/packages/error-reporting/test/unit/configuration.js b/packages/error-reporting/test/unit/configuration.js index 5171935ebeb..b1d13f0d306 100644 --- a/packages/error-reporting/test/unit/configuration.js +++ b/packages/error-reporting/test/unit/configuration.js @@ -92,6 +92,9 @@ describe('Configuration class', function() { assert.deepEqual(c.getServiceContext(), {service: 'node', version: undefined}); }); + it('Should specify to not report unhandledRejections', function() { + assert.strictEqual(c.getReportUnhandledRejections(), false); + }); }); describe('with ignoreEnvironmentCheck', function() { var conf = merge({}, stubConfig, {ignoreEnvironmentCheck: true}); @@ -134,6 +137,13 @@ describe('Configuration class', function() { new Configuration({serviceContext: {version: true}}, logger); }); }); + it('Should throw if invalid for reportUnhandledRejections', + function() { + assert.throws(function() { + new Configuration({ reportUnhandledRejections: 'INVALID' }, + logger); + }); + }); it('Should not throw given an empty object for serviceContext', function() { assert.doesNotThrow(function() { @@ -278,6 +288,19 @@ describe('Configuration class', function() { assert.strictEqual(c.getKey(), key); }); }); + describe('reportUnhandledRejections', function() { + var c; + var reportRejections = false; + before(function() { + c = new Configuration({ + reportUnhandledRejections: reportRejections + }); + }); + it('Should assign', function() { + assert.strictEqual(c.getReportUnhandledRejections(), + reportRejections); + }); + }); }); }); });