- Notifications
You must be signed in to change notification settings - Fork927
feat(coderd): return agent script timings#14923
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
Add the agent script timings into the `/workspaces/:workspaceId/timings`response.
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.
Good start, but I think this needs some hardening.
coderd/database/dbmem/dbmem.go Outdated
| if slices.Contains(scriptIDs, t.ScriptID) { | ||
| var script database.WorkspaceAgentScript | ||
| for _, s := range scripts { | ||
| if s.ID == t.ScriptID { | ||
| script = s | ||
| break | ||
| } | ||
| } | ||
| rows = append(rows, database.GetWorkspaceAgentScriptTimingsByWorkspaceIDRow{ | ||
| ScriptID: t.ScriptID, | ||
| StartedAt: t.StartedAt, | ||
| EndedAt: t.EndedAt, | ||
| ExitCode: t.ExitCode, | ||
| Stage: t.Stage, | ||
| Status: t.Status, | ||
| DisplayName: script.DisplayName, | ||
| }) | ||
| } |
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.
Nit: reduce nesting by keeping the happy path unindented.
i.e.continue if the slice does not contain what you're looking for.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@dannykopping I'm not sure if I addressed all your comments, but since I made a bunch of changes, I feel it is time for a second review round. |
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.
Great work@BrunoQuaresma!
coderd/coderd.go Outdated
| @@ -1150,7 +1150,6 @@ func New(options *Options) *API { | |||
| r.Post("/", api.postWorkspaceAgentPortShare) | |||
| r.Delete("/", api.deleteWorkspaceAgentPortShare) | |||
| }) | |||
| r.Get("/timings", api.workspaceTimings) | |||
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 should check if this endpoint got included in a release. If it has, we'll have to keep it.
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.
@BrunoQuaresma In theory this is a breaking change... is there any frontend component using it?
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.
Even if there isn't a frontend using it, customers might have already started integrating with it.
coderd/database/dbmem/dbmem.go Outdated
| build, err := q.GetWorkspaceBuildByID(ctx, id) | ||
| if err != nil { | ||
| return nil, err |
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.
Please wrap these errors.
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.
👍
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.
What do you mean by " wrap these errors"?
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.
| returnnil,err | |
| returnnil,xerrors.Errorf("failed to retrieve workspace build: %w",err) |
This adds context for what might be a fairly generic error.
coderd/workspacebuilds_test.go Outdated
| // When: fetching an inexistent build | ||
| buildID := uuid.New() | ||
| _, err := client.WorkspaceBuildTimings(context.Background(), buildID) |
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.
Instead of usingcontext.Background, use a context which has a timeout.
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.
👍
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 marked some Danny's comments as important. I think the rest can be pushed in the follow-up or next iteration 👍
coderd/workspacebuilds_test.go Outdated
| // When: fetching an inexistent build | ||
| buildID := uuid.New() | ||
| _, err := client.WorkspaceBuildTimings(context.Background(), buildID) |
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.
👍
coderd/database/dbmem/dbmem.go Outdated
| build, err := q.GetWorkspaceBuildByID(ctx, id) | ||
| if err != nil { | ||
| return nil, err |
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.
👍
coderd/coderd.go Outdated
| @@ -1150,7 +1150,6 @@ func New(options *Options) *API { | |||
| r.Post("/", api.postWorkspaceAgentPortShare) | |||
| r.Delete("/", api.deleteWorkspaceAgentPortShare) | |||
| }) | |||
| r.Get("/timings", api.workspaceTimings) | |||
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.
@BrunoQuaresma In theory this is a breaking change... is there any frontend component using it?
@mtojek there is not |
then I think we can remove it |
We have to keep in mind that users can also use the API, so let's check if it made it into a release yet - and if so we have to keep it. |
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.
Great work@BrunoQuaresma!
I think we can land this
| ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) | ||
| t.Cleanup(cancel) |
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.
You can usetestutil.Context to handle this in future, btw.
| @@ -1763,30 +1763,16 @@ func (api *API) workspaceTimings(rw http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
| provisionerTimings, err := api.Database.GetProvisionerJobTimingsByJobID(ctx, build.JobID) | |||
| if err != nil && !errors.Is(err, sql.ErrNoRows) { | |||
| timings, err := api.buildTimings(ctx, build) | |||
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.
Nice!
9c8ecb8 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Add the agent script timings into the
/workspacebuilds/:workspacebuild/timingsresponse.Close#14876