- Notifications
You must be signed in to change notification settings - Fork927
fix(coderd): ensure agent timings are non-zero on insert#18065
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
small non-blocking comment but otherwise looks good to me
Uh oh!
There was an error while loading.Please reload this page.
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.
I think the extra validation is good, but this fix doesn't seem to address the issue raised in#15432?
| ) | ||
| continue | ||
| } | ||
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 issue referenced in#15432 originates from thecodersdk.TimingStageConnect timing that's further down, but that one doesn't seem addressed?
Theagent.FirstConnectedAt.Time is used unconditionally, but may be null. Do we have a way to represent this better?
We could always omit the entry, but personally I'd like to see it in the graph as an unterminated starting point, would give an indication we're waiting for something.
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.
We could represent both of these ascodersdk.NullTime instead.
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 turns out the timings chart is built to render with a "loading" state until all of provisioner timings, script timings, and connection timings are populated with at least one value. The assumption here is that timings are only useful after a build has completed.
My previous change (#18058) adds a check in the UI so that causes a time range with a zero time to be rendered as an "instant" with an "Invalid" label.
I think that, for now, it makes sense to simply exclude the agent connection timing if!FirstConnectedAt.Valid. Does that sound alright?
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.
I'm fine with that if it can show up later if the agent finally connects 👍🏻
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.
Small suggestion, but otherwise LGTM!
Uh oh!
There was an error while loading.Please reload this page.
776c144 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Relates to#15432
I checked in our dogfood database and couldn't find any instances of timings where
started_atorended_atwas the zero time. However it makes sense to me to ensure this can't ever happen.