wolph / python-statsd

Python Client for the Etsy NodeJS Statsd Server
http://readthedocs.org/docs/python-statsd/en/latest/
BSD 3-Clause "New" or "Revised" License
110 stars 39 forks source link

Allow sending multiple stats + minimize DNS lookups #21

Closed thisismiller closed 11 years ago

thisismiller commented 11 years ago

In wrapping the library for use in ChromiumOS's testing infrastructure, there were a couple small changes that are exceedingly hacky to maintain locally. Thus, here they are, I hope you find them agreeable. If you do, I'd much appreciate a pypi package bump afterwards.

wolph commented 11 years ago

Does this actually help for anything? Since UDP is a connectionless protocol I don't really understand what this is supposed to do... I can't really find any documentation about it either.

Wouldn't gethostbyname be a more logical solution here? http://docs.python.org/2/library/socket.html#socket.gethostbyname

thisismiller commented 11 years ago

Behavior of calling connect() on a UDP socket versus just sendto() is mentioned briefly in the related chapter in Beej's tutorial. The main part being that doing the connect() causes the gethostbyname() to be called internally (which issues the DNS lookup), and then result is saved and automatically used on every call to send() or recv(). I have verified my claims of when and how often DNS lookups are issued by running wireshark while running code against the unmodified, and then the modified versions of the code.

Additionally, calling connect() also enables one to be able to tell if the messages are making it or not, as if they don't, sometimes the remote host is friendly and will send back ICMP Port Unreachable messages. (Sometimes this is disabled, as having it enabled lets one do UDP port scanning.) You can read about this in some overly descriptive ICMP guide. Given that there's already the True/False being returned trying to indicate if the stat sending was successful, this seemed the slightly better way to go as it could let you get more information back as to if the packets are actually being received. Amusingly though, this is exactly what caused your travis failures, since there is no remote end that's responding, it seems.

If you'd prefer me to just do the gethostbyname() call in __init__() and use that as the argument to sendto(), that would still appease my need to avoid excessive amounts of DNS lookups, and I'd be happy to rework the code to do so.

wolph commented 11 years ago

Great, thank you for the very interesting explanation.

Unfortunately I cannot merge it yet though, Travis tells me that this patch breaks the test (most likely the tests need an update for your patch) so I'll have to look into that first.

thisismiller commented 11 years ago

No problem, and don't worry! I had just pointed nosetests at tests/, and forgot about the doctests in the source. I'll see if I can think up some clever way to get things working without axing the doctests, and push another another commit onto the branch, probably tomorrow. Thanks!

thisismiller commented 11 years ago

I've verified that nosetests statsd/ tests/ now passes, and this was the best way that I could think of to allow one to keep the doctests in counter.py, and allow for them to pass (and stop sending data on the wire). Unfortunately, nosetests's --doctest-fixtures argument only seems to apply if you're running doctests in a reStructuredText document, so I couldn't use that to hide the ugly setup/teardown code from sight (well, at least into another file). If you have any better ideas, please let me know.

That travis failure also doesn't seem related to my code, and seems doubly suspicious since only python2.7 failed with an error that isn't 2.7-specific. It looks like a timer unittest actually took a non-insignificant amount of time to run, so it logged 1ms instead of 0ms.

wolph commented 11 years ago

Nice, looks great :)

As for the Travis build failing... I guess I need to fix that test to be slightly more relaxed for the case that the Travis server is a bit busy.

Thanks for the patch!