measure UserSignup provision time

[MatousJobanek]

Measures the provision time of the UserSignups.
The start-time is taken from the annotation set in reg-service, as the end-time is taken the moment when UserSignup is marked as Complete.
The provision time is measured only when:

• the user didn't have to complete the whole verification
• the UserSignup wasn't approved manually
• when the annotation is present

The annotation is removed to ensure that we don't record the provision time twice for the same user

TODO:

• e2e tests

depends on:
codeready-toolchain/api#464
codeready-toolchain/toolchain-common#461
codeready-toolchain/registration-service#515 (this needs to be merged first to properly test it in e2e tests)

paired with codeready-toolchain/toolchain-e2e#1126

SANDBOX-957

[alexeykazakov]

Code-wise looks good but this metric measures the time between the Signup request (UserSignupRequestReceivedTimeAnnotationKey which I guess will be added later in a separate PR?) and the UserSingup set to the Complete status.

But https://docs.google.com/document/d/1xr1CVKIjcTdp5obpmTMGoNr7syFplnVArd-h5-yQnDk/edit?tab=t.0#heading=h.xd8t24f9nad talks about the time from the signup request to the namespace/user creation. Which basically means a combination of UserSignup complete status AND Space & Mur Ready statuses.
The UserSignup.Status.Complete doesn't necessarily mean everything has been provisioned.

[MatousJobanek]

Code-wise looks good but this metric measures the time between the Signup request (UserSignupRequestReceivedTimeAnnotationKey which I guess will be added later in a separate PR?) and the UserSingup set to the Complete status.

I guess that you already noticed the other PR in the reg-service: codeready-toolchain/registration-service#515

Which basically means a combination of UserSignup complete status AND Space & Mur Ready statuses.
The UserSignup.Status.Complete doesn't necessarily mean everything has been provisioned.

Look at this part of the code:

host-operator/controllers/usersignup/usersignup_controller.go

Lines 375 to 380 in ec07825

	// if the Space is not Ready, another reconcile will occur when space status is updated since this controller watches space (without a predicate)
	logger.Info("Checking whether Space is Ready", "Space", space.Name)
	if !condition.IsTrueWithReason(space.Status.Conditions, toolchainv1alpha1.ConditionReady, toolchainv1alpha1.SpaceProvisionedReason) &&
	!condition.IsTrue(userSignup.Status.Conditions, toolchainv1alpha1.UserSignupComplete) {
	return true, r.updateIncompleteStatus(ctx, userSignup, fmt.Sprintf("space %s was not ready", space.Name))
	}

it actually waits until the Space is ready and provisioned, so this part is covered. Since MUR is created before and acts immediately on the presence of SpaceBinding and Space.Spec.TargetCluster, we could assume that it should be ready before the Space, but it's true that we don't check it explicitly. I added the check/wait here 84c6983

[mfrancisc]

Nice job 👍

I have only few cosmetic comments.

[mfrancisc]

minor - should we delete at the end? once the provisionTimeSeconds was computed. Just in case something goes wrong in the middle.

[MatousJobanek]

Yeah, I was thinking about it too. For the simplicity, I decided to delete it and log the information. In this case, the presence of the annotation says if the given UserSignup was already recorded or not. If we would want to keep it, then we would need to add another field into UserSignup.Status to store the provision time.
Both options are valid, I decided to go with the simpler one. We can always change it later.
But if there would be more people asking for having the information stored in UserSignup, then I can do it in this PR.

[mfrancisc]

Sorry , I was just wondering if we should move those lines after line 427, not keeping the information. Just move it as last thing in the function.

[MatousJobanek]

ah, gotcha.
I used this order intentionally to ensure that the provision time for the particular UserSignup will be recorded only once. If the update fails (because of a conflict) then the controller requeue a new reconcile and try to remove the annotation again. With this approach, the provision time will be recorded only when the removal of the annotation is successful. If the removal was at the end of the function and the update failed, then we would have two records for the same UserSignup.

[mfrancisc]

gotcha, makes sense! thanks for explaining that!

I was thinking about the case in which fails after the update and before the recording ( basically parsing the value ). But I think that is tolerated, more than having duplicated recordings.

[MatousJobanek]

If it fails for parsing the value, then there is some problem between reg-service and host-operator and we should only log the error and not return it nor try to parse it again. The resulting duration would be irrelevant and we shouldn't block the UserSignup because of some broken telemetry data.

[alexeykazakov]

it actually waits until the Space is ready and provisioned, so this part is covered. Since MUR is created before and acts immediately on the presence of SpaceBinding and Space.Spec.TargetCluster, we could assume that it should be ready before the Space, but it's true that we don't check it explicitly. I added the check/wait here

OK. I missed when we started waiting for the Space to be ready. And with this change (wait for MUR) we can clean up our reg-service a bit: https://github.com/codeready-toolchain/registration-service/blob/60b898ffd19907210b19e9a43f0b26facabb1583/pkg/signup/service/signup_service.go#L398-L399
It still assumes that MUR can be NotReady when UserSignup is Complete. But it's minor/cosmetic and we can do it later.

[alexeykazakov]

Looks good!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.6% Duplication on New Code

See analysis details on SonarQube Cloud

[mfrancisc]

Nice Job 🚀

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [MatousJobanek,alexeykazakov,mfrancisc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[MatousJobanek]

/retest
fix in e2e tests

[codecov]

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79.71%. Comparing base (d291504) to head (612e006).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
controllers/usersignup/usersignup_controller.go	83.78%	4 Missing and 2 partials ⚠️

❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
+ Coverage   79.67%   79.71%   +0.03%     
==========================================
  Files          82       82              
  Lines        8154     8203      +49     
==========================================
+ Hits         6497     6539      +42     
- Misses       1466     1471       +5     
- Partials      191      193       +2     

Files with missing lines	Coverage Δ
pkg/metrics/metrics.go	100.00% <100.00%> (ø)
test/usersignup_assertions.go	97.95% <100.00%> (ø)
controllers/usersignup/usersignup_controller.go	75.57% <83.78%> (+0.42%)	⬆️

... and 1 file with indirect coverage changes