Repository: brightcove/hot-shots (js)

Issue:
Pass key-value tags as objects
To my understanding, the StatsD implementations with tag support expect the tags to be key-value pairs. Hot-shots expects users to do the formatting, which is not ideal. Users must also do sanitation of tags which is even less ideal.

Instead, there could be automatic formatting of key-value tags to array

```javascript
statsd.increment("metric", 1, { "key": "value" });
```

should be the same as

```javascript
statsd.increment("metric", 1, ["key:value"]);
```

The implementation could be backwards compatible. If tags is passed as an array, no formatting would be performed.

[InfluxDB](https://www.influxdata.com/blog/getting-started-with-sending-statsd-metrics-to-telegraf-influxdb/) and [Datadog Statsd](http://docs.datadoghq.com/guides/dogstatsd/) use different format for tags. This annoying, but could be addressed by a configuration option.

I've created [a wrapper](https://github.com/lautis/hot-shots-deux) for hot-shots that implements the tagging for the Datadog format. Would you be interested in having this functionality in hot-shots? I'd be happy to make a PR out of this.

Pull request:
Pass key-value tags as objects
Implement passing key-value tags as objects. This reduces the chance of errors, and also allows implementing sanitation of tags.

The old format is still supported, and this is relying on the existing parsing and formatting functionality. If the array format would be removed, some of the parsing would become redundant and tags could be passed as an object until they are serialised as a string over the wire.

I'm not at all sure about the escaped characters: is the list complete and are there redundant characters? The documentation for this seems to be non-existent as even the reference implementations use arrays instead of objects.

Fixes #55.

Patch:
diff --git a/README.md b/README.md
index 902f870..cdf0fe2 100644
--- a/README.md
+++ b/README.md
@@ -30,7 +30,7 @@ Parameters (specified as an options hash):
 * `globalize`:   Expose this StatsD instance globally? `default: false`
 * `cacheDns`:    Cache the initial dns lookup to *host* `default: false`
 * `mock`:        Create a mock StatsD instance, sending no stats to the server? `default: false`
-* `globalTags`:  Tags that will be added to every metric `default: []`
+* `globalTags`:  Tags that will be added to every metric. Can be either an object or list of tags. `default: {}`
 * `maxBufferSize`: If larger than 0,  metrics will be buffered and only sent when the string length is greater than the size. `default: 0`
 * `bufferFlushInterval`: If buffering is in use, this is the time in ms to always flush any buffered metrics. `default: 1000`
 * `telegraf`:    Use Telegraf's StatsD line protocol, which is slightly different than the rest `default: false`
@@ -41,7 +41,7 @@ All StatsD methods other than event and close have the same API:
 * `name`:       Stat name `required`
 * `value`:      Stat value `required except in increment/decrement where it defaults to 1/-1 respectively`
 * `sampleRate`: Sends only a sample of data to StatsD `default: 1`
-* `tags`:       The Array of tags to add to metrics `default: []`
+* `tags`:       The tags to add to metrics. Can be either an object `{ tag: "value"}` or an array of tags. `default: []`
 * `callback`:   The callback to execute once the metric has been sent or buffered
 
 If an array is specified as the `name` parameter each item in that array will be sent along with the specified value.
@@ -61,7 +61,7 @@ The event method has the following API:
   * `priority`         Can be ‘normal’ or ‘low’ `default: normal`
   * `source_type_name` Assign a source type to the event.
   * `alert_type`       Can be ‘error’, ‘warning’, ‘info’ or ‘success’ `default: info`
-* `tags`:       The Array of tags to add to metrics `default: []`
+* `tags`:       The tags to add to metrics. Can be either an object `{ tag: "value"}` or an array of tags. `default: []`
 * `callback`:   The callback to execute once the metric has been sent.
 
 The check method has the following API:
@@ -72,7 +72,7 @@ The check method has the following API:
   * `date_happened`    Assign a timestamp to the check `default is now`
   * `hostname`         Assign a hostname to the check.
   * `message`          Assign a message to the check.
-* `tags`:       The Array of tags to add to metrics `default: []`
+* `tags`:       The tags to add to metrics. Can be either an object `{ tag: "value"}` or an array of tags. `default: []`
 * `callback`:   The callback to execute once the metric has been sent.
 
 ```javascript
@@ -132,19 +132,20 @@ The check method has the following API:
 
   // Sampling, tags and callback are optional and could be used in any combination (DataDog and Telegraf only)
   client.histogram('my_histogram', 42, 0.25); // 25% Sample Rate
-  client.histogram('my_histogram', 42, ['tag']); // User-defined tag
+  client.histogram('my_histogram', 42, { tag: 'value'}]); // User-defined tag
+  client.histogram('my_histogram', 42, ['tag:value']); // Tags as an array
   client.histogram('my_histogram', 42, next); // Callback
   client.histogram('my_histogram', 42, 0.25, ['tag']);
   client.histogram('my_histogram', 42, 0.25, next);
-  client.histogram('my_histogram', 42, ['tag'], next);
-  client.histogram('my_histogram', 42, 0.25, ['tag'], next);
+  client.histogram('my_histogram', 42, { tag: 'value'}, next);
+  client.histogram('my_histogram', 42, 0.25, { tag: 'value'}, next);
 
   // Use a child client to add more context to the client.
   // Clients can be nested.
   var childClient = client.childClient({
     prefix: 'additionalPrefix.',
     suffix: '.additionalSuffix',
-    globalTags: ['globalTag1:forAllMetricsFromChildClient']
+    globalTags: { globalTag1: 'forAllMetricsFromChildClient'}
   });
   childClient.increment('my_counter_with_more_tags');
 
@@ -159,7 +160,7 @@ The check method has the following API:
 
 Some of the functionality mentioned above is specific to DogStatsD or Telegraf.  They will not do anything if you are using the regular statsd client.
 * globalTags parameter- DogStatsD or Telegraf
-* tags parameter- DogStatsD or Telegraf
+* tags parameter- DogStatsD or Telegraf.
 * telegraf parameter- Telegraf
 * histogram method- DogStatsD or Telegraf
 * event method- DogStatsD
diff --git a/lib/statsd.js b/lib/statsd.js
index 6b41c15..932495b 100644
--- a/lib/statsd.js
+++ b/lib/statsd.js
@@ -52,7 +52,7 @@ var Client = function (host, port, prefix, suffix, globalize, cacheDns, mock,
   this.suffix      = options.suffix || '';
   this.socket      = options.isChild ? options.socket : dgram.createSocket('udp4');
   this.mock        = options.mock;
-  this.globalTags  = Array.isArray(options.globalTags) ? options.globalTags : [];
+  this.globalTags  = typeof options.globalTags === "object" ? formatTags(options.globalTags, options.telegraf) : [];
   this.telegraf    = options.telegraf || false;
   this.maxBufferSize = options.maxBufferSize || 0;
   this.sampleRate  = options.sampleRate || 1;
@@ -219,8 +219,8 @@ Client.prototype.check = function(name, status, options, tags, callback) {
   }
 
   var mergedTags = this.globalTags;
-  if (tags && Array.isArray(tags)) {
-    mergedTags = overrideTags(mergedTags, tags);
+  if (tags && typeof(tags) === "object") {
+    mergedTags = overrideTags(mergedTags, tags, this.telegraf);
   }
   if (mergedTags.length > 0) {
     check.push('#' + mergedTags.join(','));
@@ -335,7 +335,7 @@ Client.prototype.sendAll = function(stat, value, type, sampleRate, tags, callbac
     sampleRate = undefined;
   }
 
-  if(tags && !Array.isArray(tags)){
+  if(tags && typeof tags !== "object"){
     callback = tags;
     tags = undefined;
   }
@@ -410,8 +410,8 @@ Client.prototype.sendStat = function (stat, value, type, sampleRate, tags, callb
  */
 Client.prototype.send = function (message, tags, callback) {
   var mergedTags = this.globalTags;
-  if(tags && Array.isArray(tags)){
-    mergedTags = overrideTags(mergedTags, tags);
+  if(tags && typeof tags === "object"){
+    mergedTags = overrideTags(mergedTags, tags, this.telegraf);
   }
   if(mergedTags.length > 0){
     if (this.telegraf) {
@@ -576,8 +576,8 @@ var ChildClient = function (parent, options) {
     globalize   : false, // Only "root" client can be global
     mock        : parent.mock,
     // Append child's tags to parent's tags
-    globalTags  : Array.isArray(options.globalTags) ?
-        overrideTags(parent.globalTags, options.globalTags) : parent.globalTags,
+    globalTags  : typeof options.globalTags === "object" ?
+        overrideTags(parent.globalTags, options.globalTags, parent.telegraf) : parent.globalTags,
     maxBufferSize : parent.maxBufferSize,
     bufferFlushInterval: parent.bufferFlushInterval,
     telegraf    : parent.telegraf
@@ -596,14 +596,31 @@ Client.prototype.childClient = function(options) {
   return new ChildClient(this, options);
 };
 
+function sanitizeTags(value, telegraf) {
+  var blacklist = telegraf ? /:|\|/g : /:|\||@|,/g;
+  // Replace reserved chars with underscores.
+  return (value + "").replace(blacklist, "_");
+}
+
+function formatTags(tags, telegraf) {
+  if (Array.isArray(tags)) {
+    return tags;
+
+  } else {
+    return Object.keys(tags).map(function (key) {
+      return sanitizeTags(key, telegraf) + ":" + sanitizeTags(tags[key], telegraf);
+    });
+  }
+}
+
 /**
  * Overrides tags in parent with tags from child with the same name (case sensitive) and return the result as new
  * array. parent and child are not mutated.
  */
-function overrideTags (parent, child) {
+function overrideTags (parent, child, telegraf) {
   var childCopy = {};
   var toAppend = [];
-  child.forEach(function (tag) {
+  formatTags(child, telegraf).forEach(function (tag) {
     var idx = typeof tag === 'string' ? tag.indexOf(':') : -1;
     if (idx < 1) { // Not found or first character
       toAppend.push(tag);
diff --git a/types.d.ts b/types.d.ts
index 82a293b..27f50e9 100644
--- a/types.d.ts
+++ b/types.d.ts
@@ -1,12 +1,13 @@
 import dgram = require("dgram");
 
 declare module "hot-shots" {
+  export type Tags = { [key: string]: string } | string[];
   export interface ClientOptions {
     bufferFlushInterval?: number;
     bufferHolder?: { buffer: string };
     cacheDns?: boolean;
     errorHandler?: (err: Error) => void;
-    globalTags?: string[];
+    globalTags?: Tags;
     globalize?: boolean;
     host?: string;
     isChild?: boolean;
@@ -19,9 +20,9 @@ declare module "hot-shots" {
     suffix?: string;
     telegraf?: boolean;
   }
-  
+
   export interface ChildClientOptions {
-    globalTags?: string[];
+    globalTags?: Tags;
     prefix?: string;
     suffix?: string;
   }
@@ -59,28 +60,28 @@ declare module "hot-shots" {
   }
 
   export type StatsCb = (error: Error | undefined, bytes: any) => void;
-  export type StatsCall = (stat: string | string[], value: number, sampleRate?: number, tags?: string[], callback?: StatsCb) => void;
+  export type StatsCall = (stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb) => void;
 
   export class StatsD {
     constructor(options?: ClientOptions);
     childClient(options?: ChildClientOptions): StatsD;
-    
+
     increment(stat: string): void;
-    increment(stat: string | string[], value: number, sampleRate?: number, tags?: string[], callback?: StatsCb): void;
+    increment(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
 
     decrement(stat: string): void;
-    decrement(stat: string | string[], value: number, sampleRate?: number, tags?: string[], callback?: StatsCb): void;
+    decrement(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
 
-    timing(stat: string | string[], value: number, sampleRate?: number, tags?: string[], callback?: StatsCb): void;
-    histogram(stat: string | string[], value: number, sampleRate?: number, tags?: string[], callback?: StatsCb): void;
-    gauge(stat: string | string[], value: number, sampleRate?: number, tags?: string[], callback?: StatsCb): void;
-    set(stat: string | string[], value: number, sampleRate?: number, tags?: string[], callback?: StatsCb): void;
-    unique(stat: string | string[], value: number, sampleRate?: number, tags?: string[], callback?: StatsCb): void;
+    timing(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
+    histogram(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
+    gauge(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
+    set(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
+    unique(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
 
     close(callback: () => void): void;
 
-    event(title: string, text?: string, options?: EventOptions, tags?: string[], callback?: StatsCb): void;
-    check(name: string, status: DatadogChecksValues, options?: CheckOptions, tags?: string[], callback?: StatsCb): void;
+    event(title: string, text?: string, options?: EventOptions, tags?: Tags, callback?: StatsCb): void;
+    check(name: string, status: DatadogChecksValues, options?: CheckOptions, tags?: Tags, callback?: StatsCb): void;
 
     public CHECKS: DatadogChecks;
   }


Test patch:
diff --git a/test/test_statsd.js b/test/test_statsd.js
index daf7ee1..ad9d2c6 100644
--- a/test/test_statsd.js
+++ b/test/test_statsd.js
@@ -397,6 +397,40 @@ function doTests(StatsD) {
       });
     });
 
+    it('should format global tags', function(finished){
+      udpTest(function(message, server){
+        assert.equal(message, 'test:1337|c|#gtag:234,foo:bar');
+        server.close();
+        finished();
+      }, function(server){
+        var address = server.address(),
+            statsd = new StatsD({
+              host: address.address,
+              port: address.port,
+              globalTags: { gtag: "123", foo: "bar"}
+            });
+
+        statsd.increment('test', 1337, { gtag: "234"});
+      });
+    });
+
+    it('should replace reserved characters with underscores in tags', function(finished){
+      udpTest(function(message, server){
+        assert.equal(message, 'test:1337|c|#foo:b_a_r,reserved_character:is_replaced_');
+        server.close();
+        finished();
+      }, function(server){
+        var address = server.address(),
+            statsd = new StatsD({
+              host: address.address,
+              port: address.port,
+              globalTags: { foo: "b,a,r"}
+            });
+
+        statsd.increment('test', 1337, { "reserved:character": "is@replaced@"});
+      });
+    });
+
     it('should add global tags using telegraf format when enabled', function(finished){
       udpTest(function(message, server){
         assert.equal(message, 'test,gtag=gvalue,gtag2=gvalue2:1|c');
@@ -433,6 +467,24 @@ function doTests(StatsD) {
       });
     });
 
+    it('should format global key-value tags using telegraf format when enabled', function(finished){
+      udpTest(function(message, server){
+        assert.equal(message, 'test,gtag=gvalue,foo=bar:1337|c');
+        server.close();
+        finished();
+      }, function(server){
+        var address = server.address(),
+            statsd = new StatsD({
+              host: address.address,
+              port: address.port,
+              globalTags: { gtag: "gvalue"},
+              telegraf: true
+            });
+
+        statsd.increment('test', 1337, { foo: "bar" });
+      });
+    });
+
   });
 
   describe('#timing', function(){
@@ -508,6 +560,37 @@ function doTests(StatsD) {
     it('should send no timing stat when a mock Client is used', function(finished){
       assertMockClientMethod('timing', finished);
     });
+
+    it('should format tags to datadog format by default', function(finished){
+      udpTest(function(message, server){
+        assert.equal(message, 'test:42|ms|#foo:bar');
+        server.close();
+        finished();
+      }, function(server){
+        var address = server.address(),
+            statsd = new StatsD(address.address, address.port);
+
+        statsd.timing('test', 42, {foo: 'bar'});
+      });
+    });
+
+    it('should format tags when using telegraf format', function(finished){
+      udpTest(function(message, server){
+        assert.equal(message, 'test,foo=bar:42|ms');
+        server.close();
+        finished();
+      }, function(server){
+        var address = server.address(),
+            statsd = new StatsD({
+              address: address.address,
+              port: address.port,
+              telegraf: true
+            });
+
+        statsd.timing('test', 42, { foo: 'bar'});
+      });
+    });
+
   });
 
   describe('#histogram', function(){