Add event recorder for ISBService, Pipeline and NumaflowController Rollout #173
Conversation
| @@ -143,12 +153,14 @@ func (r *ISBServiceRolloutReconciler) Reconcile(ctx context.Context, req ctrl.Re | |||
| statusUpdateErr := r.updateISBServiceRolloutStatus(ctx, isbServiceRollout) | |||
| if statusUpdateErr != nil { | |||
| r.customMetrics.ISBServicesSyncFailed.WithLabelValues().Inc() | |||
| r.recorder.Eventf(isbServiceRollout, corev1.EventTypeWarning, "StatusUpdateFailed", "Failed to update isb service rollout status: %v", err.Error()) | |||
There was a problem hiding this comment.
maybe the ISBServicesSyncFailed metric, the Event, and possibly the logging of the error should all go into one function?
There was a problem hiding this comment.
I don't think it's valid for cases, like here logger is not available
There was a problem hiding this comment.
How do you mean that logger isn't available there? It's always available. If it's a function which is sort of at the top of the stack in our code we call logger.GetBaseLogger() like we do in that function here. Otherwise, we call logger.FromContext(ctx) to get the logger.
There was a problem hiding this comment.
I mean, Currently we are not logging the error here and if we have a separate fn which will log error as well then we might get duplicate log as it's returning the error from here so parent fn will also print that error. if that's okay then I'll make the change.
There was a problem hiding this comment.
Are we always returning an error from Reconcile() - I'm curious if doing so results in an error being logged by the controller-runtime framework. It seems to me like it does. In that case, we could remove where we are explicitly logging because the framework does it anyway. Then this function could just write the Event and write the metric.
There was a problem hiding this comment.
Make the changes accordingly
chandankumar4
force-pushed
the
event
branch
3 times, most recently
from
97f1b58
to
92fe7f1
Compare
Signed-off-by: chandankumar4 <[email protected]>
Signed-off-by: chandankumar4 <[email protected]>
chandankumar4
changed the title
chandankumar4
changed the title
Signed-off-by: chandankumar4 <[email protected]>
Fixes #153
Modifications
Verification