uber-go / tally

A Go metrics interface with fast buffered metrics and third party reporters
MIT License
845 stars 115 forks source link

NopCachedStatsReporter #23

Open anuptalwalkar opened 7 years ago

anuptalwalkar commented 7 years ago

I see that we have implemented CachedStatsReporter here, https://github.com/uber-go/tally/pull/22 but there is no Nop implementation for testing. This is making transition from StatsReporter to CachedStatsReporter rather difficult. Do we have any plans to provide NopCachedStatsReporter similar to existing tally.NullStatsReporter?

Raynos commented 7 years ago

We've not implemented Null, Statsd or Prometheus yet for the cached stats reporter.

For testing purposes I will need to use use Statsd and Null reporters soon, I can contribute these to tally.

I've currently worked around it on master by having all tests use an InternalMetricsReporter.

I don't personally use Prometheus so someone else will need to have look into implementing CachedRepoter for Prometheus.

anuptalwalkar commented 7 years ago

I have a NopCachedStatsReporter written for testing - https://github.com/uber-go/fx/blob/master/metrics/nop_reporter.go I am happy to share if needed.

robskillington commented 7 years ago

@anuptalwalkar is there any reason you can't use a tally.TestScope?

You can create one by calling tally.NewTestScope(...) as implemented here:

func NewTestScope(
    prefix string,
    tags map[string]string,
) TestScope {
    // ...
}

Then you get the following abilities:

type TestScope interface {
    Scope

    // Snapshot returns a copy of all values since the last report execution,
    // this is an expensive operation and should only be use for testing purposes
    Snapshot() Snapshot
}

// Snapshot is a snapshot of values since last report execution
type Snapshot interface {
    // Counters returns a snapshot of all counter summations since last report execution
    Counters() map[string]CounterSnapshot

    // Gauges returns a snapshot of gauge last values since last report execution
    Gauges() map[string]GaugeSnapshot

    // Timers returns a snapshot of timer values since last report execution
    Timers() map[string]TimerSnapshot
}

// CounterSnapshot is a snapshot of a counter
type CounterSnapshot interface {
    // Name returns the name
    Name() string

    // Tags returns the tags
    Tags() map[string]string

    // Value returns the value
    Value() int64
}

// GaugeSnapshot is a snapshot of a gauge
type GaugeSnapshot interface {
    // Name returns the name
    Name() string

    // Tags returns the tags
    Tags() map[string]string

    // Value returns the value
    Value() float64
}

// TimerSnapshot is a snapshot of a timer
type TimerSnapshot interface {
    // Name returns the name
    Name() string

    // Tags returns the tags
    Tags() map[string]string

    // Values returns the values
    Values() []time.Duration
}
jeffbean commented 7 years ago

The issue here is doing a setup function to return the scope, reporter and closer can not easily be replaced today. The NullStatsReporter does not implement CachedStatsReporter interface.

The use case is something I want to test with as well. If you make a public function to create the metrics with a CachedStatsReporter and need to test it out there is no supporting tally NullCachedStatsReporter.

Example from my use case as well. https://github.com/jeffbean/zkpacket/blob/master/metrics.go

anuptalwalkar commented 7 years ago

@robskillington, @jeffbean answered your question. The M3 backend also takes CachedStatsReporter instead of StatsReported. This prompted me to have NopCachedStatsReporter for nop use cases. Updated implementation is here with histogram support - https://github.com/uber-go/fx/pull/334 This should be part of Tally.

robskillington commented 7 years ago

Hey, cool no problems. Sure if you don't want to collect the actual values then a nil reporter is useful. Let me add.

madhuravi commented 7 years ago

any update on this? @robskillington

akshayjshah commented 7 years ago

Not sure about @jeffbean, but FX doesn't need this any more. Unless Bean still needs this functionality, feel free to close this.