search

lukevenediger / statsd-csharp-client

A simple c# client library for statsd and statsd.net
MIT License
47 stars 20 forks source link

Bad performance of UDP client #5

Closed majkinetor closed 8 years ago

majkinetor commented 11 years ago

You should change UDP client so it cashes IP address instead obtaining it each time. The lag is enormous in web apps. Furthermore, if one already gives IP address there is no need to check for IP address. Next, If something is wrong with server name, UDP channel should be substituted with null channel.

The statsd client should first perform well in order not to block application, then have good interface.

lukevenediger commented 11 years ago

Thanks, these are good suggestions. I'm about to do another release so I'll fix these quickly.

In terms of the IP address resolution, I do this once in the constructor. This is probably the best place to cache the result, and means that it should only really happen on application start-up.

lukevenediger commented 11 years ago

Next, If something is wrong with server name, UDP channel should be substituted with null channel

Can you describe the specific scenario when this happened? I've just run a test now where I create a new instance with a bad hostname and it falls back to the null output channel. Maybe I'm missing something?

majkinetor commented 11 years ago

It depends on implementation. How will you handle non existent servers (server down, changed IP etc.). If the server doesn't exist the throw will gets into effect in UDPChannel and the calling class will return nullchannel. Now, if you do this only once on start of the app, OK, but if you recreate client often (on request for example) you are doing this each time (since you didn't remember that server is invalid. However, this is expensive if you have multiple threads (throwing is expensive). So null channel should be returned once you recognize this. Also the server may be up and down during life time of app or be invalid forever at the start.

In any case this shouldn't influence app performance at all. Some caching and periodic probing might be good idea.

izevaka commented 10 years ago

It's pretty easy to repro actually, connect to a functioning statsd, send a few metrics, then either turn off statsd or disconnect from network. Next metric would cause a SocketException.

I had to look at the code but the client is not actually thread safe. Underlying UdpClient methods are not thread safe according to MSDN.

In light of that I would recommend to implement connection pooling rather than DNS caching. Setting up a new socket per web request is expensive-ish, both on client and server.

niemyjski commented 9 years ago

+1 we are also seeing socket exceptions

ejsmith commented 9 years ago

Yep, I am using this library in a couple apps and it will occasionally blow up.

niemyjski commented 9 years ago

@lukevenediger If you want to give me write permission, I'd be more than happy to review these pull requests and make some changes. I don't mind spending time on this lib as long as I know others can benefit as well.

niemyjski commented 9 years ago

@majkinetor I don't think we need to do the null channel fallback. I was hitting a lot of perf issues and connectivitely issues and it all came down to using the udp client. The solution is to use a socket and have it recreate the socket anytime there is an error. Also you can squeeze a bit more perf in other areas (I'll try and make this changes when ever I get some free time). I ended up writing a udp implementation for my project before I had write access here.. I took it from ~20k requests/s to over 90k requests/s with failover: https://github.com/exceptionless/Foundatio/blob/master/src/Foundatio/Metrics/StatsDMetricsClient.cs

I run an oss project ( https://github.com/exceptionless/Exceptionless ), and it helped me find a lot of exceptions with the udp implementations.

niemyjski commented 8 years ago

I'm going to close this, if you can submit a pr for any perf improvements that would be greatly appreciated