Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix datagram destination to use the host/port provided to DefaultSocketProvider #1

Merged
merged 1 commit into from Mar 15, 2013

Conversation

Projects
None yet
4 participants
Contributor

robert-chiniquy commented Sep 4, 2012

  • Use default constructor for DatagramSocket. Previously the host/port were
    passed to DatagramSocket to be used as the locally-bound address.
  • Set the destination host/port in the DatagramPacket constructor. Previously
    the DatagramPacket was being constructed with no destination.
  • Add import of java.net.InetAddress, remove import of java.net.InetSocketAddress.

http://docs.oracle.com/javase/6/docs/api/java/net/DatagramSocket.html#DatagramSocket()
http://docs.oracle.com/javase/6/docs/api/java/net/DatagramPacket.html#DatagramPacket(byte[], int, java.net.InetAddress, int)

Fix datagram destination to use the host/port provided to DefaultSock…
…etProvider

- Use default constructor for DatagramSocket. Previously the host/port were
  passed to DatagramSocket to be used as the locally-bound address.
- Set the destination host/port in the DatagramPacket constructor. Previously
  the DatagramPacket was being constructed with no destination.
- Add import of java.net.InetAddress, remove import of java.net.InetSocketAddress.

http://docs.oracle.com/javase/6/docs/api/java/net/DatagramSocket.html#DatagramSocket()
http://docs.oracle.com/javase/6/docs/api/java/net/DatagramPacket.html#DatagramPacket(byte[], int, java.net.InetAddress, int)

romankor commented Mar 6, 2013

Bro , what about merging that pull request. I used your class , but without that pull request its kinda useless.

Contributor

robert-chiniquy commented Mar 6, 2013

@pfeffer-kopf Until he does merge it you can always build from my fork, that's what I do.

Hey Robert
Do you have actually a working setup with that statsD reporter. As far as I saw thats a pretty almost copy of the GraphiteReporter from Metrics. It means it sends all the gauges and timers and all the stuff that StatsD supposed to compute. I don't really understand the point in computing all the stuff twice. Am i missing something ? ( sorry for dumb questions , since i got all the setup (graphite+statsd+graphitit) ready only a week ago.

What is being send right now i think that way too much info. Probably count, percentile_90, mean will be enough

Owner

organicveggie commented Mar 15, 2013

Wow. I'm really sorry. I don't know how I missed the email to merge the pull request.

organicveggie added a commit that referenced this pull request Mar 15, 2013

Merge pull request #1 from robert-chiniquy/fix/datagram-destination
Fix datagram destination to use the host/port provided to DefaultSocketProvider

@organicveggie organicveggie merged commit d67b470 into organicveggie:master Mar 15, 2013

Contributor

robert-chiniquy commented Mar 15, 2013

@pfeffer-kopf Yep it works, but you're right, some stuff seems to be aggregated twice, and I don't see the point of that either. I assumed it was just the yammer metrics way, but now that you mention it…

@organicveggie Thanks for the merge!

Contributor

fourk commented Mar 15, 2013

@pfeffer-kopf The benefit of using statsd as an intermediary is that it performs some extra aggregations for you, although admittedly most of which are useless with sane flush intervals. For example, let's say you have a Timer. Metrics will send, every flush interval, 13 [1] metrics to Statsd. For each of those 13 metrics, Statsd will compute and flush 9 [2] metrics, every statsd->graphite flush interval (a value that should be bigger than the metrics->statsd flush interval), based only on the data it received within that flush interval.

[1]: The metrics for a Timer are: 15MinuteRate, 1MinuteRate, 5MinuteRate, 75percentile, 95percentile, 98percentile, 999percentile, max, mean, meanRate, median, min, stddev

[2]: Statsd metrics: count, lower, mean, mean_90, std, sum, sum_90, upper, upper_90

Count is generally only useful for showing whether there was a problem transmitting metrics to statsd. If it is a value other than $STATSD_TO_GRAPHITE_FLUSH_INTERVAL / $METRICS_TO_STATSD_FLUSH_INTERVAL this indicates that some data points didn't end up in statsd, which is generally totally fine.

99% of the time, the only statsd metric that has value for me is mean. This probably changes if you have a larger ratio of statsd flushes per graphite flush, but with around 12 metrics->statsd flushes per statsd->graphite flush, there's not really much value provided by anything besides mean.

This is how probably a proper statdClient should look like. https://github.com/etsy/statsd/blob/master/examples/StatsdClient.java

I dont see any point in using metrics as big and as it wide it would be. My app is running on Jersey-Guice , i have it all wrapped up with an simple MethodInterceptor around @timed annotations , that uses that client to send out metrics. The outcome is tiny and clear , probably the only things that really bother me are mean,count,uppper_90

Owner

organicveggie commented Apr 10, 2013

As a side note, the data that metrics-statsd publishes is a function of codahale/metrics, not metrics-statsd. Since I generally don't care about absolute mean, I wrote different implementations on the metrics side that generated exponential weighted moving averages. You could do the same thing and only include mean, count and upper_90.

Contributor

robert-chiniquy commented Apr 10, 2013

Interesting, thanks for the note.

dclements referenced this pull request in dclements/metrics-statsd Jun 8, 2013

Merge pull request #1 from ReadyTalk/library_upgrades
Upgrading libraries to most recent versions and changing URI to fork.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment