add request-received-time annotation #515
Conversation
MatousJobanek
requested review from
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
mfrancisc,
rsoaresd,
fbm3307,
metlos and
jrosental
as code owners
| userSignup := &toolchainv1alpha1.UserSignup{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: signupcommon.EncodeUserIdentifier(ctx.GetString(context.UsernameKey)), | ||
| Namespace: configuration.Namespace(), | ||
| Annotations: map[string]string{ | ||
| toolchainv1alpha1.UserSignupVerificationCounterAnnotationKey: "0", | ||
| toolchainv1alpha1.UserSignupRequestReceivedTimeAnnotationKey: requestReceivedTime.(time.Time).Format(time.RFC3339), |
There was a problem hiding this comment.
if I understand correctly, we're not tracking the milliseconds, so is there a risk of significant loss of precision in the measures?
There was a problem hiding this comment.
see the comment in the other PR: https://github.com/codeready-toolchain/api/blob/2aa06e36874e6f5b086896a0c21be5a53ed31874/api/v1alpha1/usersignup_types.go#L47
There was a problem hiding this comment.
we can measure milliseconds or even nanoseconds, but does it really matter? can you notice that you need to wait 500millisecond more or less?
There was a problem hiding this comment.
one more thing, the buckets in the histogram are distributed by seconds anyway, so measuring milliseconds doesn't bring any benefit in distributing the numbers in the buckets.
https://github.com/codeready-toolchain/host-operator/pull/1148/files#diff-4bc91a74bc47b8467cdcd25a7e3fe1651dbe44907f478377eca31b982f444f23R145
As I see it, we should care about provision time that takes more than 10 seconds, milliseconds at this scale doesn't play any role. That's why I decided to keep it simple and use the standard RFC3339 format.
There was a problem hiding this comment.
Ok, let's see how the histogram buckets will be filled, then
Also, buckets can be configured
There was a problem hiding this comment.
this is exactly what I do in the host-operator PR, see the link that I pasted in my previous comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #515 +/- ##
==========================================
- Coverage 85.30% 85.24% -0.06%
==========================================
Files 32 32
Lines 3109 3117 +8
==========================================
+ Hits 2652 2657 +5
- Misses 371 373 +2
- Partials 86 87 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -78,6 +79,8 @@ func (s *TestSignupServiceSuite) TestSignup() { | |||
| require.True(s.T(), states.VerificationRequired(&val)) | |||
| require.Equal(s.T(), "a7b1b413c1cbddbcd19a51222ef8e20a", val.Labels[toolchainv1alpha1.UserSignupUserEmailHashLabelKey]) | |||
| require.Empty(s.T(), val.Annotations[toolchainv1alpha1.SkipAutoCreateSpaceAnnotationKey]) // skip auto create space annotation is not set by default | |||
| require.NotEmpty(s.T(), val.Annotations) | |||
| require.Equal(s.T(), requestTime.Format(time.RFC3339), val.Annotations[toolchainv1alpha1.UserSignupRequestReceivedTimeAnnotationKey]) | |||
There was a problem hiding this comment.
That assumes that the test is finished within 1s. Which should be the case in a normal test run. But at least maybe worth adding a comment? Just in case, for whatever reason, the test fails due to some delay (during test debugging for example), so it could help to understand why it failed.
There was a problem hiding this comment.
or should we just assert that the annotation is there and has a valid time.RFC3339 formatted value ?
There was a problem hiding this comment.
Not really, this test doesn't rely on the execution time anyhow - the requestTime is set once at the beginning of the suite
and the same value is set in the context
and the same value is also used for comparison
So, it doesn't matter how long the test runs, it will always pass. (I've tested it locally)
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, xcoulon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
MatousJobanek
merged commit d4f3575
into
codeready-toolchain:master
Adds request-received-time annotation to store when the request to either signup or reactivate the account was received.
This will be used by the host-operator to record the provision time in a histogram.
We cannot measure the time in registration-service, because it would create a hard dependency on the calls made by the client. Reg-service could measure it only when the user keeps the browser open so the UI keeps sending requests to reg-service. However, if the user closes (or leaves) the UI, reg-service won't be triggered to record the provision time.
depends on codeready-toolchain/api#464
SANDBOX-957