feat: add a new reportMode configuration option (#295)

DominicKramer · web-flow · commit 4cae0af7291c · 2019-01-11T13:14:00.000-08:00
Adds the `reportMode` configuration option. The `ignoreEnvironmentCheck` configuration option is now deprecated, and the `reportMode` option should be used instead. In particular, the `reportMode` can have one of three values: * 'production' (default) - Only report errors if NODE_ENV is set to "production".: * 'always' - Always report errors regardless of the value of NODE_ENV. * 'never' - Never report errors regardless of the value of NODE_ENV. Fixes #127
diff --git a/src/configuration.ts b/src/configuration.ts
@@ -35,13 +35,16 @@ export interface Logger {
   fatal(...args: Array<{}>): void;
 }
 
+export type ReportMode = 'production'|'always'|'never';
+
 export interface ConfigurationOptions {
   projectId?: string;
   keyFilename?: string;
   logLevel?: string|number;
   key?: string;
   serviceContext?: {service?: string; version?: string;};
   ignoreEnvironmentCheck?: boolean;
+  reportMode?: ReportMode;
   credentials?: {};
   reportUnhandledRejections?: boolean;
 }
@@ -74,7 +77,7 @@ export interface ServiceContext {
  */
 export class Configuration {
   _logger: Logger;
-  _shouldReportErrorsToAPI: boolean;
+  _reportMode: ReportMode;
   _projectId: string|null;
   _key: string|null;
   keyFilename: string|null;
@@ -89,23 +92,7 @@ export class Configuration {
      * for configuration logging purposes.
      */
     this._logger = logger;
-    /**
-     * The _shouldReportErrorsToAPI property is meant to denote whether or not
-     * the Stackdriver error reporting library will actually try to report
-     * Errors to the Stackdriver Error API. The value of this property is
-     * derived from the `NODE_ENV` environmental variable or the value of
-     * ignoreEnvironmentChec property if present in the runtime configuration.
-     * If either the `NODE_ENV` variable is set to 'production' or the
-     * ignoreEnvironmentCheck propery on the runtime configuration is set to
-     * true then the error reporting library attempt to send errors to the Error
-     * API. Otherwise the value will remain false and errors will not be
-     * reported to the API.
-     * @memberof Configuration
-     * @private
-     * @type {Boolean}
-     * @defaultvalue false
-     */
-    this._shouldReportErrorsToAPI = false;
+    this._reportMode = 'production';
     /**
      * The _projectId property is meant to contain the string project id that
      * the hosting application is running under. The project id is a unique
@@ -259,6 +246,12 @@ export class Configuration {
       }
     }
   }
+  _determineReportMode() {
+    if (this._givenConfiguration.reportMode) {
+      this._reportMode =
+          this._givenConfiguration.reportMode.toLowerCase() as ReportMode;
+    }
+  }
   /**
    * The _gatherLocalConfiguration function is responsible for determining
    * directly determing whether the properties `reportUncaughtExceptions` and
@@ -273,23 +266,56 @@ export class Configuration {
    * @returns {Undefined} - does not return anything
    */
   _gatherLocalConfiguration() {
-    if (this._givenConfiguration.ignoreEnvironmentCheck === true) {
-      this._shouldReportErrorsToAPI = true;
-    } else if (
-        has(this._givenConfiguration, 'ignoreEnvironmentCheck') &&
-        !is.boolean(this._givenConfiguration.ignoreEnvironmentCheck)) {
-      throw new Error('config.ignoreEnvironmentCheck must be a boolean');
-    } else {
-      this._shouldReportErrorsToAPI = env.NODE_ENV === 'production';
+    let isReportModeValid = true;
+    if (has(this._givenConfiguration, 'reportMode')) {
+      const reportMode = this._givenConfiguration.reportMode;
+      isReportModeValid = is.string(reportMode) &&
+          (reportMode === 'production' || reportMode === 'always' ||
+           reportMode === 'never');
+    }
+
+    if (!isReportModeValid) {
+      throw new Error(
+          'config.reportMode must a string that is one ' +
+          'of "production", "always", or "never".');
     }
-    if (!this._shouldReportErrorsToAPI) {
+
+    const hasEnvCheck = has(this._givenConfiguration, 'ignoreEnvironmentCheck');
+    const hasReportMode = has(this._givenConfiguration, 'reportMode');
+    if (hasEnvCheck) {
+      this._logger.warn(
+          'The "ignoreEnvironmentCheck" config option is deprecated.  ' +
+          'Use the "reportMode" config option instead.');
+    }
+    if (hasEnvCheck && hasReportMode) {
       this._logger.warn([
-        'Stackdriver error reporting client has not been configured to send',
-        'errors, please check the NODE_ENV environment variable and make sure it',
-        'is set to "production" or the ignoreEnvironmentCheck property is set to',
-        'true in the runtime configuration object',
+        'Both the "ignoreEnvironmentCheck" and "reportMode" configuration options',
+        'have been specified.  The "reportMode" option will take precedence.'
       ].join(' '));
+      this._determineReportMode();
+    } else if (hasEnvCheck) {
+      if (this._givenConfiguration.ignoreEnvironmentCheck === true) {
+        this._reportMode = 'always';
+      } else if (
+          has(this._givenConfiguration, 'ignoreEnvironmentCheck') &&
+          !is.boolean(this._givenConfiguration.ignoreEnvironmentCheck)) {
+        throw new Error('config.ignoreEnvironmentCheck must be a boolean');
+      } else {
+        this._reportMode = 'production';
+      }
+    } else if (hasReportMode) {
+      this._determineReportMode();
     }
+
+    if (this.isReportingEnabled() && !this.getShouldReportErrorsToAPI()) {
+      this._logger.warn([
+        'The stackdriver error reporting client is configured to report errors',
+        'if and only if the NODE_ENV environment variable is set to "production".',
+        'Errors will not be reported.  To have errors always reported, regardless of the',
+        'value of NODE_ENV, set the reportMode configuration option to "always".'
+      ].join(' '));
+    }
+
     if (is.string(this._givenConfiguration.key)) {
       this._key = this._givenConfiguration.key!;
     } else if (has(this._givenConfiguration, 'key')) {
@@ -349,14 +375,22 @@ export class Configuration {
     return this._projectId;
   }
   /**
-   * Returns the _shouldReportErrorsToAPI property on the instance.
+   * Returns whether this configuration specifies that errors should be
+   * reported to the error reporting API.  That is, "reportMode" is
+   * either set to "always" or it is set to "production" and the value
+   * of the NODE_ENV environment variable is "production".
    * @memberof Configuration
    * @public
    * @function getShouldReportErrorsToAPI
-   * @returns {Boolean} - returns the _shouldReportErrorsToAPI property
+   * @returns {Boolean} - whether errors should be reported to the API
    */
   getShouldReportErrorsToAPI() {
-    return this._shouldReportErrorsToAPI;
+    return this._reportMode === 'always' ||
+        (this._reportMode === 'production' &&
+         (process.env.NODE_ENV || '').toLowerCase() === 'production');
+  }
+  isReportingEnabled() {
+    return this._reportMode !== 'never';
   }
   /**
    * Returns the _projectId property on the instance.
diff --git a/src/google-apis/auth-client.ts b/src/google-apis/auth-client.ts
@@ -159,7 +159,11 @@ export class RequestHandler extends Service {
       userCb?:
           (err: Error|null, response: http.ServerResponse|null,
            body: {}) => void) {
-    const cb: Function = (is.function(userCb) ? userCb : RequestHandler.noOp)!;
+      const cb: Function = (is.function(userCb) ? userCb : RequestHandler.noOp)!;
+      if (!this._config.isReportingEnabled()) {
+        cb(null, null, {});
+        return;
+      }
       if (this._config.getShouldReportErrorsToAPI()) {
         this.request(
             {
@@ -181,14 +185,14 @@ export class RequestHandler extends Service {
             });
       } else {
         cb(new Error([
-             'Stackdriver error reporting client has not been configured to send',
-             'errors, please check the NODE_ENV environment variable and make sure',
-             'it is set to "production" or set the ignoreEnvironmentCheck property',
-             'to true in the runtime configuration object',
+             'The stackdriver error reporting client is configured to report errors',
+             'if and only if the NODE_ENV environment variable is set to "production".',
+             'Errors will not be reported.  To have errors always reported, regardless of the',
+             'value of NODE_ENV, set the reportMode configuration option to "always".'
            ].join(' ')),
            null, null);
       }
-  }
+    }
   }
 
   /**
diff --git a/src/index.ts b/src/index.ts
@@ -55,6 +55,14 @@ export type RestifyRequestHandler = (req: any, res: any, next: Function) => any;
  * @property {Boolean} [ignoreEnvironmentCheck] - flag indicating whether or not
  *  to communicate errors to the Stackdriver service even if NODE_ENV is not set
  *  to production
+ * @property {String} [reportMode] - flag indicating whether or not
+ *  to communicate errors to the Stackdriver service.  Possible values are:
+ *  -> 'production' (default)
+ *     -> Only report errors if NODE_ENV is set to "production".
+ *  -> 'always'
+ *     -> Always report errors regardless of the value of NODE_ENV.
+ *  -> 'never'
+ *     -> Never report errors regardless of the value of NODE_ENV.
  */
 
 /**
diff --git a/system-test/error-reporting.ts b/system-test/error-reporting.ts
@@ -161,7 +161,7 @@ describe('Request/Response lifecycle mocking', () => {
             .post('/events:report?');
     logger = createLogger({logLevel: 5});
     client = new RequestHandler(
-        new Configuration({ignoreEnvironmentCheck: true}, logger), logger);
+        new Configuration({reportMode: 'always'}, logger), logger);
   });
 
   afterEach(() => {
@@ -207,8 +207,7 @@ describe('Request/Response lifecycle mocking', () => {
        const key = env.apiKey;
        const logger = createLogger({logLevel: 5});
        const client = new RequestHandler(
-           new Configuration({key, ignoreEnvironmentCheck: true}, logger),
-           logger);
+           new Configuration({key, reportMode: 'always'}, logger), logger);
        fakeService.query({key}).reply(200, (uri: string) => {
          assert(uri.indexOf('key=' + key) > -1);
          return {};
@@ -242,7 +241,7 @@ describe('Client creation', () => {
        const cfg = new Configuration(
            {
              projectId: env.injected().projectId,
-             ignoreEnvironmentCheck: true,
+             reportMode: 'always',
            },
            logger);
        this.timeout(10000);
@@ -262,7 +261,7 @@ describe('Client creation', () => {
      function(this, done) {
        env.sterilizeProcess().setProjectId().setKeyFilename();
        const logger = createLogger({logLevel: 5});
-       const cfg = new Configuration({ignoreEnvironmentCheck: true}, logger);
+       const cfg = new Configuration({reportMode: 'always'}, logger);
        this.timeout(10000);
        assert.doesNotThrow(() => {
          new RequestHandler(cfg, logger)
@@ -283,7 +282,7 @@ describe('Client creation', () => {
        const cfg = new Configuration(
            {
              projectId: '' + Number(env.injected().projectNumber),
-             ignoreEnvironmentCheck: true,
+             reportMode: 'always',
            },
            logger);
        this.timeout(10000);
@@ -303,7 +302,7 @@ describe('Client creation', () => {
      function(this, done) {
        env.sterilizeProcess().setKeyFilename().setProjectNumber();
        const logger = createLogger({logLevel: 5});
-       const cfg = new Configuration({ignoreEnvironmentCheck: true}, logger);
+       const cfg = new Configuration({reportMode: 'always'}, logger);
        this.timeout(10000);
        assert.doesNotThrow(() => {
          new RequestHandler(cfg, logger)
@@ -319,10 +318,10 @@ describe('Client creation', () => {
 
 describe('Expected Behavior', () => {
   const ERROR_STRING = [
-    'Stackdriver error reporting client has not been configured to send',
-    'errors, please check the NODE_ENV environment variable and make',
-    'sure it is set to "production" or set the ignoreEnvironmentCheck',
-    'property to true in the runtime configuration object',
+    'The stackdriver error reporting client is configured to report errors',
+    'if and only if the NODE_ENV environment variable is set to "production".',
+    'Errors will not be reported.  To have errors always reported, regardless of the',
+    'value of NODE_ENV, set the reportMode configuration option to "always".'
   ].join(' ');
 
   const er = new Error(ERR_TOKEN);
@@ -332,11 +331,11 @@ describe('Expected Behavior', () => {
     env.sterilizeProcess();
   });
 
-  it('Should callback with an error with a configuration to not report errors',
+  it('Should callback with an error with a configuration that cannot report errors',
      done => {
        env.sterilizeProcess().setKeyFilename().setProjectId();
        process.env.NODE_ENV = 'null';
-       const logger = createLogger({logLevel: 5});
+       const logger = createLogger({logLevel: 5, reportMode: 'production'});
        const client =
            new RequestHandler(new Configuration(undefined, logger), logger);
        client.sendError({} as ErrorMessage, (err, response) => {
@@ -353,7 +352,7 @@ describe('Expected Behavior', () => {
     const cfg = new Configuration(
         {
           projectId: env.injected().projectId,
-          ignoreEnvironmentCheck: true,
+          reportMode: 'always',
         },
         logger);
     const client = new RequestHandler(cfg, logger);
@@ -373,7 +372,7 @@ describe('Expected Behavior', () => {
     const cfg = new Configuration(
         {
           projectId: '' + Number(env.injected().projectNumber),
-          ignoreEnvironmentCheck: true,
+          reportMode: 'always',
         },
         logger);
     const client = new RequestHandler(cfg, logger);
@@ -467,7 +466,7 @@ describe('error-reporting', () => {
     process.removeAllListeners('unhandledRejection');
     const initConfiguration = Object.assign(
         {
-          ignoreEnvironmentCheck: true,
+          reportMode: 'always' as 'always',
           serviceContext: {
             service: SERVICE,
             version: VERSION,
diff --git a/test/unit/configuration.ts b/test/unit/configuration.ts
diff --git a/test/unit/google-apis/auth-client.ts b/test/unit/google-apis/auth-client.ts
