-
Notifications
You must be signed in to change notification settings - Fork 2
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
release/v4.1.0 #62
base: master
Are you sure you want to change the base?
release/v4.1.0 #62
Conversation
NorseGaud
commented
Jan 29, 2025
- feat: add metric to export instance age #60
- Better errors for communication/client errors on starting the exporter
feat: add metric to export instance age
better communication errors telling us what and why it failed
@gjasny , are the small changes I made ok with you? It shouldn't break your metric. It essentially uses last updated timestamp for non-Started metrics since cr_time doesn't work for those. |
src/metrics/instance_state_per.go
Outdated
var instanceTime time.Time | ||
var err error | ||
if instance.Vm.State != "Started" { | ||
instanceTime, err = time.Parse(time.RFC3339, instance.Vm.LastUpdateTime) // can't use CreationTime because it's not updated for non-started instances |
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.
@gjasny Please review
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.
The idea of the age was to see how old the oldest instance (of a certain template) is. If the timestamp that is used to calculate the age is now updated as well, the "birth date" of the instance is moving which defeats the purpose. E.g. in the case of pulling, the instance is now reported with a maximum value of 5 seconds.
If the cr_time
field is not present for the Scheduling
state (and others) we should exclude those states (or only include states which are guaranteed to have the cr_time
field). Or skip all instances that don't have the cr_time
field.
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.
On my controller even a "Scheduling" instance has the cr_time
filed:
"vm": {
"instance_id": "050823b7-ab5c-4947-77bb-06aa2a4a69d6",
"instance_state": "Scheduling",
"cr_time": "2025-01-30T07:55:07.334254871Z",
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.
@arturmelanchyk , can you check to see which states use cr_time
and which use ts
?
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.
in the most recent version of the controller (1.45.0) all instances in all states have both cr_time
and ts
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.
cr_time only set on an instance creation (in the DB) and never changed
ts gets updated from time to time due to different events, like save image, termination etc
both ts and cr_time are members of an Instance object, they do not depend on the vm (regardless if the vm has started or not)