tally icon indicating copy to clipboard operation
tally copied to clipboard

NopCachedStatsReporter

Open anuptalwalkar opened this issue 8 years ago • 8 comments

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?

anuptalwalkar avatar Jan 27 '17 21:01 anuptalwalkar

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.

Raynos avatar Jan 27 '17 23:01 Raynos

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.

anuptalwalkar avatar Jan 30 '17 19:01 anuptalwalkar

@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
}

robskillington avatar Feb 05 '17 23:02 robskillington

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

jeffbean avatar Feb 11 '17 18:02 jeffbean

@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.

anuptalwalkar avatar Feb 27 '17 18:02 anuptalwalkar

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

robskillington avatar May 15 '17 17:05 robskillington

any update on this? @robskillington

madhuravi avatar May 19 '17 18:05 madhuravi

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

akshayjshah avatar Jul 08 '17 17:07 akshayjshah