Repository: dbader/node-datadog-metrics
Pull request #100 (2022-09-26 21:26:00)

Rename `apiHost` → `site` and `DataDogReporter` → `DatadogReporter`
In some other spots, I’ve noted that it might be good to rename `apiHost` to `site` so that it matches the Datadog documentation and official libraries, which all call this concept the `site`. This does so by adding a new `site` option and *deprecating* `apiHost` (so it still works, but prints a warning message) and having it set `site` under the hood.

While I was at it, I followed the lead of #59 and #98 and renamed `DataDogReporter` to `DatadogReporter` (no capital "D" in the middle). I didn’t do a deprecation here since there hasn’t yet been a release where it’s publicly visible. But we could do a deprecation instead if we want.

~**This also builds on some of the tooling in #99,** so if we want to land this, it needs to be done a little more carefully so everything merges correctly.~ *(Edit: updated to merge cleanly.)*

@ErikBoesen what do you think of these changes? I think the first one is a good change that makes things easier to understand for new users, although neither one is really *necessary*.

More broadly, I’ve been holding off on implementing the breaking changes (#83 and part of #84 both involve subtle breakage) in case you want to cut a non-breaking 0.10.2 release first. The deprecation stuff here probably isn’t useful if you aren’t interested in that.

Mr0grog: > I’ve been holding off on implementing the breaking changes (#83 and part of #84 both involve subtle breakage) in case you want to cut a non-breaking 0.10.2 release first.

I’m going to see about doing #85 tomorrow, but will probably make that the last change for now until I hear more feedback on this question of breaking changes and release plans.
Mr0grog: Rebased this to resolve merge conflicts. We wound up shipping a release that publicly exposes `DataDogReporter` in the mean time, so this needs to either not rename it, or *deprecate* it instead of removing the old name.

I think the straightforward way to deprecate is with a subclass:

```js
class DataDogReporter extends DatadogReporter {
    constructor(apiKey, appKey, site) {
        logDeprecation(
            'DataDogReporter has been renamed to DatadogReporter ' +
            '(lower-case D in "dog"); the old name will be removed in a ' +
            'future release.'
        );
        super();
    }
}
```

On the other hand, I’m not sure this really minor change is worthwhile in the way that `apiHost` → `site` is, where we are making the name line up with all kinds of documentation from Datadog.
Mr0grog: @ErikBoesen did you have any thoughts on this? In particular, whether to rename `DataDogReporter`?

If not, I’m going to figure out how much I want to merge this and see about pushing out a v0.11.0 release later in the week (so the built-in type annotations are available, hopefully solving things like #109).
ErikBoesen: Frankly I trust your judgment on this, considering I don't actually use this package myself anymore.
Mr0grog: 👍 That's helpful feedback!

Patch:
diff --git a/README.md b/README.md
index 1b7318c..182c6c6 100644
--- a/README.md
+++ b/README.md
@@ -88,7 +88,7 @@ If you need even more control you can create one or more `BufferedMetricsLogger`
 ```js
 var metrics = require('datadog-metrics');
 var metricsLogger = new metrics.BufferedMetricsLogger({
-    apiHost: 'datadoghq.eu',
+    site: 'datadoghq.eu',
     apiKey: 'TESTKEY',
     host: 'myhost',
     prefix: 'myapp.',
@@ -117,8 +117,7 @@ Where `options` is an object and can contain the following:
 * `flushIntervalSeconds`: How often to send metrics to Datadog. (optional)
     * This defaults to 15 seconds. Set it to 0 to disable auto-flushing which
       means you must call `flush()` manually.
-* `apiHost`: Sets the Datadog API host (also called "site" in Datadog docs).
-    (optional)
+* `site`: Sets the Datadog "site", or server where metrics are sent. (optional)
     * Defaults to `datadoghq.com`.
     * See more details on setting your site at:
         https://docs.datadoghq.com/getting_started/site/#access-the-datadog-site
@@ -141,7 +140,7 @@ Where `options` is an object and can contain the following:
     (optional)
 * `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
+        1. `reporters.DatadogReporter` sends metrics to Datadog’s API, and is
            the default.
         2. `reporters.NullReporter` throws the metrics away. It’s useful for
            tests or temporarily disabling your metrics.
@@ -283,6 +282,10 @@ npm test
 
     * **Breaking change:** datadog-metrics now uses modern `class` syntax internally. In most cases, you shouldn’t need to change anything. However, if you are calling `BufferedMetricsLogger.apply(...)` or `BufferedMetricsLogger.call(...)`, you’ll need to change your code to use `new BufferedMetricsLogger(...)` instead.
 
+    * **Deprecated:** The `apiHost` option has been renamed to `site` so that it matches up with Datadog docs and official packages. The old `apiHost` name still works for now, but will be removed in the future.
+
+    * **Deprecated:** The `reporters.DataDogReporter` class has been renamed to `reporters.DatadogReporter` (lower-case D in "dog") so that it correctly matches Datadog’s actual name. The old name still works, but prints a warning when it is used and will be removed in the future.
+
     * Built-in TypeScript definitions. If you use TypeScript, you no longer need to install separate type definitions from `@types/datadog-metrics` — they’re now built-in. Please make sure to remove `@types/datadog-metrics` from your dev dependencies.
 
         Even if you’re writing regular JavaScript, you should now see better autocomplete suggestions and documentation in editors that support TypeScript definitions (e.g. VisualStudio Code, WebStorm).
diff --git a/lib/loggers.js b/lib/loggers.js
index 4141580..78120cb 100644
--- a/lib/loggers.js
+++ b/lib/loggers.js
@@ -1,7 +1,7 @@
 'use strict';
-const { logDebug } = require('./logging');
+const { logDebug, logDeprecation } = require('./logging');
 const Aggregator = require('./aggregators').Aggregator;
-const DataDogReporter = require('./reporters').DataDogReporter;
+const { DatadogReporter } = require('./reporters');
 const Gauge = require('./metrics').Gauge;
 const Counter = require('./metrics').Counter;
 const Histogram = require('./metrics').Histogram;
@@ -34,9 +34,10 @@ const Distribution = require('./metrics').Distribution;
  * @property {string} [appKey] Datadog APP key
  * @property {string} [host] Default host for all reported metrics
  * @property {string} [prefix] Default key prefix for all metrics
- * @property {string} [apiHost] Sets the Datadog "site", or server where metrics
+ * @property {string} [site] Sets the Datadog "site", or server where metrics
  *           are sent. For details and options, see:
  *           https://docs.datadoghq.com/getting_started/site/#access-the-datadog-site
+ * @property {string} [apiHost] DEPRECATED: Please use `site` instead.
  * @property {number} [flushIntervalSeconds] How often to send metrics to
  *           Datadog (in seconds).
  * @property {string[]} [defaultTags] Default tags used for all metrics.
@@ -74,10 +75,19 @@ class BufferedMetricsLogger {
      * @param {BufferedMetricsLoggerOptions} [opts]
      */
     constructor (opts) {
+        if (opts.apiHost) {
+            logDeprecation(
+                'The `apiHost` option for `init()` and `BufferedMetricsLogger` ' +
+                'has been deprecated and will be removed in a future release. ' +
+                'Please use the `site` option instead.'
+            );
+            opts.site = opts.apiHost;
+        }
+
         /** @private */
         this.aggregator = opts.aggregator || new Aggregator(opts.defaultTags);
         /** @private */
-        this.reporter = opts.reporter || new DataDogReporter(opts.apiKey, opts.appKey, opts.apiHost);
+        this.reporter = opts.reporter || new DatadogReporter(opts.apiKey, opts.appKey, opts.site);
         /** @private */
         this.host = opts.host;
         /** @private */
diff --git a/lib/logging.js b/lib/logging.js
index 8a21f0e..1e87e23 100644
--- a/lib/logging.js
+++ b/lib/logging.js
@@ -4,6 +4,7 @@ const util = require('util');
 const debug = require('debug');
 
 const prefix = 'metrics';
+const deprecationMessages = new Set();
 
 /**
  * A prefixed instance of the `debug` logger. You can call this directly, or
@@ -27,7 +28,21 @@ function logError(error, ...extra) {
     }
 }
 
+/**
+ * Logs a deprecation warning to stderr once. If this is called multiple times
+ * with the same message, it will only log once.
+ * @param {string} message Deprecation message to log.
+ */
+function logDeprecation(message) {
+    if (!deprecationMessages.has(message)) {
+        // We always want to log here, so don't use `logDebug`.
+        console.warn(`${prefix}: ${message}`);
+        deprecationMessages.add(message);
+    }
+}
+
 module.exports = {
     logDebug,
+    logDeprecation,
     logError
 };
diff --git a/lib/reporters.js b/lib/reporters.js
index 7e4d9d0..9dc3590 100644
--- a/lib/reporters.js
+++ b/lib/reporters.js
@@ -1,6 +1,6 @@
 'use strict';
 const datadogApiClient = require('@datadog/datadog-api-client');
-const { logDebug, logError } = require('./logging');
+const { logDebug, logDeprecation, logError } = require('./logging');
 
 /**
  * A Reporter that throws away metrics instead of sending them to Datadog. This
@@ -20,17 +20,17 @@ const datadogClients = new WeakMap();
 /**
  * Create a reporter that sends metrics to Datadog's API.
  */
-class DataDogReporter {
+class DatadogReporter {
     /**
      * Create a reporter that sends metrics to Datadog's API.
      * @param {string} [apiKey]
      * @param {string} [appKey]
-     * @param {string} [apiHost]
+     * @param {string} [site]
      */
-    constructor(apiKey, appKey, apiHost) {
+    constructor(apiKey, appKey, site) {
         apiKey = apiKey || process.env.DATADOG_API_KEY;
         appKey = appKey || process.env.DATADOG_APP_KEY;
-        this.apiHost = apiHost || process.env.DATADOG_API_HOST;
+        this.site = site || process.env.DATADOG_API_HOST;
 
         if (!apiKey) {
             throw new Error('DATADOG_API_KEY environment variable not set');
@@ -42,13 +42,13 @@ class DataDogReporter {
                 appKeyAuth: appKey
             }
         });
-        if (this.apiHost) {
+        if (this.site) {
             // Strip leading `app.` from the site in case someone copy/pasted the
             // URL from their web browser. More details on correct configuration:
             // https://docs.datadoghq.com/getting_started/site/#access-the-datadog-site
-            this.apiHost = this.apiHost.replace(/^app\./i, '');
+            this.site = this.site.replace(/^app\./i, '');
             datadogApiClient.client.setServerVariables(configuration, {
-                site: this.apiHost
+                site: this.site
             });
         }
         datadogClients.set(this, new datadogApiClient.v1.MetricsApi(configuration));
@@ -100,7 +100,28 @@ class DataDogReporter {
     }
 }
 
+/**
+ * @deprecated Please use `DatadogReporter` instead.
+ */
+class DataDogReporter extends DatadogReporter {
+    /**
+     * Create a reporter that sends metrics to Datadog's API.
+     * @deprecated
+     * @param {string} [apiKey]
+     * @param {string} [appKey]
+     * @param {string} [site]
+     */
+    constructor(apiKey, appKey, site) {
+        logDeprecation(
+            'DataDogReporter has been renamed to DatadogReporter (lower-case ' +
+            'D in "dog"); the old name will be removed in a future release.'
+        );
+        super(apiKey, appKey, site);
+    }
+}
+
 module.exports = {
     NullReporter,
+    DatadogReporter,
     DataDogReporter
 };


Test patch:
diff --git a/test/lint_text.sh b/test/lint_text.sh
index 08e8550..f7e9e3d 100755
--- a/test/lint_text.sh
+++ b/test/lint_text.sh
@@ -28,7 +28,7 @@ bad_lines=$(
 
 if [ -n "${bad_lines}" ]; then
     echo 'The correct spelling of "Datadog" does not capitalize the second "D".'
-    echo 'Please fix these lines ("DataDogReporter" is allowed):'
+    echo 'Please fix these lines ("DataDogReporter" is allowed for now):'
     echo ''
     echo "${bad_lines}"
     echo ''
diff --git a/test/loggers_tests.js b/test/loggers_tests.js
index 33b7b12..7c33e24 100644
--- a/test/loggers_tests.js
+++ b/test/loggers_tests.js
@@ -11,16 +11,21 @@ const reporters = require('../lib/reporters');
 const BufferedMetricsLogger = loggers.BufferedMetricsLogger;
 
 describe('BufferedMetricsLogger', function() {
+    let warnLogs = [];
     let errorLogs = [];
+    const originalWarn = console.warn;
     const originalError = console.error;
 
     this.beforeEach(() => {
+        console.warn = (...args) => warnLogs.push(args);
         console.error = (...args) => errorLogs.push(args);
     });
 
     this.afterEach(() => {
         nock.cleanAll();
+        console.warn = originalWarn;
         console.error = originalError;
+        warnLogs = [];
         errorLogs = [];
     });
 
@@ -167,20 +172,31 @@ describe('BufferedMetricsLogger', function() {
         l.aggregator.defaultTags.should.deep.equal(['one', 'two']);
     });
 
-    it('should allow setting apiHost/site', function() {
+    it('should allow setting site', function() {
         const l = new BufferedMetricsLogger({
             apiKey: 'abc123',
-            apiHost: 'datadoghq.eu'
+            site: 'datadoghq.eu'
+        });
+        l.reporter.should.have.property('site', 'datadoghq.eu');
+    });
+
+    it('should allow setting site with "app.*" URLs', function() {
+        const l = new BufferedMetricsLogger({
+            apiKey: 'abc123',
+            site: 'app.datadoghq.eu'
         });
-        l.reporter.should.have.property('apiHost', 'datadoghq.eu');
+        l.reporter.should.have.property('site', 'datadoghq.eu');
     });
 
-    it('should allow setting apiHost/site with "app.*" URLs', function() {
+    it('should allow deprecated `apiHost` option', function() {
         const l = new BufferedMetricsLogger({
             apiKey: 'abc123',
-            apiHost: 'app.datadoghq.eu'
+            apiHost: 'datadoghq.eu'
         });
-        l.reporter.should.have.property('apiHost', 'datadoghq.eu');
+        l.reporter.should.have.property('site', 'datadoghq.eu');
+
+        const apiHostWarnings = warnLogs.filter(x => x[0].includes('apiHost'));
+        apiHostWarnings.should.have.lengthOf(1);
     });
 
     it('should call the flush success handler after flushing', function(done) {
diff --git a/test/module_tests.js b/test/module_tests.js
index b2b8552..720aa48 100644
--- a/test/module_tests.js
+++ b/test/module_tests.js
@@ -77,7 +77,7 @@ describe('datadog-metrics', function() {
     });
 
     it('should publicly export built-in reporters', function() {
-        metrics.reporters.should.have.property('DataDogReporter', reporters.DataDogReporter);
+        metrics.reporters.should.have.property('DatadogReporter', reporters.DatadogReporter);
         metrics.reporters.should.have.property('NullReporter', reporters.NullReporter);
     });
 });