Conversation
There was a problem hiding this comment.
Pull request overview
Adds extensibility points to the listener package by introducing a metrics hook interface that is invoked during session/message handling, and updates tests/mocks to validate the new behavior.
Changes:
- Introduce
MetricsRecorderinterface +WithMetricsRecorderoption with a default discard implementation. - Emit metrics hooks for statistics, job started/completed, and desired runner count decisions.
- Update listener unit tests and add an autogenerated
MockMetricsRecorder.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| listener/listener.go | Adds MetricsRecorder hooks, option wiring, and records metrics during Run/message handling. |
| listener/listener_test.go | Updates tests to pass a metrics recorder and assert metrics hooks are called. |
| listener/mocks_test.go | Adds autogenerated mock implementation for MetricsRecorder. |
Comments suppressed due to low confidence (1)
listener/listener_test.go:178
assert.ErrorIshas the arguments reversed here. The signature isassert.ErrorIs(t, err, target); as written it will treatcontext.Canceledas the error under test anderras the target, which can cause the assertion to behave incorrectly.
err = l.Run(ctx, handler)
assert.ErrorIs(t, context.Canceled, err)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func WithMetricsRecorder(recorder MetricsRecorder) Option { | ||
| return func(l *Listener) { | ||
| l.metricsRecorder = recorder | ||
| } | ||
| } |
There was a problem hiding this comment.
WithMetricsRecorder will happily accept nil and overwrite the default recorder, which will panic later when the listener calls l.metricsRecorder.*. Consider ignoring nil recorders (keep the discard implementation) or defaulting nil to &discardMetricsRecorder{} inside this option.
| func (l *Listener) handleMessage(ctx context.Context, handler Scaler, msg *scaleset.RunnerScaleSetMessage) error { | ||
| l.handleStatistics(ctx, msg.Statistics) | ||
|
|
||
| if err := l.client.DeleteMessage(ctx, msg.MessageID); err != nil { | ||
| return fmt.Errorf("failed to delete message: %w", err) | ||
| } | ||
|
|
||
| for _, jobStarted := range msg.JobStartedMessages { | ||
| l.metricsRecorder.RecordJobStarted(jobStarted) | ||
| if err := handler.HandleJobStarted(ctx, jobStarted); err != nil { | ||
| return fmt.Errorf("failed to handle job started: %w", err) | ||
| } | ||
| } | ||
| for _, jobCompleted := range msg.JobCompletedMessages { | ||
| l.metricsRecorder.RecordJobCompleted(jobCompleted) | ||
| if err := handler.HandleJobCompleted(ctx, jobCompleted); err != nil { | ||
| return fmt.Errorf("failed to handle job completed: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if _, err := handler.HandleDesiredRunnerCount(ctx, msg.Statistics.TotalAssignedJobs); err != nil { | ||
| desiredCount, err := handler.HandleDesiredRunnerCount(ctx, msg.Statistics.TotalAssignedJobs) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to handle desired runner count: %w", err) |
There was a problem hiding this comment.
handleMessage assumes msg.Statistics is non-nil (handleStatistics assigns it and later msg.Statistics.TotalAssignedJobs is dereferenced). Since RunnerScaleSetMessage.Statistics is a pointer, a nil value here will cause a panic and can also set l.latestStatistics to nil (breaking the msg == nil path). Add a nil check and return an error (similar to the initial session statistics check) before using it.
No description provided.