Relative gauges #40
Relative gauges #40
Conversation
|
I need to comb through the actual code, but it looks like it could use a little Thanks for all these contributions @JamesCohen-awin |
|
Thanks - didn't know about |
JamesCohen-awin
force-pushed
the
relative-gauges
branch
from
b425dd7
to
00da05f
Compare
|
rebased to current HEAD 1ac36f2 |
| @@ -349,6 +397,32 @@ func parseMessage(data []byte) []*Packet { | |||
| log.Printf("ERROR: failed to ParseInt %s - %s", string(val), err) | |||
| continue | |||
| } | |||
| } else if mtypeStr[0] == 'g' { | |||
|
|
|||
| var ( | |||
There was a problem hiding this comment.
style 👮
We don't normally use this style of var declarations, would you mind using one per line?
Also, can you remove any instances of vertical whitespace after the start of a block? (like above the start of var here)
Thanks!
|
@JamesCohen-awin thanks again, this looks good functionally - just had a few style/go idiom nitpicks |
|
Thanks - all of the suggestions made perfect sense |
| switch val[0] { | ||
| case '+', '-': | ||
| relative = true | ||
| negative = string(val[0]) == "-" |
There was a problem hiding this comment.
missed this the first time around, don't need the string(), use the "rune check" from my previous comment
|
give me a sec and I'll squash all of this down into a single commit |
JamesCohen-awin
force-pushed
the
relative-gauges
branch
from
c7ed5e0
to
deae9d9
Compare
| for g, c := range gauges { | ||
| if c == math.MaxUint64 { | ||
|
|
||
| var lastValue uint64 |
There was a problem hiding this comment.
you can drop this var declaration, the next line is all that's necessary (it uses :=)
JamesCohen-awin
force-pushed
the
relative-gauges
branch
from
deae9d9
to
4a76977
Compare
| @@ -122,7 +129,39 @@ func packetHandler(s *Packet) { | |||
| } | |||
| timers[s.Bucket] = append(timers[s.Bucket], s.Value.(uint64)) | |||
| } else if s.Modifier == "g" { | |||
| gauges[s.Bucket] = s.Value.(uint64) | |||
| var gaugeValue uint64 = 0 | |||
There was a problem hiding this comment.
all int types have a default value of 0, so you can drop the = 0
|
I can't fix/squash/push fast enough ;) Going to wait until you're done and re-push |
|
P.S. in the future you don't have to squash as we're going through code review... What we normally do is wait for a PR to get to a "LGTM" stage and then squash. Not only is this an easier back-and-forth, but it allows the reviewer to see the changes as individual commits as they're happening, rather than losing context. |
|
and I think I got everything now 😁 |
JamesCohen-awin
force-pushed
the
relative-gauges
branch
from
4a76977
to
523a751
Compare
|
All sorted and thanks for the education edit: just saw the bit about the LGTM too. Noted for next time. |
|
awesome, nice work and thanks again for all your contributions! |
The etsy statsd implementation includes support for relative operations on gauges (see https://github.com/etsy/statsd/blob/master/docs/metric_types.md#gauges). I've attempted to duplicate this functionality.
I followed the statsd specs and retained the gauges as uint64 values.
This is my first attempt at coding in Go. Please be gentle!