From 254328ef8453efe97d030c8e437cdce69b617395 Mon Sep 17 00:00:00 2001 From: Dominic Kramer Date: Mon, 5 Jun 2017 15:13:02 -0700 Subject: [PATCH 1/7] errors-report:Document unhandledRejection handling --- packages/error-reporting/README.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/error-reporting/README.md b/packages/error-reporting/README.md index a2dc5b51ecf..bd8d8ef4f9e 100644 --- a/packages/error-reporting/README.md +++ b/packages/error-reporting/README.md @@ -66,7 +66,7 @@ errors.report(new Error('Something broke!')); ## Catching and Reporting Application-wide Uncaught Errors -*It is recommended to catch `uncaughtExceptions` for production-deployed applications.* +Uncaught exceptions and unhandled rejections are not reported by default. *It is recommended to process `uncaughtException`s and `unhandledRejection`s for production-deployed applications.* ```js var errors = require('@google-cloud/error-reporting')(); @@ -76,9 +76,13 @@ process.on('uncaughtException', (e) => { // Report that same error the Stackdriver Error Service errors.report(e); }); + +process.on('unhandledRejection', (reason, p) => { + errors.report('Unhandled rejection of promise: ' + p + ', reason: ' + reason); +}); ``` -More information about uncaught exception handling in Node.js and what it means for your application can be found [here](https://nodejs.org/api/process.html#process_event_uncaughtexception). +More information on uncaught exception handling in Node.js can be found [here](https://nodejs.org/api/process.html#process_event_uncaughtexception), and more information on unhandled promise handling can be found [here](https://nodejs.org/api/process.html#process_event_unhandledrejection). ## Running on Google Cloud Platform From 666b57e01c8c9ab7186ef31ad4092bc12eae3bb7 Mon Sep 17 00:00:00 2001 From: Dominic Kramer Date: Thu, 8 Jun 2017 10:55:54 -0700 Subject: [PATCH 2/7] error-reporting: Unhandled rejections are reported Now unhandled rejections are reported, by default, to the error reporting console. The `reportUnhandledRejections` configuration option can be used to disable the reporting of unhandled rejections. --- packages/error-reporting/README.md | 19 ++- packages/error-reporting/src/configuration.js | 24 +++ packages/error-reporting/src/index.js | 10 ++ .../system-test/testAuthClient.js | 143 ++++++++++++++---- .../test/unit/configuration.js | 23 +++ 5 files changed, 180 insertions(+), 39 deletions(-) diff --git a/packages/error-reporting/README.md b/packages/error-reporting/README.md index bd8d8ef4f9e..e2ead9fb467 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 reported by default. The reporting of unhandled rejections can be disabled 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 -Uncaught exceptions and unhandled rejections are not reported by default. *It is recommended to process `uncaughtException`s and `unhandledRejection`s 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. However, whether or not, and if so how, the addition of such a listener influences the execution of an application is specific to that particular application. As such, it is necessary for `uncaughtException`s to be reported manually. ```js var errors = require('@google-cloud/error-reporting')(); @@ -76,13 +84,9 @@ process.on('uncaughtException', (e) => { // Report that same error the Stackdriver Error Service errors.report(e); }); - -process.on('unhandledRejection', (reason, p) => { - errors.report('Unhandled rejection of promise: ' + p + ', reason: ' + reason); -}); ``` -More information on uncaught exception handling in Node.js can be found [here](https://nodejs.org/api/process.html#process_event_uncaughtexception), and more information on unhandled promise handling can be found [here](https://nodejs.org/api/process.html#process_event_unhandledrejection). +More information about uncaught exception handling in Node.js and what it means for your application can be found [here](https://nodejs.org/api/process.html#process_event_uncaughtexception). ## Running on Google Cloud Platform @@ -160,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. The default value of this property is true. + reportUnhandledRejections: false, 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..143149a3e4b 100644 --- a/packages/error-reporting/src/index.js +++ b/packages/error-reporting/src/index.js @@ -154,6 +154,16 @@ 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 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..ebce14fb509 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,43 @@ 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 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..dd981c6aad0 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 report unhandledRejections', function() { + assert.strictEqual(c.getReportUnhandledRejections(), true); + }); }); 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); + }); + }); }); }); }); From a05f25bc79b3b893193f6f393cf7509af9095e81 Mon Sep 17 00:00:00 2001 From: Dominic Kramer Date: Mon, 12 Jun 2017 16:31:27 -0700 Subject: [PATCH 3/7] Report unhandledRejections is disabled by default --- packages/error-reporting/test/unit/configuration.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/error-reporting/test/unit/configuration.js b/packages/error-reporting/test/unit/configuration.js index dd981c6aad0..4d1343d04bc 100644 --- a/packages/error-reporting/test/unit/configuration.js +++ b/packages/error-reporting/test/unit/configuration.js @@ -93,7 +93,7 @@ describe('Configuration class', function() { {service: 'node', version: undefined}); }); it('Should specify to report unhandledRejections', function() { - assert.strictEqual(c.getReportUnhandledRejections(), true); + assert.strictEqual(c.getReportUnhandledRejections(), false); }); }); describe('with ignoreEnvironmentCheck', function() { From 34d6f3793a7e36f3c839bb68fb2f219b775e31f4 Mon Sep 17 00:00:00 2001 From: Dominic Kramer Date: Wed, 14 Jun 2017 09:17:04 -0700 Subject: [PATCH 4/7] Address GitHub comments --- packages/error-reporting/README.md | 2 +- packages/error-reporting/src/index.js | 3 ++- packages/error-reporting/system-test/testAuthClient.js | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/error-reporting/README.md b/packages/error-reporting/README.md index e2ead9fb467..b6c67093d4f 100644 --- a/packages/error-reporting/README.md +++ b/packages/error-reporting/README.md @@ -74,7 +74,7 @@ If unhandled rejections are set to be reported, then, when an unhandled rejectio 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. However, whether or not, and if so how, the addition of such a listener influences the execution of an application is specific to that particular application. As such, it is necessary for `uncaughtException`s to be reported manually. +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')(); diff --git a/packages/error-reporting/src/index.js b/packages/error-reporting/src/index.js index 143149a3e4b..3fec7a6e052 100644 --- a/packages/error-reporting/src/index.js +++ b/packages/error-reporting/src/index.js @@ -160,7 +160,8 @@ function Errors(initConfiguration) { process.on('unhandledRejection', function(reason) { console.log('UnhandledPromiseRejectionWarning: ' + 'Unhandled promise rejection: ' + reason + - '. This rejection has been reported to the error-reporting console.'); + '. This rejection has been reported to the ' + + 'Google Cloud Platform error-reporting console.'); that.report(reason); }); } diff --git a/packages/error-reporting/system-test/testAuthClient.js b/packages/error-reporting/system-test/testAuthClient.js index ebce14fb509..a97e2198842 100644 --- a/packages/error-reporting/system-test/testAuthClient.js +++ b/packages/error-reporting/system-test/testAuthClient.js @@ -512,7 +512,8 @@ describe('error-reporting', function() { setImmediate(function() { var expected = 'UnhandledPromiseRejectionWarning: Unhandled ' + 'promise rejection: ' + rejectValue + - '. This rejection has been reported to the error-reporting console.'; + '. 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); From a57d5edae019e0d876b9f87b1a0d16e7bb989864 Mon Sep 17 00:00:00 2001 From: Dominic Kramer Date: Wed, 14 Jun 2017 15:00:26 -0700 Subject: [PATCH 5/7] Specify rejections not reported by default in docs --- packages/error-reporting/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/error-reporting/README.md b/packages/error-reporting/README.md index b6c67093d4f..0812cfb617f 100644 --- a/packages/error-reporting/README.md +++ b/packages/error-reporting/README.md @@ -66,7 +66,7 @@ errors.report(new Error('Something broke!')); ## Unhandled Rejections -Unhandled Rejections are reported by default. The reporting of unhandled rejections can be disabled using the `reportUnhandledRejections` configuration option. See the [Configuration](#configuration) section for more details. +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. From 6c0a399e134f754daa17c5957244a597e24dea13 Mon Sep 17 00:00:00 2001 From: Dominic Kramer Date: Thu, 15 Jun 2017 09:45:18 -0700 Subject: [PATCH 6/7] Update README config sect to match default values In particular, the configuration section, as well as the section discussing the handling of unhandled rejections, discussed the default behavior of handling unhandled rejections. Now the default action is not described in the configuration section so that only one section describes that behavior. --- packages/error-reporting/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/error-reporting/README.md b/packages/error-reporting/README.md index 0812cfb617f..a7c4bb137c7 100644 --- a/packages/error-reporting/README.md +++ b/packages/error-reporting/README.md @@ -165,8 +165,8 @@ var errors = require('@google-cloud/error-reporting')({ // defaults to 2 (warnings) logLevel: 2, // determines whether or not unhandled rejections are reported to the - // error-reporting console. The default value of this property is true. - reportUnhandledRejections: false, + // error-reporting console + reportUnhandledRejections: true, serviceContext: { service: 'my-service', version: 'my-service-version' From cb7362ce3cb5e46c21ddf110de8fa8a92a3990a8 Mon Sep 17 00:00:00 2001 From: Dominic Kramer Date: Thu, 15 Jun 2017 09:58:34 -0700 Subject: [PATCH 7/7] Update test name to match behavior --- packages/error-reporting/test/unit/configuration.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/error-reporting/test/unit/configuration.js b/packages/error-reporting/test/unit/configuration.js index 4d1343d04bc..b1d13f0d306 100644 --- a/packages/error-reporting/test/unit/configuration.js +++ b/packages/error-reporting/test/unit/configuration.js @@ -92,7 +92,7 @@ describe('Configuration class', function() { assert.deepEqual(c.getServiceContext(), {service: 'node', version: undefined}); }); - it('Should specify to report unhandledRejections', function() { + it('Should specify to not report unhandledRejections', function() { assert.strictEqual(c.getReportUnhandledRejections(), false); }); });