Skip to content
This repository has been archived by the owner on Dec 8, 2023. It is now read-only.

parse metric name and tags with histograms support #81

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mugurax
Copy link

@mugurax mugurax commented Apr 21, 2017

When sending timers with histograms, statsd creates new gauges with .bin_x added to the original metric name.

For instance: global-prefix.name#tag=val is sent to statsd-librato-backend as global-prefix.name#tag=val.bin_50

This format is not supported by the existing parseAndSetTags.

Thanks,
Mugur

@bryanmikaelian
Copy link
Contributor

Thanks for the report / fix + test! I'll check on this tomorrow morning (I'm CST)

@bryanmikaelian bryanmikaelian added this to the 2.x.x: Tagged Measurements milestone Apr 21, 2017
Copy link
Contributor

@bryanmikaelian bryanmikaelian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to move your logic for parsing the bin_ suffix out to here https://github.com/librato/statsd-librato-backend/blob/master/lib/librato.js#L377? I would prefer to keep parseAndSetTags focused on just parsing tags out and keep timer-specific logic contained inside here: https://github.com/librato/statsd-librato-backend/blob/master/lib/librato.js#L343

For timers and their suffixes (p99, etc), we handle similar logic with tags inside the function called here https://github.com/librato/statsd-librato-backend/blob/master/lib/librato.js#L365.

Other than that, looks great!

@bryanmikaelian
Copy link
Contributor

cc @jderrett

@@ -62,6 +62,22 @@ module.exports = {
this.emitter.emit('flush', 123, metrics);
},

testValidMeasurementSingleTagFromHistogram: function(test) {
test.expect(4);
let metrics = {gauges: {'my_gauge#foo=bar.bin_50': 1}};
Copy link
Contributor

@bryanmikaelian bryanmikaelian Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good test but we probably want to build a proper timer metric to fully test this code path: https://github.com/librato/statsd-librato-backend/blob/master/lib/librato.js#L373.

Something like https://github.com/librato/statsd-librato-backend/blob/master/test/librato_tests.js#L85-L94 but with a histogram property

@mugurax
Copy link
Author

mugurax commented Apr 21, 2017

Regarding these commits:

@mugurax disabled debug override 94da786
@mugurax disabled debug override 4dcd2d6

They were not intended to reach this pull request :( I am not sure how should I proceed. Sorry for the trouble.

@bryanmikaelian
Copy link
Contributor

@mugurax No worries! You can make the change on your branch and update the PR.

@bryanmikaelian
Copy link
Contributor

Any updates?

@johanneswuerbach
Copy link
Contributor

@bryanmikaelian any update here? I would happily take this over otherwise.

@bryanmikaelian
Copy link
Contributor

cc @mbeale . You ok with @johanneswuerbach taking this over? I can try to get it knocked out but no ETA. Is this something Librato still wants to support?

@mbeale
Copy link
Contributor

mbeale commented Aug 22, 2018

@bryanmikaelian probably ok for @johanneswuerbach to take over. I'm not sure if Librato will release another version.

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

Successfully merging this pull request may close these issues.

4 participants