Update metrics to follow reddit's spec #224
Conversation
| @@ -10,7 +12,7 @@ import ( | |||
|
|
|||
| const ( | |||
| success = "success" | |||
| fail = "fail" | |||
| failure = "failure" | |||
There was a problem hiding this comment.
For easier transition, can we keep both fail and failure for a while (e.g. two weeks), then remove the wrong one (fail)?
pacejackson
force-pushed
the
follow-the-spec
branch
from
2e95d65
to
dfd0d22
Compare
metricsbp/baseplate_hooks.go
Outdated
| } | ||
|
|
||
| func (c *atomicCounter) current() int64 { | ||
| return *c.count |
There was a problem hiding this comment.
this needs to be atomic.LoadInt64(c.count) instead.
metricsbp/baseplate_hooks.go
Outdated
| ) | ||
| go func() { | ||
| for { | ||
| time.Sleep(interval) |
There was a problem hiding this comment.
don't use time.Sleep in a for loop, use time.Tick/time.Ticker instead.
but I think this should be handled by RunSysStats function, using same prefixes and stuff (we are actually using labels for that one, and I think we should still use labels for this one as well).
metricsbp/baseplate_hooks.go
Outdated
| hostname = "UNKOWN-HOSTNAME" | ||
| } | ||
| gauge := metrics.Gauge( | ||
| fmt.Sprintf("runtime.%s.PID%d.active_requests", hostname, os.Getpid()), |
There was a problem hiding this comment.
shouldn't the name be concurrency instead of active_requests?
There was a problem hiding this comment.
No it's active_requests, concurrency was a mistake.
metricsbp/baseplate_hooks.go
Outdated
|
|
||
| type atomicCounter struct { | ||
| count *int64 | ||
| } |
There was a problem hiding this comment.
I also feel this is overkill/overcomplex.
That gauge, by its definition, is not per-request but only one per process. So it perfectly fine to just declare a global variable of int64, and just call atomic.AddInt64 1/-1 in the hook, and call atomic.LoadInt64 in RunSysStats to report it.
There was a problem hiding this comment.
I dunno, to me directly editing a global value using the atomic functions seems like a bad example to set? I don't think this is complex at all and sets a better example for other developers reading the code.
I can change it if that's not the case.
|
Should we also add the |
pacejackson
force-pushed
the
follow-the-spec
branch
from
03c8c2d
to
12244d3
Compare
There was a problem hiding this comment.
The build file needs to be sorted alphabetically :) https://cloud.drone.io/reddit/baseplate.go/1198/2/3
metricsbp/sys_stats.go
Outdated
| @@ -24,45 +28,49 @@ func pullRuntimeStats() (cpu cpuStats, mem runtime.MemStats) { | |||
| // RunSysStats starts a goroutine to periodically pull and report sys stats. | |||
| // | |||
| // Canceling the context passed into NewStatsd will stop this goroutine. | |||
| func (st *Statsd) RunSysStats(labels Labels) { | |||
| func (st *Statsd) RunSysStats(_ Tags) { | |||
There was a problem hiding this comment.
Oh I thought it was part of a config struct so I said leave it but ignore it to keep backward compatibility. Since this is a function arg, doing that is kinda defeating the point and we can just remove it. Also this is already a breaking change :)
pacejackson
force-pushed
the
follow-the-spec
branch
from
0013c36
to
2cdf085
Compare
No description provided.