hi,

We use riemann and so have added a package to report events to there. The code can't be immediately pulled in without changing the import to reference rcrowley/go-metrics rather than pingles/go-metrics but that should be it.

Hope it's useful, Paul @ uSwitch

Remove lock contention in go-metrics in a few key places. For all of these changes there is expectation around CPU performance gain but the real benefit is reduced lock contention which shows up as significant bottlenecks in both Go 1.9 and Go 1.10 for high performance services.

1. gauge_float64.go

• Removes the mutex lock around the internal value field in favor of atomic ops.
• Changes the value field to be an uint64 (no precision is lost) and does conversions
• Utilizes atomic.Load/atomic.Store to reduce lock contention
• Adds a parallel benchmark

BEFORE:
BenchmarkGuageFloat64Parallel-8   	20000000	        88.1 ns/op	       0 B/op	       0 allocs/op
AFTER:
BenchmarkGuageFloat64Parallel-8   	50000000	        28.8 ns/op	       0 B/op	       0 allocs/op

2. ewma.go

• Removes lock contention around a few fields in favor of atomic ops.
• For the mutex lock in place this lock is only taken on the first Tick() operation only
• Adds a parallel benchmark

BEFORE:
BenchmarkEWMAParallel-8   	20000000	        99.4 ns/op	       0 B/op	       0 allocs/op
AFTER:
BenchmarkEWMAParallel-8   	20000000	        74.5 ns/op	       0 B/op	       0 allocs/op

3. meter.go

• Removes lock contention around multiple fields in favor of atomic ops.
• Mutex locking is left in places where there is not high concurrency such as tickMeters and Stop.
• Adds a parallel benchmark

BEFORE:
BenchmarkMeterParallel-8   	 3000000	       467 ns/op	       0 B/op	       0 allocs/op
AFTER:
BenchmarkMeterParallel-8   	10000000	       169 ns/op	       0 B/op	       0 allocs/op

Creating a map entry with a string type as key leads to a copy of that string in Go. Iterating via Each creates a lot of these entries, especially while reporting lots of values to a remote metrics system.

Our use case is modelled with HugeRegistry and the below benchmarks show improvements there to to the removed memory allocation.

name                  old time/op    new time/op    delta
Counter-4               9.83ns ± 0%   10.03ns ± 1%   +2.03%          (p=0.016 n=4+5)
GuageFloat64-4          62.7ns ± 4%    60.6ns ± 1%   -3.38%          (p=0.008 n=5+5)
Histogram-4              117ns ± 3%     129ns ± 4%  +10.26%          (p=0.008 n=5+5)
Registry-4               760ns ± 2%     232ns ± 2%  -69.52%          (p=0.008 n=5+5)
HugeRegistry-4          2.58ms ± 3%    0.82ms ± 1%  -68.10%          (p=0.008 n=5+5)
UniformSample257-4       108ns ± 2%     123ns ± 5%  +13.10%          (p=0.008 n=5+5)
UniformSample514-4       106ns ± 2%     118ns ± 5%  +10.90%          (p=0.008 n=5+5)
UniformSample1028-4      106ns ± 1%     119ns ± 2%  +12.43%          (p=0.008 n=5+5)

name                  old alloc/op   new alloc/op   delta
Registry-4                336B ± 0%       32B ± 0%  -90.48%          (p=0.008 n=5+5)
HugeRegistry-4           598kB ± 0%     328kB ± 0%     ~             (p=0.079 n=4+5)

name                  old allocs/op  new allocs/op  delta
Registry-4                2.00 ± 0%      1.00 ± 0%  -50.00%          (p=0.008 n=5+5)
HugeRegistry-4            5.00 ± 0%      3.00 ± 0%  -40.00%          (p=0.008 n=5+5)

@armon is a maintainer of serf and statsite. He also has a project with the same name and goal as yours: https://github.com/armon/go-metrics

Perhaps you might consider combining effort?

In the spirit of how gauge is implemented in the original codahale metrics shouldn't the gauge type "pull values"? Maybe by accepting a function pointer and starting a goroutine to call the function on a fixed interval to retrieve value. It can also use a task manager to avoid spawning too many goroutines.

Histogram's Max() (and probably other, similar functions) are wrong and store values independently of the Sample being used. This means that Max() on a Histogram backed by an ExpDecaySample doesn't return the sample's current Max.

I read through the related @codahale's metrics implementation and they solve this by taking a snapshot of the sample (there it's called a Reservoir) during reporting. The snapshot is what has functions like Max() on it.

This is a nice abstraction, allowing the different Sample implementation deal with handling values, letting the snapshot tell you about the values of a sample at a given time, etc.

Would you consider a patch that re-organizes things and inserts Snapshots in between Samples and Histograms?

This commit changes the behavior of PrefixedChildRegistry to only apply the Each function to registered metrics that have the the appropriate prefix.

This commit contains two changes: 1: An originally failing test showing the behavior 2: A wrapper func around the passed in function for Each that applies the required filter

The behavior that this addresses was quite unexpected, but happy to discuss if this was the intended behavior.

Here's my proposal for addressing #26 and #31.

I still have to tackle how to expose the Sample-based Count, Max, Mean, and Min to the various reporters. Ideas are welcome.

Influxdb's client package broke their public API. I'll try to create a snapshot from the last valid version and PR with a new import path for go-metrics.

The code does not allow for non-integer metrics for gauge and meter. This was not the case in the original codahale metrics and I would like to understand the reasoning behind this limitation.

How would you feel about supporting Inc() and Dec() methods on the gauge type? A motivating use case that's come up for me is wanting to track the number of things that are currently in some undesirable state but may leave that state later.

For example, to track the number of requests that have been waiting on a response for more than a minute -- we'd increment the gauge once a request had been waiting for at least a minute, and decrement the gauge once the request completes. Doing this with a gauge gives more information than with a counter, since we can decrement it once the request is done.

Adding these methods would add incredibly little code to the repo. If this sounds alright, I'll send a quick PR for it. What do you think?

@mihasya

Do we expect StandardEWMA.Tick to be called concurrently?

If not, then there's no need to use a mutex and atomic operations on a.init, just below code is fine:

    if a.init == 0 {
        atomic.StoreUint64(&a.rate, math.Float64bits(a.fetchInstantRate())) 
        a.init = 1
    } else {
         a.updateRate(a.fetchInstantRate()) 
    }

if yes, then the current implementation is incorrect, because, after the current goroutine executed L115, other goroutines could execute L103, which creates a race condition with L116 of the current goroutine, and L103 itself could create race conditions between concurrent goroutines.

https://github.com/rcrowley/go-metrics/blob/cf1acfcdf4751e0554ffa765d03e479ec491cad6/ewma.go#L100-L120

Hi, is there any way that I can the ranges with their counts for a timer metric? Something like this:

0-10ms: 9123 requests
10-50ms: 234 requests
50-100ms: 67 requests
100-200ms: 89 requests
200-500ms: 4 requests
500-1000ms: 3 requests
1000ms+: 7 requests

I'm sorry if this is a duplicate question. I searched for anything like this, but couldn't find any issue about it.