feat: expose metrics about the operator

[CAJan93]

What's changed and what's your intention?

WORK IN PROGRESS! DO NOT MERGE!

Implementing feature request from Issue 123

• Summarize your change (mandatory)
• How does this PR work? Need a brief introduction for the changed logic (optional)
• Describe clearly one logical change and avoid lazy messages (optional)
• Describe any limitations of the current code (optional)

Checklist

• I have written the necessary docs and comments
• I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

[github-actions]

license-eye has totally checked 112 files.

Valid	Invalid	Ignored	Fixed
111	1	0	0

Click to see the invalid file list

• pkg/metrics/metrics.go

[codecov]

Codecov Report

Merging #214 (1bfe4f3) into main (d42f944) will increase coverage by 0.42%.
The diff coverage is 87.14%.

@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
+ Coverage   81.43%   81.86%   +0.42%     
==========================================
  Files          34       36       +2     
  Lines        3022     3115      +93     
==========================================
+ Hits         2461     2550      +89     
- Misses        510      511       +1     
- Partials       51       54       +3     

Flag	Coverage Δ
unittests	81.86% <87.14%> (+0.42%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/controller/risingwave_controller.go	71.42% <61.76%> (-1.48%)	⬇️
...hook/risingwave_pod_template_validating_webhook.go	80.95% <75.00%> (+2.00%)	⬆️
pkg/webhook/mutating_webhook_metrics_recorder.go	91.30% <91.30%> (ø)
pkg/webhook/risingwave_validating_webhook.go	98.80% <96.77%> (+0.01%)	⬆️
pkg/ctrlkit/loop.go	100.00% <100.00%> (ø)
pkg/webhook/risingwave_mutating_webhook.go	100.00% <100.00%> (ø)
...ebhook/risingwave_pod_template_mutating_webhook.go	100.00% <100.00%> (ø)
pkg/webhook/validating_webhook_metrics_recorder.go	100.00% <100.00%> (ø)
pkg/command/create/create.go	88.28% <0.00%> (+8.59%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[CAJan93]

I am currently able to expose the metrics on localhost:8080. I am unable to see the metrics in prometheus. This may be due to one of two things:

• I did not deploy the latest version of the operator
• The metrics are not being scraped correctly

[arkbriar]

Generally, the metrics are what we wanted but I think we'd better:

• Don't inject metrics update in the ctrlkit package, make it less coupling
• Label metrics with distinguishable labels, such as webhook type and name for webhook metrics, controller name and action name for workflow actions

I'll detail in #123 later.

[arkbriar]

I don't know what happened, mergify just closed the PR? 🤔 Anyway, I reopened it.

[arkbriar]

Yes, I think so. IMO we should return an error to notify the controller-runtime framework that an error happens.
However, an error means an immediate requeueing so our best strategy would be to log the panic and return ctrl.Result{}, nil.

Sorry, I didn't make it consistent. Actually reconcile.Result is just an alias of ctrl.Result so I always mixed them up.

Good point. I think this totally makes sense. Sidetone: The function returns a reconcile.Result, not ctrl.Result, Error.
IMHO it would be the best to requeue here with some delay, to avoid immediately crashing again. What do you think @arkbriar ?

Sounds good, but how 🤔 ? I don't think we could achieve that by blocking the thread. The controller uses a worker pool and if we block too many, the others might not be able to run anymore.

[github-actions]

license-eye has totally checked 115 files.

Valid	Invalid	Ignored	Fixed
114	1	0	0

Click to see the invalid file list

• pkg/utils/types.go

[CAJan93]

In a deployment where the Prometheus operator is also installed. This should be an optional setting. We can add a doc about how to apply this. We can leave it to another PR.

I opened this PR. Let's discuss it there.

[arkbriar]

Generally LGTM! Great work! Thanks for your great effort!

I left some comments and please fix the issues listed before merging.

[arkbriar]

I think we don't need the return value here anymore since it is not used. Shall we remove it?

[CAJan93]

True. We have to change the res *reconcile.Request though. Let me fix that.

[CAJan93]

Sounds good, but how 🤔 ? I don't think we could achieve that by blocking the thread. The controller uses a worker pool and if we block too many, the others might not be able to run anymore.

I also do not know :/