Send data plane health request #683
Conversation
aphralG
requested review from
oliveromahony,
dhurley,
Dean-Coakley,
craigell,
olli-holmala and
spencerugbo
✅ Deploy Preview for agent-public-docs canceled.
|
| if !gc.isConnected.Load() { | ||
| slog.InfoContext(ctx, "gRPC client not connected yet. Skipping sending data plane health update") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This is likely out of scope but should we be buffering messages in the event that we aren't connected?
There was a problem hiding this comment.
yeah maybe create a separate task for that. We should be retrying until a connection is made
There was a problem hiding this comment.
I'm not sure, to be honest I just went off what was done for the data plane status for this.
| } | ||
|
|
||
| func (hw *HealthWatcherService) Watch(ctx context.Context, ch chan<- InstanceHealthMessage) { | ||
| monitoringFrequency := hw.agentConfig.Watchers.InstanceHealthWatcher.MonitoringFrequency |
There was a problem hiding this comment.
security: Add nil checks for agentConfig and Watchers. Or at least for Watchers.
There was a problem hiding this comment.
Discussed in office, will be fixed in later PR
| } | ||
| } | ||
|
|
||
| func (hw *HealthWatcherService) health(ctx context.Context) ([]*v1.InstanceHealth, bool, |
There was a problem hiding this comment.
I generally think that it's a good idea to have named return values if one of them is a bool, as you can't infer what the bool means based on the signature alone without them.
| func (hw *HealthWatcherService) health(ctx context.Context) ([]*v1.InstanceHealth, bool, | ||
| ) { | ||
| healthStatuses := make([]*v1.InstanceHealth, 0, len(hw.watchers)) | ||
| currentHealth := make(map[string]*v1.InstanceHealth) |
There was a problem hiding this comment.
nit: I think we can add a size hint for this one?
https://github.com/uber-go/guide/blob/master/style.md#specifying-map-capacity-hints
Proposed changes
Added:
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocumentmake install-toolsand have attached any dependency changes to this pull requestREADME.md)