TemplateInstance metrics update#19133
TemplateInstance metrics update#19133openshift-merge-robot merged 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
size/L
|
@gabemontero ptal |
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
Just a minor question around the time function used wrt the core prometheus stuff ... if there is an unexpected inconsistency, let's address
if it was intentional, some comments explaining when time.Now() vs. time.Unix() are used (unless there is some commonality between those I'm forgetting ... didn't bring up the golang doc to see)
otherwise generally looks like what I remember from Jim's original pull and am assuming you transported those as is
| func (c *TemplateInstanceController) Collect(ch chan<- prometheus.Metric) { | ||
| templateInstanceCompleted.Collect(ch) | ||
|
|
||
| now := c.clock.Now() |
There was a problem hiding this comment.
I forget, was there a reason that unix epoch time was not used here?
Curious on the req here vs. what we had to the for build's active metric
There was a problem hiding this comment.
In this case, c.clock will be an instance of RealClock which has as its Now implementation:
// Now returns the current time.
func (RealClock) Now() time.Time {
return time.Now()
}
There was a problem hiding this comment.
(it was done this way to provide an abstraction for the clock implementation so tests could plug in their own clock impl)
| openshift_template_instance_completed_total{condition="Ready"} 1 | ||
| ` | ||
|
|
||
| clock := &fakeClock{now: time.Unix(0, 0)} |
There was a problem hiding this comment.
Yeah here we are using unix epoch ...
| informer: informer.Informer(), | ||
| queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "openshift_template_instance_controller"), | ||
| readinessLimiter: workqueue.NewItemFastSlowRateLimiter(5*time.Second, 20*time.Second, 200), | ||
| clock: clock.RealClock{}, |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, gabemontero The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
New changes are detected. LGTM label has been removed. |
|
@adambkaplan mildly relevant to your interests. |
5 participants
tweak of #18650