Added metrics for the encoding manager. #967
Conversation
Signed-off-by: Cody Littley <[email protected]>
| @@ -154,5 +176,12 @@ func RunController(ctx *cli.Context) error { | |||
| return fmt.Errorf("failed to start dispatcher: %v", err) | |||
| } | |||
|
|
|||
| go func() { | |||
There was a problem hiding this comment.
should we have this routine start in encodingManager.Start instead?
There was a problem hiding this comment.
I thought about this before you commented. The core issue is that RunController() starts both the encoding manager and the dispatcher, and we only want to use a single metrics server that they both share.
| } | ||
|
|
||
| func (m *encodingManagerMetrics) reportBatchSubmissionLatency(duration time.Duration) { | ||
| m.batchSubmissionLatency.WithLabelValues().Observe(float64(duration.Nanoseconds()) / float64(time.Millisecond)) |
There was a problem hiding this comment.
why do we need this division? can we just report duration.Millisecond?
There was a problem hiding this comment.
My dislike for Milliseconds() is that it reports an integer value, but we are measuring things where sub-millisecond accuracy may end up being meaningful. Converting from nanoseconds is more cumbersome, but utilizes the full accuracy of the clock.
There was a problem hiding this comment.
but we are measuring things where sub-millisecond accuracy may end up being meaningful
Not sure if this is true, but I'm fine with this
| batchSize *prometheus.GaugeVec | ||
| batchDataSize *prometheus.GaugeVec | ||
| batchRetryCount *prometheus.GaugeVec | ||
| batchSleepTime *prometheus.GaugeVec |
There was a problem hiding this comment.
not sure if this metric would be useful given that it's already tracking retry count?
Signed-off-by: Cody Littley <[email protected]>
Why are these changes needed?
Add metrics for the encoding manager.
Checks