feat: exposing metrics about the operator about risingwave-operator HOT 6 CLOSED

FYI: These are the metrics which are implemented by default: docs

from risingwave-operator.

TODOs:

• Currently metrics are labeled as coming from the kube-rbac-proxy and not from the manager. We need to change that
• Use NewCounterVec with the other metrics
• Use NewCounterVec to implement the other attributes, e.g. the API version

from risingwave-operator.

@arkbriar I have a couple questions. Could you have a look, please?

• Should webhook_request_pass_count be incremented during calls to the mutating webhook? IMHO a request to a mutating webhook always passes and only a request to a validating webhook can be rejected.
• I do not fully understand controller_reconcile_requeue_after. What is the difference to controller_reconcile_duration. To be clear: We want to count the ms when we call RequeueAfter, right?
• I am not sure how much sense controller_reconcile_panic_count makes sense at the moment, since we do not have any calls to panic in the reconciler
• What collector do you refer to in the issue?

from risingwave-operator.

• Should webhook_request_pass_count be incremented during calls to the mutating webhook? IMHO a request to a mutating webhook always passes and only a request to a validating webhook can be rejected.

Yes, it should be incremented. The mutating webhook can reject a request by returning an error.

• I do not fully understand controller_reconcile_requeue_after. What is the difference to controller_reconcile_duration. To be clear: We want to count the ms when we call RequeueAfter, right?

Yes, you're right! IMO, controller_reconcile_requeue_after should be a Histogram value and only be updated when the result contains a non-zero RequeueAfter, e.g., Result{RequeueAfter: time.Second}. It's quite different from the controller_reconcile_duration which records the elapsed time of the Reconcile method per execution. The controller_reconcile_duration is similar to the controller_runtime_reconcile_time_seconds but with additional labels.

• I am not sure how much sense controller_reconcile_panic_count makes sense at the moment, since we do not have any calls to panic in the reconciler

Panics can be implicitly triggered, e.g., divide-zero panics. Panic always means bugs that need to be fixed ASAP, but also we don't want conditional panics caused by some objects to affect others. So the best idea is to recover from the panic and let the controller keep running, and of course record it so that we can set an alert on this. Currently, there's no recovery implemented and I think it's easy to add one while adding the controller_reconcile_panic_count metric.

• What collector do you refer to in the issue?

Oh, I mean the metric collectors, i.e., the codes for recording the metrics. And by proxy I mean we can use a proxy pattern to do that, like the following:

type MutatingWebhook interface {
  Default(context.Context, runtime.Object) error
}

type MutatingWebhookMetricsRecorder struct {
   // extra labels
   // ...
   
   // webhook
   webhook MutatingWebhook
}

func (r *MutatingWebhookMetricsRecorder) Default(ctx context.Context, obj runtime.Object) error {
  r.RecordBefore()
  defer r.RecordAfter()
 
  return r.webhook.Default(ctx, obj)
}

from risingwave-operator.

Closing because the targets are all implemented! Thanks @CAJan93 for your efforts in this!

from risingwave-operator.

Thanks for closing. My pleasure

from risingwave-operator.