error-reporting: Fix bug with errors not published

packages/error-reporting/README.md

@@ -145,19 +145,24 @@ var errors = require('@google-cloud/error-reporting')({
 ```js
 var errors = require('@google-cloud/error-reporting')();
 
-// Use the error message builder to custom set all message fields
+// Use the error message builder to customize all fields ...
 var errorEvt = errors.event()
                      .setMessage('My error message')
                      .setUser('root@nexus');
 errors.report(errorEvt, () => console.log('done!'));
 
-// Or just use a regular error
+// or just use a regular error ...
 errors.report(new Error('My error message'), () => console.log('done!'));
 
-// One can even just use a string
+// or one can even just use a string.
 errors.report('My error message');
 ```
 
+The stack trace associated with an error can be viewed in the error reporting console.
+* If the `errors.report` method is given an `ErrorMessage` object built using the `errors.event` method, the stack trace at the point where the error event was constructed will be used.
+* If the `errors.report` method is given an `Error` object, the stack trace where the error was instantiated will be used.
+* If the `errors.report` method is given a string, the stack trace at the point where `errors.report` is invoked will be used.
+
 ### Using Express
 
 ```js

packages/error-reporting/src/index.js

@@ -106,7 +106,7 @@ function Errors(initConfiguration) {
    *  console.log('done!');
    * });
    */
-  this.report = manual(this._client, this._config);
+  this.report = manual(this._client, this._config, this._logger);
   /**
    * @example
    * // Use to create and report errors manually with a high-degree

packages/error-reporting/src/interfaces/manual.js

@@ -33,10 +33,12 @@ var errorHandlerRouter = require('../error-router.js');
  * @param {AuthClient} client - an initialized API client
  * @param {NormalizedConfigurationVariables} config - the environmental
  *  configuration
+ * @param {Object} logger - The logger instance created when the library API has
+ *  been initialized.
  * @returns {reportManualError} - a bound version of the reportManualError
  *  function
  */
-function handlerSetup(client, config) {
+function handlerSetup(client, config, logger) {
   /**
    * The interface for manually reporting errors to the Google Error API in
    * application code.
@@ -74,6 +76,21 @@ function handlerSetup(client, config) {
     }
 
     if (err instanceof ErrorMessage) {
+      // The API expects the error to contain a stack trace.  Thus we
+      // append the stack trace of the point where the error was constructed.
+      // See the `message-builder.js` file for more details.
+      if (err._autoGeneratedStackTrace) {
+        err.setMessage(err.message + '\n' + err._autoGeneratedStackTrace);
+        // Delete the property so that if the ErrorMessage is reported a second
+        // time, the stack trace is not appended a second time.  Also, the API
+        // will not accept the ErrorMessage if it has additional properties.
+        delete err._autoGeneratedStackTrace;
+      } else {
+        logger.warn('Encountered a manually constructed error with message \"'+
+                    err.message + '\" but without a construction site ' +
+                    'stack trace.  This error might not be visible in the ' +
+                    'error reporting console.');
+      }
       em = err;
     } else {
       em = new ErrorMessage();

packages/error-reporting/src/interfaces/message-builder.js

@@ -33,9 +33,26 @@ function handlerSetup(config) {
    * @returns {ErrorMessage} - returns a new instance of the ErrorMessage class
    */
   function newMessage() {
-    return new ErrorMessage().setServiceContext(
+    // The API expects a reported error to contain a stack trace.
+    // However, users do not need to provide a stack trace for ErrorMessage
+    // objects built using the message builder.  Instead, here we store
+    // the stack trace with the parts that reference the error-reporting's
+    // internals removed.  Then when the error is reported, the stored
+    // stack trace will be appended to the user's message for the error.
+    //
+    // Note: The stack trace at the point where the user constructed the
+    //       error is used instead of the stack trace where the error is
+    //       reported to be consistent with the behavior of reporting a
+    //       an error when reporting an actual Node.js Error object.
+    var fauxError = new Error('');
+    var fullStack = fauxError.stack.split('\n');
+    var cleanedStack = fullStack.slice(2).join('\n');
+
+    var em = new ErrorMessage().setServiceContext(
                          config.getServiceContext().service,
                          config.getServiceContext().version);
+    em._autoGeneratedStackTrace = cleanedStack;
+    return em;
   }
 
   return newMessage;

packages/error-reporting/system-test/testAuthClient.js

@@ -361,10 +361,9 @@ describe('Expected Behavior', function() {
 });
 
 describe('error-reporting', function() {
-  const TIMESTAMP = Date.now();
   const BASE_NAME = 'error-reporting-system-test';
   function buildName(suffix) {
-    return [TIMESTAMP, BASE_NAME, suffix].join('_');
+    return [Date.now(), BASE_NAME, suffix].join('_');
   }
 
   const SERVICE_NAME = buildName('service-name');
@@ -390,15 +389,8 @@ describe('error-reporting', function() {
     });
   });
 
-  it('Should correctly publish errors', function(done) {
-    // After an error is reported, this test waits TIMEOUT ms before
-    // verifying the error has been reported to ensure the system had
-    // enough time to receive the error report and process it.
-    // As such, this test is set to fail due to a timeout only if sufficiently
-    // more than TIMEOUT ms has elapsed to avoid test fragility.
-    this.timeout(TIMEOUT * 2);
-    var errorId = buildName('message');
-    errors.report(new Error(errorId), function(err, response, body) {
+  function verifyReporting(errOb, messageTest, timeout, cb) {
+    errors.report(errOb, function(err, response, body) {
       assert.ifError(err);
       assert(isObject(response));
       assert.deepEqual(body, {});
@@ -410,7 +402,7 @@ describe('error-reporting', function() {
 
           var matchedErrors = groups.filter(function(errItem) {
             return errItem && errItem.representative &&
-              errItem.representative.message.startsWith('Error: ' + errorId);
+              messageTest(errItem.representative.message);
           });
 
           // The error should have been reported exactly once
@@ -424,9 +416,53 @@ describe('error-reporting', function() {
           assert.ok(context);
           assert.strictEqual(context.service, SERVICE_NAME);
           assert.strictEqual(context.version, SERVICE_VERSION);
-          done();
+          cb();
         });
-      }, TIMEOUT);
+      }, timeout);
     });
+  }
+
+  // For each test below, after an error is reported, the test waits
+  // TIMEOUT ms before verifying the error has been reported to ensure
+  // the system had enough time to receive the error report and process it.
+  // As such, each test is set to fail due to a timeout only if sufficiently
+  // more than TIMEOUT ms has elapsed to avoid test fragility.
+
+  it('Should correctly publish errors using the Error constructor',
+    function(done) {
+    this.timeout(TIMEOUT * 2);
+    var errorId = buildName('with-error-constructor');
+    var errOb = new Error(errorId);
+    verifyReporting(errOb, function(message) {
+      return message.startsWith('Error: ' + errorId);
+    }, TIMEOUT, done);
+  });
+
+  it('Should correctly publish errors using a string', function(done) {
+    this.timeout(TIMEOUT * 2);
+    var errorId = buildName('with-string');
+    verifyReporting(errorId, function(message) {
+      return message.startsWith(errorId);
+    }, TIMEOUT, done);
+  });
+
+  it('Should correctly publish errors using an error builder', function(done) {
+    this.timeout(TIMEOUT * 2);
+    var errorId = buildName('with-error-builder');
+    var errOb = (function definitionSiteFunction() {
+      return errors.event()
+                   .setMessage(errorId);
+    })();
+    (function callingSiteFunction() {
+      verifyReporting(errOb, function(message) {
+        // Verify that the stack trace of the constructed error
+        // uses the stack trace at the point where the error was constructed
+        // and not the stack trace at the point where the `report` method
+        // was called.
+        return message.startsWith(errorId) &&
+          message.indexOf('callingSiteFunction') === -1 &&
+          message.indexOf('definitionSiteFunction') !== -1;
+      }, TIMEOUT, done);
+    })();
   });
 });

packages/error-reporting/test/unit/interfaces/manual.js

@@ -37,7 +37,21 @@ describe('Manual handler', function() {
       }
     }
   };
-  var report = manual(client, config);
+  var report = manual(client, config, {
+    warn: function(message) {
+      // The use of `report` in this class should issue the following warning
+      // becasue the `report` class is used directly and, as such, cannot
+      // by itself have information where a ErrorMesasge was constructed.  It
+      // only knows that an error has been reported.
+      // Thus, the ErrorMessage objects given to the `report` method in the
+      // tests do not have construction site information to verify that if
+      // that information is not available, the user is issued a warning.
+      assert.strictEqual(message, 'Encountered a manually constructed error ' +
+        'with message "builder test" but without a construction site stack ' +
+        'trace.  This error might not be visible in the error reporting ' +
+        'console.');
+    }
+  });
   describe('Report invocation behaviour', function() {
     it('Should allow argument-less invocation', function() {
       var r = report();
@@ -140,20 +154,20 @@ describe('Manual handler', function() {
 
   describe('Custom Payload Builder', function() {
     it('Should accept builder inst as only argument', function() {
-      var msg = 'test';
+      var msg = 'builder test';
       var r = report(new ErrorMessage().setMessage(msg));
-      assert.strictEqual(r.message, msg,
+      assert(r.message.startsWith(msg),
         'string message should propagate from error message inst');
     });
     it('Should accept builder and request as arguments', function() {
-      var msg = 'test';
+      var msg = 'builder test';
       var oldReq = {method: 'GET'};
       var newReq = {method: 'POST'};
       var r = report(
         new ErrorMessage().setMessage(msg).consumeRequestInformation(oldReq),
         newReq
      );
-      assert.strictEqual(r.message, msg,
+      assert(r.message.startsWith(msg),
         'string message should propagate from error message inst');
       assert.strictEqual(r.context.httpRequest.method, newReq.method,
         [
@@ -163,7 +177,7 @@ describe('Manual handler', function() {
      );
     });
     it('Should accept message and additional message params as', function() {
-      var oldMsg = 'test';
+      var oldMsg = 'builder test';
       var newMsg = 'analysis';
       var r = report(
         new ErrorMessage().setMessage(oldMsg),
@@ -176,7 +190,7 @@ describe('Manual handler', function() {
         ].join('\n'));
     });
     it('Should accept message and callback function', function(done) {
-      var oldMsg = 'test';
+      var oldMsg = 'builder test';
       report(
         new ErrorMessage().setMessage(oldMsg),
         function() { done(); }