Fixing the Librato output-plugin #722
Conversation
|
@jipperinbham Did you ever manage to get this to work? I've used this branch to successfully send metrics to Librato (one of my clients uses Librato). |
|
you need to update the unit tests |
|
|
||
| ### Build metric names from tag-values | ||
| ### Uses the measurement, tag-values and field-names to derive the Librato metric name. | ||
| # name_from_tags = false |
There was a problem hiding this comment.
why supply this as an option? why not always build the name from tags?
There was a problem hiding this comment.
I mainly didn't want to scare people with huge metric names in Librato!
|
@sparrc OK I got the tests working (had to downgrade to GO 1.4.3 to get around the "internal package" thing. |
|
In that case there might be something wrong with your GOPATH, you should be allowed to import the internal package within influxdata/telegraf. if you paste the output of |
|
or it might also be that your source code is in $GOPATH/src/github.com/influxdb/telegraf rather than $GOPATH/src/github.com/influxdata/telegraf |
| tagValues = append(tagValues, tagValue) | ||
| } | ||
| } | ||
| gaugeName = strings.Replace(fmt.Sprintf("%v.%v.%v", m.Name(), strings.Join(tagValues, "."), fieldName), "/", "-", -1) |
There was a problem hiding this comment.
This seems like it could be non-deterministic for the ordering of the tags.
There was a problem hiding this comment.
I could always sort them.
|
I'm not super familiar with librato, so correct me if I'm wrong, but I think that the measurement names are the same as on graphite, correct? (ie, dot buckets like For this, I think you should use the graphite serializer: https://github.com/influxdata/telegraf/blob/master/plugins/serializers/graphite/graphite.go to get the graphite bucket name. With your approach, there is an issue where you will get overwritten metrics when not using tags within the bucket. For example, these two separate influx line-protocol metrics: would both be translated into the same graphite dot-bucket: but if you include the tags, they will be separated: |
|
PS: feel free to make a new public function on the graphite serializer just for getting the bucket name(s) |
|
Actually yes, that's why I added the option to "build names from tags" because in its original state, this plugin would have behaved exactly as you suggest. I'll check out the graphite-serialiser and try to use that. |
|
even if the metric names get large, I'd rather not allow people to be sending metrics that get overwritten (people will undoubtedly use this option and then not understand why their metrics are off) Soon the graphite serializer should be able to support templates that allow for users to specify which tags they want in their output. |
|
I've removed that option completely, and switched to the graphite serializer. |
|
Thanks, can you remove the code duplication in serializers/graphite? Also please add unit tests for the new function, SerializeBucketName |
|
@sparrc this seems to work now |
|
@chrusty the initial implementation worked with the account I had created to test with but many things have changed in telegraf since (for the better) and it's also possible the librato API changed. thanks for getting this fixed up |
|
@jipperinbham Hey man, no problem |
|
@chrusty Thanks for this fix. I'm having problems rebasing this though, can you squash your commits and rebase with master? |
|
@sparrc OK, I think it should work now. That WAS a whole lot of commits. |
|
thanks @chrusty |
|
@sparrc hey are you on any chats (Slack, IRC etc)? I've got a couple of other PRs in the pipeline |
|
@chrusty Yes, you can find me in the Gophers slack room under username "cameronsparr" |
The Librato plugin simply didn't work, was never going to work in its current state.
I've done enough here to get it up and running, and also make it work with tagged metrics (by joining them with dots, as seems to be the way Librato would expect).