The result label of `federatedhpa_pull_metrics_duration_seconds` is bound with `success`

[tangzhongren]

karmada/pkg/controllers/federatedhpa/metrics/client.go

Line 73 in 45cf955

defer metrics.ObserveFederatedHPAPullMetricsLatency(err, "ResourceMetric", startTime)

karmada/pkg/controllers/federatedhpa/metrics/client.go

Line 157 in 45cf955

defer metrics.ObserveFederatedHPAPullMetricsLatency(err, "RawMetric", startTime)

karmada/pkg/controllers/federatedhpa/metrics/client.go

Line 194 in 45cf955

defer metrics.ObserveFederatedHPAPullMetricsLatency(err, "ObjectMetric", startTime)

karmada/pkg/controllers/federatedhpa/metrics/client.go

Line 226 in 45cf955

defer metrics.ObserveFederatedHPAPullMetricsLatency(err, "ExternalMetric", startTime)

karmada/pkg/controllers/federatedhpa/federatedhpa_controller.go

Line 179 in 45cf955

defer metrics.ObserveProcessFederatedHPALatency(err, startTime)

The defer statement has already assigned a value to the function's parameters when it is declared. Therefore, the value of the err parameter is always nil when the metrics function is executed. Regardless of whether the actual execution result is "success" or "error", the metrics record is always "success".

[zhzhuang-zju]

karmada/pkg/controllers/federatedhpa/federatedhpa_controller.go

Lines 176 to 186 in 1417bef

	// observe process FederatedHPA latency
	var err error
	startTime := time.Now()
	defer metrics.ObserveProcessFederatedHPALatency(err, startTime)
	err = c.reconcileAutoscaler(ctx, hpa)
	if err != nil {
	return controllerruntime.Result{}, err
	}
	return controllerruntime.Result{RequeueAfter: c.HorizontalPodAutoscalerSyncPeriod}, nil

Hi @ClamRen, I don't quite understand. Taking karmada/pkg/controllers/federatedhpa/federatedhpa_controller.go as an example, shouldn't the value of err in defer metrics.ObserveProcessFederatedHPALatency(err, startTime) be the one returned by c.reconcileAutoscaler?

[tangzhongren]

karmada/pkg/controllers/federatedhpa/federatedhpa_controller.go

Lines 176 to 186 in 1417bef

// observe process FederatedHPA latency
var err error
startTime := time.Now()
defer metrics.ObserveProcessFederatedHPALatency(err, startTime)

err = c.reconcileAutoscaler(ctx, hpa)
if err != nil {
return controllerruntime.Result{}, err
}

return controllerruntime.Result{RequeueAfter: c.HorizontalPodAutoscalerSyncPeriod}, nil
Hi @ClamRen, I don't quite understand. Taking karmada/pkg/controllers/federatedhpa/federatedhpa_controller.go as an example, shouldn't the value of err in defer metrics.ObserveProcessFederatedHPALatency(err, startTime) be the one returned by c.reconcileAutoscaler?

I know your intention is to record the error returned by the entire function into the defer metrics. However, the defer statement parameters are already determined when they are declared, so no matter what the value of the err variable is after the defer statement, it will not be passed to the defer execution logic.

understand?

[zhzhuang-zju]

Get it, thank you for your explanation. We were negligent before. If we want to achieve the effect we expect, then we should probably want to use a closure.

defer func(){
    metrics.ObserveProcessFederatedHPALatency(err, startTime)
}()

Is that right?

FYI: The deferred call's arguments are evaluated immediately, but the function call is not executed until the surrounding function returns.

[RainbowMango]

Nice catch!
@ClamRen Would you like to send a PR for this?

[tangzhongren]

Get it, thank you for your explanation. We were negligent before. If we want to achieve the effect we expect, then we should probably want to use a closure.

defer func(){
metrics.ObserveProcessFederatedHPALatency(err, startTime)
}()
Is that right?

FYI: The deferred call's arguments are evaluated immediately, but the function call is not executed until the surrounding function returns.

Right! this way can solve the problem.

[tangzhongren]

Nice catch! @ClamRen Would you like to send a PR for this?

My pleasure，and which git branch should be submitted to?

[zhzhuang-zju]

/assign @ClamRen

My pleasure，and which git branch should be submitted to?

You can first submit the fixes for the master branch, and then cherry-pick them into Release-1.11, Release-1.12, and Release-1.13.

[zhzhuang-zju]

@ClamRen May I ask how you discovered this problem? Did you find this problem when using the FHPA?

[tangzhongren]

@ClamRen May I ask how you discovered this problem? Did you find this problem when using the FHPA?

When we were troubleshooting other issues with FHPA, we read the code and found the problem by the way.

[RainbowMango]

/kind bug