-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial implementation #1
Conversation
18e50cc
to
e1f5fe6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
One architectural question (actually maybe you already explained that to me and I just forgot):
Why do you track the transitioning states as an annotation and only observe them in the histogram after completion. Wouldn't it be easier to just observe them directly when the state changes?
if hasScheduledTime { | ||
usm.WithLabelValues(metricCompletedLabel).Observe(scheduledTime.Sub(createdTime).Seconds()) | ||
} else if hasAcknowledgedTime { | ||
usm.WithLabelValues(metricCompletedLabel).Observe(acknowledgedTime.Sub(createdTime).Seconds()) | ||
} else if hasWaitingTime { | ||
usm.WithLabelValues(metricCompletedLabel).Observe(waitingTime.Sub(createdTime).Seconds()) | ||
} else if timedOut { | ||
usm.WithLabelValues(metricTimedOutLabel).Observe(time.Since(createdTime).Seconds()) | ||
} | ||
|
||
uam := podTimeUntilAcknowledged.MustCurryWith(metricLabels) | ||
if hasAcknowledgedTime { | ||
uam.WithLabelValues(metricCompletedLabel).Observe(acknowledgedTime.Sub(createdTime).Seconds()) | ||
} else if hasWaitingTime { | ||
uam.WithLabelValues(metricCompletedLabel).Observe(waitingTime.Sub(createdTime).Seconds()) | ||
} else if timedOut { | ||
uam.WithLabelValues(metricTimedOutLabel).Observe(time.Since(createdTime).Seconds()) | ||
} | ||
|
||
ptuw := podTimeUntilWaiting.MustCurryWith(metricLabels) | ||
if hasWaitingTime { | ||
ptuw.WithLabelValues(metricCompletedLabel).Observe(waitingTime.Sub(createdTime).Seconds()) | ||
} else if timedOut { | ||
ptuw.WithLabelValues(metricTimedOutLabel).Observe(time.Since(createdTime).Seconds()) | ||
} else { | ||
return fmt.Errorf("pod has no waiting time") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: These might be a bit more readable if combined into single switch statement instead of spreading it out into three if elseif blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it kinda melts my brain a bit atm how this switch would look 😅 i'll try reformatting with some additional tests when i have time this afternoon
Sometimes state changes happen too fast to track and the event is missed. I use the annotation to save which changes I've seen and which not. Otherwise I would update metrics twice. |
Ah I think I get what you mean now. If you miss the scheduled event and directly to acknowledged you can't know if you already observed the schedule event so you can't observe it then. Makes sense. |
Implements a controller which scheduled canary pods and measures their execution times.
Under the hood there are two controllers. One creates canary pods defined by an interval. The other controller reconciles pods with an annotation from the first controller.
State change times are tracked in an annotation and calculated after pod completion or if the pod times out in some step. Completed and timed out pods are deleted after calculating the metrics.