switch metrics to float64 #68
Conversation
|
Thanks @teepark. Looks good. The |
|
worth mentioning that @jsocol's pystatsd recently started sending floats for all timings (so I had to temporarily back down to pystatsd-3.1) |
|
I agree that supporting floats is a good idea, but I don't think those "official statsd specs" support the idea:
|
markrechler
changed the title
teepark
force-pushed
the
float-timers
branch
from
050ee02 to
4bf6aa9
Compare
|
@markrechler all rebased and squashed! |
| @@ -289,12 +289,12 @@ func processGauges(buffer *bytes.Buffer, now int64) int64 { | |||
|
|
|||
| switch { | |||
| case hasChanged: | |||
| fmt.Fprintf(buffer, "%s %d %d\n", bucket, currentValue, now) | |||
| fmt.Fprintf(buffer, "%s %.0f %d\n", bucket, currentValue, now) | |||
There was a problem hiding this comment.
So, with this PR, statsdaemon will still send integer values to graphite, even though statsdaemon will accept floats?
The most specific thing I can find in the official graphite documentation is that the Whisper backend stores doubles and can handle anything python's float() can parse: http://graphite.readthedocs.org/en/0.9.12/whisper.html#data-points
There was a problem hiding this comment.
Good catch. On re-read was wondering how the test values did not change.
There was a problem hiding this comment.
ugh, the documentation never even touched on that: http://graphite.readthedocs.org/en/0.9.12/feeding-carbon.html#the-plaintext-protocol
had to dig for a bit, but it looks like carbon parses it that way as well:
https://github.com/tmm1/graphite/blob/master/carbon/lib/carbon/protocols.py#L66
My goal was to accept floats from the statsd client, leaving the statsdaemon->graphite connection untouched. I'll change it to pass floats back to graphite, but one question: for float values that represent integers (as closely as they can), do we want to truncate to the integer in the protocol (it's a pretty verbose integer now for the most common case http://play.golang.org/p/AA1ku5wkDF)? And if so, what should that test look like?
There was a problem hiding this comment.
I'd vote for not truncating and accepting the increased verbosity. Feels like it will be cleaner not having to have a truncate case.
There was a problem hiding this comment.
Actually looks like it's a moot point -- we can format them all with strconv.FormatFloat(n, 'f', -1, 64) which includes the minimum number of decimal places to represent it exactly (and appears to omit the decimal point entirely for integers).
There was a problem hiding this comment.
strconv.FormatFloat() looks great - I was a bit worried initially about avoiding exponent notations that would be printed and parsed differently by go and python, and FormatFloat() seems to do exactly what we want
+ timers + guages + counters Also pass values back to graphite as formatted floats.
teepark
force-pushed
the
float-timers
branch
from
4bf6aa9 to
751c707
Compare
| @@ -289,12 +289,12 @@ func processGauges(buffer *bytes.Buffer, now int64) int64 { | |||
|
|
|||
| switch { | |||
| case hasChanged: | |||
| fmt.Fprintf(buffer, "%s %d %d\n", bucket, currentValue, now) | |||
| fmt.Fprintf(buffer, "%s %s %d\n", bucket, strconv.FormatFloat(currentValue, 'f', -1, 64), now) | |||
| lastGaugeValue[bucket] = currentValue | |||
| gauges[bucket] = math.MaxUint64 | |||
I was having trouble with go-statsd-client and realized the issue was server-side: timers are being parsed as uint64s, but decimal points are allowed. evidence