Support for "condensed" statsd protocol.

[drawks]

In addition to the standard single update string protocol, the etsy (reference) implementation of stats supports appending multiple timer updates for a single metric as well as multiple different types of updates for a single key to be combined:

I can't find official documentation for this optimization, but you can see a proxy which implements pre-processing datagrams using this technique at https://github.com/juliusv/ne-statsd-backend and sure enough if you test the example updates against the etsy statsd they all work as described.

This can provide a nice bump in wire efficiency for some use patterns:

Here are few inline examples in case the above url goes dead:

foo:8|ms
foo:5|ms
foo:3|ms

becomes:

foo:8|ms:5|ms:3|ms

which is 33% smaller in this case and could easily be much more efficient in the case of a longer metric name (which would be typical) or a metric with a very high update frequency.

or

foo:1|c
foo:3|c
foo:5|ms
foo:3|ms
foo:6|g
foo:7|g

becomes:

foo:4|c:5|ms:3|ms:7|g

which is something like 50% smaller with similar notes as above WRT metric name size and update frequency.

[armon]

Interesting. At this point, this is not something I have time to tackle. The current ASCI format parser has been fine tuned for it's current performance, so doing a casual attempt at this would likely result in a dramatic loss of parsing performance.

[sleepybishop]

i replaced the manual parser with a ragel based one without a drop in performance at sleepybishop/statsite@eeca0c0

i then implemented condensed protocol at sleepybishop/statsite@d6e82a7

there seems to be a ~3-5% drop in performance in my benchmarks. i wouldn't call it dramatic but at the same time i don't see the wide use case for the condensed protocol.

YMMV depending on you machine specs.

[drawks]

Woh! Good stuff 👏

[shamil]

Will this be implemented/merged? I see some statsd client libraries are using explicity this format. Like kamon-statsd.

[sleepybishop]

doubtful, its a fairly intrusive change that would affect all users regardless of whether they need the feature or not. i do make an effort to keep the ragel_parser branch up to date though.

[drawks]

It's sort of disappointing that adopting a protocol compliant parser which, by all appearances, causes only a very small performance regression in favor of much more easy to read/understand/maintain parser code is seen as an "intrusive change" and is unlikely to be adopted. I would expect that maintaining compatibility with the protocol as defined in the reference implementation would be a higher priority than a small performance regression which is unlikely to impact most users.

[sleepybishop]

ill preface this by saying it's ultimately not my call as to whether or not it gets merged, i was only aiming to be helpful when i wrote the patch. the code modified had been stable for years, apologies if the "intrusive" label does not seem accurate.

i dont think armon is against merging deep changes as he was quite understanding when i replaced the event system with the ae loop and im quite open to making any changes needed to make it more palatable for merge consideration, but there are a lot of users for which performance is king which is probably why they chose statsite in the first place, negatively impacting those users doesn't seem like the right thing to do.

[shamil]

@sleepybishop , maybe we should have compile time option, like --with-ragel-parser. So we can decide which parser to use and everybody will stay happy :)

[sleepybishop]

fair enough, ill work on a pr to do some refactoring to make a parser change more straightforward and see if we can get that merged in first.

[drawks]

Happy birthday to this issue! @sleepybishop any chance you've worked on this at all?

[sleepybishop]

ive worked on it some and ive kept my branch up to date but i have not made much progress beyond that. apologies.