error-reporting: Unhandled rejections are reported

DominicKramer · DominicKramer · commit 4ea2de296f44 · 2017-06-08T10:55:54.000-07:00
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.
diff --git 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
 
@@ -149,6 +153,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
@@ -135,6 +135,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 = true;
   /**
    * The _version of the Error reporting library that is currently being run.
    * This information will be logged in errors communicated to the Stackdriver
@@ -267,6 +275,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
@@ -374,4 +388,14 @@ Configuration.prototype.getServiceContext = function() {
 Configuration.prototype.getVersion = function() {
   return this._version;
 };
+/**
+ * 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
@@ -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
@@ -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();
+      });
+    });
+  });
 });
diff --git a/packages/error-reporting/test/unit/configuration.js b/packages/error-reporting/test/unit/configuration.js
@@ -96,6 +96,9 @@ describe('Configuration class', function() {
         it('Should have a version corresponding to package.json', function() {
           assert.strictEqual(c.getVersion(), version);
         });
+        it('Should specify to report unhandledRejections', function() {
+          assert.strictEqual(c.getReportUnhandledRejections(), true);
+        });
       });
       describe('with ignoreEnvironmentCheck', function() {
         var conf = merge({}, stubConfig, {ignoreEnvironmentCheck: true});
@@ -138,6 +141,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() {
@@ -282,6 +292,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);
+        });
+      });
     });
   });
 });
