mikejihbe / metrics

A metrics library for Node.js
574 stars 58 forks source link

Add support for tags in metrics #42

Open lockwobr opened 7 years ago

lockwobr commented 7 years ago

Some backend systems like influxdb, datadog, and opentsdb to name a few have support of tags. It would be nice if the library has support of tags built into it. Having tags is super helpful when using query tools like grafana to view you metrics data. I might be coding this out some time in the days to come, would this be a welcome addition to the library? Or does anyone know if this is being worked on or exists somewhere else?

tolbertam commented 7 years ago

I think this is a good idea that I hadn't considered since I primarily use graphite to store my metrics which doesn't have this capability as far as I know, 👍 . The challenging thing will be how to expose this, at the moment really the only metadata tied to the metric is its name when it's registered with a Report object via addMetric. I know that dropwizard metrics has a similar issue with MetricRegistry. I see that metrics-datadog gets around this by overloading the metric name, i.e. metricName[tagName:tagValue,tagName:tagValue,...], although I'm not really a fan of that approach.

How about something like:

Report.prototype.addMetric = function(eventName, metric, tags)

Where tags is an optional argument that is an object with property names that are interpreted as the tag name with values interpreted as the value.

These tags could then be made available in ScheduledReporter by adding a tags property to the metrics that are shared via ScheduledReporter.prototype.getMetrics (like we add a name property here.

What do you think?

It would also be nice to have a provided importer that utilizes this if you'd like to give that a go as well :).

seprich commented 7 years ago

A simpler approach that I found sufficient for my use case with telegraf-influxdb-grafana stack was to tie tags to the reporter:

class TelegrafUDPReporter extends ScheduledReporter {
  constructor(registry, host, port, tags) {
    super(registry)
    this.host = host
    this.port = port
    this.tags = tags
    this.client = new TelegrafUDPClient({host, port})  // from telegrafjs  library
    this.connected = false
  }
// .... then tying it up with:
  send(metricName, measurements) {
    this.client.sendMeasurement(new Measurement(metricName, this.tags, measurements))
  }

this doesn't give you a per-metric-instance tags , only a per-reporter set of default tags where you can pass on useful information such as os.hostname() etc. for differentiating data when multiple metric collection instances produce metrics data to a single backend DB.

I think this library could directly provide TelegrafUPD and TCP reporters with reporter tied tags and this solution would not exclude the possibility of implementing a per-metric (or even per-measurement but thats excessive) tags also. If ever several levels of tags existed they could be merged into one final set of tags with the more specific level taking priority over the more generic level.

lockwobr commented 7 years ago

The per metric is what I need, so I add one metric to a middleware and make the route a tag and u can group by route all in one query and get counts for all routes at once and still get the total without the group by and like wise have status code as a tag and do the same thing.

f0ster commented 7 years ago

I've added support for this here, https://github.com/f0ster/go-metrics-influxdb . Just by stuffing the data into the name field into the metrics interface,then unwrapping the tags before sending to influx