Implemented reconcile metrics

[KirrTap]

Added metrics to the reconcilers

[lukasfrank]

Can we keep passing the prepared logger and maybe define a global name like you did for MachineReconcilerName?

[lukas016]

about passing argument i am not completely sure. I personally suggest remove passing over argument and directly initialize in function log from r.log. because passing of arguments doesn't add any benefits because start.. functions are methods of MachineReconciler and they have direct access to main logger. Now we just "spilitting" configuration of operation into 2 functions. I prefer keep it together.

My suggestion:

func (r *MachineReconciler) startCheckAndEnqueueVolumeResize(ctx context.Context) {
  log := r.log.WithName(MachineReconcilerVolumeSize)
  ...

[lukasfrank]

WithName adds context and appends the name to the existing name.

[lukas016]

I really don't understand why are you giving me here function description and you don't react usefully to my comment.
But okay it appends name separate with ., if you want to see whole description.
But it is useless, if we discuss how passing child logger into function.

[balpert89]

Dissecting the requests here.

1. Request to introduce global consts for the individual logger decorators: I agree with @lukasfrank here, being consistent here is introducing a const for each decorator name similar to https://github.com/ironcore-dev/libvirt-provider/pull/384/files#diff-a2d4663c7af1f98625f6b53f602e53ce910de934bd827f232e31db91bf9bb5acR49

2. Injecting vs. Self-decorating: This is a bit of a matter of taste in this particular spot. You can argue for both sides, keeping the injection by method argument enables the method to just use whatever logger is passed in and go with it. Also, configuration is kept outside of the method itself and is done by the calling instance. The self-decorating approach @lukas016 suggests is also valid as it encapsulates the responsibilities for behavior in the start... methods. Ultimately, the suggestion by @lukas016 is a good compromise:

func (r *MachineReconciler) startCheckAndEnqueueVolumeResize(ctx context.Context) {
  log := r.log.WithName(MachineReconcilerVolumeSize)
  ...

When testing, you pass in your mock logging provider to the containing reconciler struct and the start... methods consume this, with also self-contained decorators.

[lukasfrank]

pls use the const here

[lukasfrank]

Why are the metrics not passed as options? It makes it hard to pass a mock metric provider in tests

[lukas016]

why do you want to mock metrics? these metrics has to represent internal stuff of controller (errors, workers, ...) and they don't have any special logic so you can create them in unit tests and it will work.

[lukasfrank]

You can pass your own simple implementation in order to check the functionality.

[lukas016]

i checked that and i don't think we need mocking interface.

You can very nice test metrics directly with testutil package from prometheus without web server:

var _ = Describe("Controller runtime metrics", func() {
	Context("metric ControllerRuntimeReconcileErrors", func() {
		It("should increment metric", func() {
			m := metrics.ControllerRuntimeReconcileErrors.WithLabelValues(controllers.MachineReconcilerName)
			m.Inc()
			m.Inc()
			expectedMetric := `
# HELP controller_runtime_reconcile_errors_total Total number of reconciliation errors per controller
# TYPE controller_runtime_reconcile_errors_total counter
controller_runtime_reconcile_errors_total{controller="machine-reconciler"} 2
`
			reader := strings.NewReader(expectedMetric)
			Expect(testutil.GatherAndCompare(prometheus.DefaultGatherer, reader, "controller_runtime_reconcile_errors_total")).ToNot(HaveOccurred())
		})
	})
})

[lukasfrank]

The code above describes a test case which checks if you can increment a metrics counter, which is covered already by the lib.
I'm talking about a test where we create a resource, and check if metric x, or y is as expected.

[lukas016]

you have it described above.

[balpert89]

What @lukasfrank means here is to add fields in the opts struct https://github.com/ironcore-dev/libvirt-provider/blob/main/internal/controllers/machine_controller.go#L64 which are created before the reconciler object is instantiated and injected from the outside. I agree that this enables better testing because you are not relying on prometheus library when testing your own reconciler here, as you will provide a mock provider for the metrics.

[balpert89]

applied in faaa29b

[lukasfrank]

Can we add at least a simple test?

[lukas016]

without support of mocking we can just create integration test.

[lukas016]

we can implement both see my comment: #384 (comment)