Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

Add ability to aggregate packets for sending #60

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

Conversation

jjofseattle
Copy link

This ports the functionality from https://github.com/msiebuhr/node-statsd-client for aggregated packets.

It shows great performance improvement. I ran perfTest/test.js to measure the time required of sending out 100,000 packets. A buffer size of 1200 is 5 times faster than no buffering.
==> node test.js 0
2143
2126
2186
2281
2487
1941
3006
2026
2909
2042
2049
^C

==> node test.js 1200
395
389
385
388
393
389
390
388
389
391
388
389
^C

==> node test.js 200
757
794
763
814
772
799
782
779
785
815
^C

@allenarthurgay
Copy link

g2g

* @param message {String}
*/
Client.prototype.enqueue = function(message){
this.buffer += message + "\n";
Copy link

Choose a reason for hiding this comment

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

Wouldn't it be better to have an array and loop on each entry on flush? I don't really the like the string thing with new lines... here's an example with a PHP implementation: https://github.com/beberlei/metrics/blob/master/src/Beberlei/Metrics/Collector/StatsD.php#L95-L107

Copy link
Author

Choose a reason for hiding this comment

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

That would defeat the purpose. We aggregate packets here to reduce the number of calls to socket.send, which is an expensive call.

@bdeitte bdeitte mentioned this pull request Dec 19, 2015
@jlutzhbo
Copy link

jlutzhbo commented Oct 6, 2016

@devdazed @eexit Can we get a signoff on this PR please and get this merged in? Thanks!

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

Successfully merging this pull request may close these issues.

5 participants