Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for tags in metrics #42

Open
lockwobr opened this issue Feb 9, 2017 · 4 comments
Open

Add support for tags in metrics #42

lockwobr opened this issue Feb 9, 2017 · 4 comments

Comments

@lockwobr
Copy link

lockwobr commented Feb 9, 2017

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
Copy link
Collaborator

tolbertam commented Feb 23, 2017

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
Copy link

seprich commented Mar 15, 2017

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
Copy link
Author

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
Copy link

f0ster commented May 17, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants