Instrument both workercache and valkey store with metrics.#347
Instrument both workercache and valkey store with metrics.#347Julian Gutierrez Oschmann (juli4n) wants to merge 1 commit into
Conversation
Add metrics to track the caching layer. Emit metrics to count pub/sub `PUBLISH` events (and failures), number of cached workers, amount of time the cache is disconnected, total resyncs (caused by a watch disconnection) and total relists.
| persistence := ateredis.NewPersistence(rdb) | ||
| persistence, err := ateredis.NewPersistence(rdb) | ||
| if err != nil { | ||
| mr.Close() |
There was a problem hiding this comment.
Is there a way to do testing.Defer or something similar so we don't need to remember to do this every time?
There was a problem hiding this comment.
That sounds like a good idea. We are already using this pattern throughout this file so I would probably tackle that as a separate PR?
| } | ||
| if err := s.rdb.Publish(ctx, workerPubSubChannel, payload).Err(); err != nil { | ||
| slog.ErrorContext(ctx, "worker event publish failed", slog.Any("err", err)) | ||
| s.metricWorkerPubsubMessages.Add(ctx, 1, metric.WithAttributes(attribute.String("error.type", "_OTHER"))) |
There was a problem hiding this comment.
I'm a little confused on this. Is it a typical pattern to increment the same metric with an error attribute, or have a separate metric for failures?
There was a problem hiding this comment.
Not an otel expert, but I think this is the recommended way of reporting errors (https://opentelemetry.io/docs/specs/semconv/general/recording-errors/#recording-errors-on-metrics).
There was a problem hiding this comment.
Putting error.type only on the failure series is 👌
There was a problem hiding this comment.
nit: always _OTHER means we can't differentiate timeouts/etc from other outages. Could we add a classifier here (and in the relist one as well) to make this more actionable?
| relistInterval: relistInterval, | ||
| workers: make(map[string]*ateapipb.Worker), | ||
| } | ||
| m := otel.Meter("workercache") |
There was a problem hiding this comment.
nit: it could be better to create a helper method for metric, and should we move this to Start method?
| return // context cancelled | ||
| } | ||
| c.ready.Store(true) | ||
| c.metricNotReadyDuration.Add(ctx, time.Since(notReadySince).Seconds()) |
There was a problem hiding this comment.
This seems to fire only after resync succeeds, so you can't alert on "cache been not ready for 2+ minutes" while it's actually happening, which is kinda the whole point, right? Also, if the pod gets oomkilled, we never record the duration.
Could we also expose readiness as a gauge?
| } | ||
| if err := s.rdb.Publish(ctx, workerPubSubChannel, payload).Err(); err != nil { | ||
| slog.ErrorContext(ctx, "worker event publish failed", slog.Any("err", err)) | ||
| s.metricWorkerPubsubMessages.Add(ctx, 1, metric.WithAttributes(attribute.String("error.type", "_OTHER"))) |
There was a problem hiding this comment.
Putting error.type only on the failure series is 👌
| } | ||
| if err := s.rdb.Publish(ctx, workerPubSubChannel, payload).Err(); err != nil { | ||
| slog.ErrorContext(ctx, "worker event publish failed", slog.Any("err", err)) | ||
| s.metricWorkerPubsubMessages.Add(ctx, 1, metric.WithAttributes(attribute.String("error.type", "_OTHER"))) |
There was a problem hiding this comment.
nit: always _OTHER means we can't differentiate timeouts/etc from other outages. Could we add a classifier here (and in the relist one as well) to make this more actionable?
There was a problem hiding this comment.
What do you think about also covering persistence.worker.pubsub.messages (success + error.type) and cache.relists?
| } | ||
| m := otel.Meter("workercache") | ||
| var err error | ||
| c.metricWorkerCount, err = m.Int64Gauge( |
There was a problem hiding this comment.
Can we make this be an Int64ObservableGauge instead? Right now, we record on every add/update/delete for a value the SDK just keeps as last value anyway. It'd also let us drop the ctx we added to applyEvent.
4 participants
Add metrics to track the caching layer. Emit metrics to count pub/sub
PUBLISHevents (and failures), number of cached workers, amount of time the cache is disconnected, total resyncs (caused by a watch disconnection) and total relists.