Remove in memory sink from fanout and add latency sampling methods #28
Conversation
| @@ -98,7 +98,7 @@ func New(cfg Config) (_ *Metrics, rerr error) { | |||
| }() | |||
|
|
|||
| m := &Metrics{memSink: memSink} | |||
There was a problem hiding this comment.
Don't create the fanout sink, you should be able to use a MetricSink object and either assign promSink or memSink to it.
telemetry/wrapper.go
Outdated
| // ModuleMeasureSinceWithSampling samples latency metrics given the sample rate. | ||
| // This is intended to be used in hot code paths. | ||
| func ModuleMeasureSinceWithSampling(module string, start time.Time, sampleRate float64, keys ...string) { | ||
| if rand.Float64() < sampleRate { |
There was a problem hiding this comment.
There are a lot of issues with sampling based upon a probabilistic rate since:
- it usually requires a lot of tuning over time.
- you can't be sure if the telemetry information is skewed due to low frequency of events or bad random number generator sequence.
Sampling per N units of time makes a lot more sense. For example only allow this metric to be sampled once every second. It would also be good to scale the sample based upon how many times it has occurred correctly but that seems like it would require changing the go-metrics package to take bulk updates for samples.
If you still want to go down this path then it would make sense to use a faster random number generator like https://github.com/flyingmutant/rand and also ensure that you perform the sampling before you compute the time which would require the sampling to happen in the callers method (which is annoying without macros).
There was a problem hiding this comment.
As discussed on slack, we will go as is.
We should swap to use float32 as we don't need the extra precision that float64 provides and it takes less effort to RNG 32 bits.
There was a problem hiding this comment.
it actually takes more effort because Float32 internally calls Float64 😅
There was a problem hiding this comment.
That is terrible. Likely why other rand libraries are so common in golang.
telemetry/wrapper.go
Outdated
| @@ -19,6 +20,18 @@ func NewLabel(name, value string) metrics.Label { | |||
| return metrics.Label{Name: name, Value: value} | |||
| } | |||
|
|
|||
| // ModuleMeasureSinceWithSampling samples latency metrics given the sample rate. | |||
There was a problem hiding this comment.
Update comment to state sampleRate should be between [0, 1.0)
telemetry/wrapper.go
Outdated
| // ModuleMeasureSinceWithSampling samples latency metrics given the sample rate. | ||
| // This is intended to be used in hot code paths. | ||
| func ModuleMeasureSinceWithSampling(module string, start time.Time, sampleRate float64, keys ...string) { | ||
| if rand.Float64() < sampleRate { |
There was a problem hiding this comment.
As discussed on slack, we will go as is.
We should swap to use float32 as we don't need the extra precision that float64 provides and it takes less effort to RNG 32 bits.
telemetry/wrapper.go
Outdated
| @@ -70,3 +83,11 @@ func SetGaugeWithLabels(keys []string, val float32, labels []metrics.Label) { | |||
| func MeasureSince(start time.Time, keys ...string) { | |||
| metrics.MeasureSinceWithLabels(keys, start.UTC(), globalLabels) | |||
| } | |||
|
|
|||
| // MeasureSinceWithSampling provides a wrapper functionality for emitting a a time measure | |||
| // metric with sampling. | |||
telemetry/wrapper.go
Outdated
| // ModuleMeasureSinceWithSampling samples latency metrics given the sample rate. | ||
| // This is intended to be used in hot code paths. | ||
| func ModuleMeasureSinceWithSampling(module string, start time.Time, sampleRate float64, keys ...string) { | ||
| if rand.Float64() < sampleRate { |
There was a problem hiding this comment.
It would be nice to move the sampling check outside of the method so that we don't have to pay the cost of defer or time.Now(). That would also make this method pointless.
There was a problem hiding this comment.
yeah maybe we should just do it that way. let me revert
telemetry/wrapper.go
Outdated
| metrics.MeasureSinceWithLabels( | ||
| keys, | ||
| start.UTC(), | ||
| append([]metrics.Label{NewLabel(MetricLabelNameModule, module)}, globalLabels...), |
There was a problem hiding this comment.
Should we add a label that states the sampling rate?
Description
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change