tim-group / java-statsd-client

a java statsd client library
Other
275 stars 139 forks source link

Allow sending of double values for gauges #23

Closed xyu closed 10 years ago

xyu commented 10 years ago

Allow sending of either long or double values for StatsD gauges, this resolves #19

scarytom commented 10 years ago

Thanks very much for your pull request.

I had a look at your changes, and realised that they probably broke the sending of large long values to gauges, which helped me discover that I have no test for this. I've just pushed a small change to introduce a test case, and, indeed, your code changes make this test fail.

I'd be interested if you could find a more elegant way to solve this, but I suspect that you'll have to implement long and double messages separately rather than casting the long to a double and chaining the methods.

scarytom commented 10 years ago

See 87a76b484616b693e158fc67a7fcf40d03c7c840 for the change I made to the gauge test.

xyu commented 10 years ago

That's a very good point, thanks for catching that. I refactored gauge sending to a private method to keep the code duplication down in the double and long overloaded methods. Let me know what you think about this change.

scarytom commented 10 years ago

Thanks for making these changes, this is looking better. I think that you have a bug though: by relying on the default toString() of the double, you will get scientific notation for large values. I suggest you try writing a test like this, and you will find it fails:

@Test(timeout=5000L) public void
sends_large_fractional_gauge_to_statsd() throws Exception {
    client.recordGaugeValue("mygauge", 423423423.9d);
    server.waitForMessage();
     assertThat(server.messagesReceived(), contains("my.prefix.mygauge:423423423.9|g"));
}
Expected: iterable containing ["my.prefix.mygauge:423423423.9|g"]
     but: item 0: was "my.prefix.mygauge:4.234234239E8|g"

You might consider making your recordGaugeCommon method take a String as its value, and do the conversion at the call site.

I also added a little comment changes to StatsDClient.java, requesting better JavaDoc.

xyu commented 10 years ago

I knew there was a reason I was using a formatter before for all this; also cleaned up the JavaDoc.

scarytom commented 10 years ago

Great. I think the code is functionally correct now.

I am, however, a little concerned that the processing path for long gauge values is more heavyweight than it used to be. This might be premature optimisation on my part, but I imagine that some users might push a lot of traffic through this library. I'm going to have a little play with your changes to see if I can restore a simpler path for long values, but I'm likely to merge this pull request soon regardless.

Thanks again for your efforts.

scarytom commented 10 years ago

I've just released version 3.0.2 of the java-statsd-client, which includes these changes.