Repository: dbader/node-datadog-metrics (js)

Issue:
Make errors more obvious
After getting no data errors this morning from Datadog, I found ```metrics ERROR: add_metrics failed: null (err=Error: certificate has expired, status=0) +324ms``` with DEBUG mode enabled.

I'd think this should fire an error, not just a debug log.

Pull request description:
Be more noisy about errors and add an `onError` option
This fixes #52 by making two changes:

1. Add an `onError` option where users can provide an error handler for auto-flushes, just like they do when calling `flush()` explicitly. This is useful for getting more detail about the error, doing custom logging, or sending the error to a separate error tracking service, like Sentry.io.

2. In cases where the error was not handled (either via the `onError` option or by manually calling `flush()` with an error handler), this logs the error to the console *regardless* of the environment’s `DEBUG` setting. This particular error is a pretty big deal and should not be silenced.

I did the second change by making a special logging function for errors, since `debug` does not have a concept of levels or any way to temporarily override the environment (you can call `enable()`, but that overrides whatever was set before, so you can’t just put things back the way they were once you do that).

Patch:
diff --git a/.jshintrc b/.jshintrc
index b201f94..d12b4af 100644
--- a/.jshintrc
+++ b/.jshintrc
@@ -20,8 +20,9 @@
     //
     // Setting to true ENABLES a check
 
-    // Enforce === instead of ==
+    // Enforce === instead of ==, except when comparing to `null`.
     "eqeqeq": true,
+    "eqnull": true,
     // Force hasOwnProperty checks within `for in`
     "forin": true,
     // Prohibits overwriting native types like `Array` or `Date`
diff --git a/README.md b/README.md
index dd1a322..f649678 100644
--- a/README.md
+++ b/README.md
@@ -93,7 +93,10 @@ var metricsLogger = new metrics.BufferedMetricsLogger({
     host: 'myhost',
     prefix: 'myapp.',
     flushIntervalSeconds: 15,
-    defaultTags: ['env:staging', 'region:us-east-1']
+    defaultTags: ['env:staging', 'region:us-east-1'],
+    onError (error) {
+        console.error('There was an error auto-flushing metrics:', error);
+    }
 });
 metricsLogger.gauge('mygauge', 42);
 ```
@@ -127,6 +130,11 @@ Where `options` is an object and can contain the following:
       Datadog-metrics looks for the APP key in `DATADOG_APP_KEY` by default.
 * `defaultTags`: Default tags used for all metric reporting. (optional)
     * Set tags that are common to all metrics.
+* `onError`: A function to call when there are asynchronous errors seding
+    buffered metrics to Datadog. It takes one argument (the error). (optional)
+    * If the error was not handled (either by setting this option or by
+      specifying a handler when manually calling `flush()`), the error will be
+      logged to stdout.
 * `reporter`: An object that actually sends the buffered metrics. (optional)
     * There are two built-in reporters you can use:
         1. `reporters.DataDogReporter` sends metrics to Datadog’s API, and is
@@ -281,6 +289,17 @@ npm test
 
         (Thanks to @Mr0grog.)
 
+    * Add an `onError` option for handling asynchronous errors while flushing. You can use this to get details on an error or to send error info to another error tracking service like Sentry.io:
+
+        ```js
+        const metrics = require('datadog-metrics');
+        metrics.init({
+            onError (error) {
+                console.error('There was an error sending to Datadog:', error);
+            }
+        });
+        ```
+
     * Expose built-in reporter classes for public use. If you need to disable the metrics library for some reason, you can now do so with:
 
         ```js
diff --git a/lib/loggers.js b/lib/loggers.js
index 2647752..e174276 100644
--- a/lib/loggers.js
+++ b/lib/loggers.js
@@ -1,6 +1,5 @@
 'use strict';
-const debug = require('debug')('metrics');
-
+const { logDebug } = require('./logging');
 const Aggregator = require('./aggregators').Aggregator;
 const DataDogReporter = require('./reporters').DataDogReporter;
 const Gauge = require('./metrics').Gauge;
@@ -44,10 +43,16 @@ function BufferedMetricsLogger(opts) {
     this.prefix = opts.prefix || '';
     this.flushIntervalSeconds = opts.flushIntervalSeconds;
 
+    if (typeof opts.onError === 'function') {
+        this.onError = opts.onError;
+    } else if (opts.onError != null) {
+        throw new TypeError(`The 'onError' option must be a function`);
+    }
+
     if (this.flushIntervalSeconds) {
-        debug('Auto-flushing every %d seconds', this.flushIntervalSeconds);
+        logDebug('Auto-flushing every %d seconds', this.flushIntervalSeconds);
     } else {
-        debug('Auto-flushing is disabled');
+        logDebug('Auto-flushing is disabled');
     }
 
     const autoFlushCallback = () => {
@@ -91,10 +96,10 @@ BufferedMetricsLogger.prototype.distribution = function(key, value, tags, timest
 BufferedMetricsLogger.prototype.flush = function(onSuccess, onError) {
     const series = this.aggregator.flush();
     if (series.length > 0) {
-        debug('Flushing %d metrics to Datadog', series.length);
-        this.reporter.report(series, onSuccess, onError);
+        logDebug('Flushing %d metrics to Datadog', series.length);
+        this.reporter.report(series, onSuccess, onError || this.onError);
     } else {
-        debug('Nothing to flush');
+        logDebug('Nothing to flush');
         if (typeof onSuccess === 'function') {
             onSuccess();
         }
diff --git a/lib/logging.js b/lib/logging.js
new file mode 100644
index 0000000..8a21f0e
--- /dev/null
+++ b/lib/logging.js
@@ -0,0 +1,33 @@
+'use strict';
+
+const util = require('util');
+const debug = require('debug');
+
+const prefix = 'metrics';
+
+/**
+ * A prefixed instance of the `debug` logger. You can call this directly, or
+ * call `extend()` on it to create a nested logger.
+ * @type {debug.Debugger}
+ */
+const logDebug = debug(prefix);
+
+/**
+ * Logs an error object or message to stderr. Unlike `logDebug()`, this will
+ * always print output, so should only be used for significant failures users
+ * *need* to know about.
+ * @param {string|Error} error The error to log.
+ */
+function logError(error, ...extra) {
+    if (typeof error === 'string') {
+        const message = util.format(error, ...extra);
+        console.error(`${prefix}: ERROR: ${message}`);
+    } else {
+        console.error(`${prefix}:`, error);
+    }
+}
+
+module.exports = {
+    logDebug,
+    logError
+};
diff --git a/lib/reporters.js b/lib/reporters.js
index 40f4927..3bcfc23 100644
--- a/lib/reporters.js
+++ b/lib/reporters.js
@@ -1,6 +1,6 @@
 'use strict';
-const debug = require('debug')('metrics');
 const datadogApiClient = require('@datadog/datadog-api-client');
+const { logDebug, logError } = require('./logging');
 
 //
 // NullReporter
@@ -51,10 +51,7 @@ function DataDogReporter(apiKey, appKey, apiHost) {
 }
 
 DataDogReporter.prototype.report = function(series, onSuccess, onError) {
-    if (debug.enabled) {
-        // Only call stringify when debugging.
-        debug('Calling report with %s', JSON.stringify(series));
-    }
+    logDebug('Calling report with %j', series);
 
     // Distributions must be submitted via a different method than other
     // metrics, so split them up.
@@ -84,15 +81,16 @@ DataDogReporter.prototype.report = function(series, onSuccess, onError) {
 
     Promise.all(submissions)
         .then(() => {
-            debug('sent metrics successfully');
+            logDebug('sent metrics successfully');
             if (typeof onSuccess === 'function') {
                 onSuccess();
             }
         })
         .catch((error) => {
-            debug('ERROR: failed to send metrics (err=%s)', error);
             if (typeof onError === 'function') {
                 onError(error);
+            } else {
+                logError('failed to send metrics (err=%s)', error);
             }
         });
 };


Test patch:
diff --git a/test/loggers_tests.js b/test/loggers_tests.js
index d27acb7..a6e56f8 100644
--- a/test/loggers_tests.js
+++ b/test/loggers_tests.js
@@ -13,8 +13,17 @@ const reporters = require('../lib/reporters');
 const BufferedMetricsLogger = loggers.BufferedMetricsLogger;
 
 describe('BufferedMetricsLogger', function() {
+    let errorLogs = [];
+    const originalError = console.error;
+
+    this.beforeEach(() => {
+        console.error = (...args) => errorLogs.push(args);
+    });
+
     this.afterEach(() => {
         nock.cleanAll();
+        console.error = originalError;
+        errorLogs = [];
     });
 
     it('should have a gauge() metric', function() {
@@ -172,6 +181,45 @@ describe('BufferedMetricsLogger', function() {
         );
     });
 
+    it('should support the `onError` option', function(done) {
+        nock('https://api.datadoghq.com')
+            .post('/api/v1/series')
+            .reply(403, { errors: ['Forbidden'] });
+
+        const logger = new BufferedMetricsLogger({
+            apiKey: 'not-valid',
+            onError (error) {
+                if (error) {
+                    done();
+                } else {
+                    done(new Error('Handler was called without error data'));
+                }
+            }
+        });
+        logger.gauge('test.gauge', 23);
+        logger.flush();
+    });
+
+    it('should log flush errors if there is no handler', function(done) {
+        nock('https://api.datadoghq.com')
+            .post('/api/v1/series')
+            .reply(403, { errors: ['Forbidden'] });
+
+        const logger = new BufferedMetricsLogger({ apiKey: 'not-valid' });
+        logger.gauge('test.gauge', 23);
+
+        logger.flush();
+        setTimeout(() => {
+            try {
+                errorLogs.should.have.lengthOf(1);
+            }
+            catch (error) {
+                return done(error);
+            }
+            done();
+        }, 50);
+    });
+
     it('should allow two instances to use different credentials', function(done) {
         const apiKeys = ['abc', 'xyz'];
         let receivedKeys = [];