Add naming support to readiness probes for better logging#13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds naming support to readiness probes to improve logging and debugging capabilities. When probes are waited on during lifecycle startup, their names are now logged for better observability. If no name is set, probes are identified by their index (e.g., "probe[0]").
Key Changes
- Added
namefield toReadinessProbewithWithNameandGetNamemethods for setting and retrieving probe names - Enhanced lifecycle startup logging to include probe names when waiting for readiness, signaling completion, or handling failures
- Automatically propagate names to readiness probes when
WithNameis called onFuncActororLifecycle
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lifecycle/readiness.go | Adds name field and accessor methods (WithName, GetName) to ReadinessProbe for identification in logs |
| lifecycle/lifecycle.go | Adds debug and error logging with probe names during readiness probe waiting; updates Lifecycle.WithName to propagate name to its readiness probe |
| lifecycle/func.go | Updates FuncActor.WithName to propagate name to its readiness probe when enabled; sets initial probe name in WithReadiness method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (rp *ReadinessProbe) WithName(name string) *ReadinessProbe { | ||
| rp.name = name | ||
| return rp | ||
| } | ||
|
|
||
| // GetName returns the name of the readiness probe. | ||
| func (rp *ReadinessProbe) GetName() string { |
There was a problem hiding this comment.
The name field is accessed without mutex protection, which could lead to race conditions. The ReadinessProbe struct already has a sync.RWMutex to protect the err field. The name field should be similarly protected since WithName could be called concurrently with GetName. Consider acquiring the appropriate lock (write lock for WithName, read lock for GetName) to ensure thread-safe access.
| func (rp *ReadinessProbe) WithName(name string) *ReadinessProbe { | |
| rp.name = name | |
| return rp | |
| } | |
| // GetName returns the name of the readiness probe. | |
| func (rp *ReadinessProbe) GetName() string { | |
| func (rp *ReadinessProbe) WithName(name string) *ReadinessProbe { | |
| rp.mu.Lock() | |
| rp.name = name | |
| rp.mu.Unlock() | |
| return rp | |
| } | |
| // GetName returns the name of the readiness probe. | |
| func (rp *ReadinessProbe) GetName() string { | |
| rp.mu.RLock() | |
| defer rp.mu.RUnlock() |
| func (rp *ReadinessProbe) GetName() string { | ||
| return rp.name | ||
| } |
There was a problem hiding this comment.
The name field access in GetName is not protected by a mutex, which could lead to race conditions when called concurrently with WithName. Consider acquiring a read lock before accessing the name field to ensure thread-safe access.
| // WithName sets the name of the readiness probe for logging and debugging. | ||
| func (rp *ReadinessProbe) WithName(name string) *ReadinessProbe { | ||
| rp.name = name | ||
| return rp | ||
| } | ||
|
|
||
| // GetName returns the name of the readiness probe. | ||
| func (rp *ReadinessProbe) GetName() string { | ||
| return rp.name | ||
| } |
There was a problem hiding this comment.
The new WithName and GetName methods lack test coverage. Since the readiness_test.go file has comprehensive test coverage for other ReadinessProbe methods, consider adding tests to verify that WithName correctly sets the name and GetName retrieves it, including edge cases like empty names and concurrent access.
Provide context or links (Jira ticket, Slack thread) for both the reviewer and your future self: