feat(site): add agent connection timings #15276

BrunoQuaresma · 2024-10-29T20:04:13Z

Local preview:

Close coder/internal#116

…nt-timings

BrunoQuaresma · 2024-10-29T20:04:36Z

@dannykopping I would appreciate a first round of review in the BE code and its usage.

dannykopping

Overall looking good!

Added a few notes for things we can do to harden.

coderd/database/dbmem/dbmem.go

coderd/workspacebuilds.go

coderd/workspacebuilds_test.go

dannykopping · 2024-10-29T22:40:48Z

coderd/workspacebuilds_test.go

 		require.Empty(t, res.AgentScriptTimings)
 	})
+
+	//nolint:paralleltest


Howcome these are not in parallel?

All mentions of nolint should be followed with a reason like:

//nolint:paralleltest // Reason.

I'm just following what we are already doing in other places:

coder/coderd/workspacebuilds_test.go

Line 1186 in 3e1170c

//nolint:paralleltest

coder/coderd/workspacebuilds_test.go

Line 1240 in 3e1170c

//nolint:paralleltest

coder/coderd/workspacebuilds_test.go

Line 1255 in 3e1170c

//nolint:paralleltest

All of them have the same reason based on this PR even thinking I had fixed it here.

coderd/workspacebuilds_test.go

site/src/modules/workspaces/WorkspaceTiming/ResourcesChart.tsx

site/src/modules/workspaces/WorkspaceTiming/StagesChart.tsx

site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx

dannykopping · 2024-10-29T22:50:46Z

coderd/workspacebuilds.go

+			StartedAt:          agent.CreatedAt,
+			Stage:              "connect",
+			EndedAt:            agent.FirstConnectedAt.Time,


first_connected_at is a nullable field; what do we do in this case?

I'm not sure. In this case, we should not return or display the connection stage in the chart 🤔 wdyt?

Can we show some pending state in this case?

Co-authored-by: Danny Kopping <danny@coder.com>

…nt-timings

aslilac · 2024-10-31T16:37:41Z

.vscode/settings.json

+	},
+	"[typescriptreact]": {
+		"editor.defaultFormatter": "biomejs.biome"


look a couple lines above, we already set this

I don't know why my vscode didn't recognize that

aslilac · 2024-10-31T16:41:01Z

site/src/modules/workspaces/WorkspaceTiming/StagesChart.tsx

-											key={stage.name}
-											id={encodeURIComponent(stage.name)}
-										>
+									{stages.map((s) => (


nit: I liked stage as a variable name here more. single letter variables names make me sad. :(

I'm working too much with Go lately 😆

aslilac · 2024-10-31T16:42:54Z

site/src/modules/workspaces/WorkspaceTiming/WorkspaceTimings.tsx

+	const stages = () => {
+		const agentStageLabels = Array.from(
+			new Set(
+				agentConnectionTimings.map((t) => `agent (${t.workspace_agent_name})`),
+			),
+		);
+		return [
+			...provisioningStages,
+			...agentStageLabels.flatMap((a) => agentStages(a)),
+		];
+	};
+


why put this in a function that doesn't take any arguments and is only called once? if it's a performance concern, we should probably be using useMemo instead

That is true. I tried to improve readability by grouping related code to a function.

aslilac · 2024-10-31T16:43:40Z

site/src/modules/workspaces/WorkspaceTiming/WorkspaceTimings.tsx

@@ -135,19 +158,18 @@ export const WorkspaceTimings: FC<WorkspaceTimingsProps> = ({
 							)}

 						{view.name === "detailed" &&
-							view.category.id === "workspaceBoot" && (
+							view.stage.section !== "provisioning" && (


this could be put in a ternary with the condition above to reduce repeated conditions

aslilac · 2024-10-31T16:44:54Z

site/src/modules/workspaces/WorkspaceTiming/WorkspaceTimings.tsx

 										.map((t) => {
 											return {
-												range: extractRange(t),
+												range: toTimeRange(t),
 												name: t.display_name,
 												status: t.status,
 												exitCode: t.exit_code,
 											};
 										})}


could be an implicit return

Suggested change

.map((t) => {

return {

range: extractRange(t),

range: toTimeRange(t),

name: t.display_name,

status: t.status,

exitCode: t.exit_code,

};

})}

.map((t) => ({

range: toTimeRange(t),

name: t.display_name,

status: t.status,

exitCode: t.exit_code,

}))

…nt-timings

dannykopping

LGTM, a few minor nits & notes but I'm happy!

dannykopping · 2024-11-01T14:46:59Z

coderd/workspacebuilds.go

+			WorkspaceAgentID:   agent.ID.String(),
+			WorkspaceAgentName: agent.Name,
+			StartedAt:          agent.CreatedAt,
+			Stage:              "connect",


I think we should add constants for these stages, and share them across coderd & the frontend.

dannykopping · 2024-11-01T14:47:41Z

coderd/workspacebuilds_test.go

@@ -1264,19 +1271,21 @@ func TestWorkspaceBuildTimings(t *testing.T) {
 		require.Empty(t, res.AgentScriptTimings)
 	})

-	//nolint:paralleltest


Why were these marked as not parallel before? I'm concerned we're rolling back a change which was necessary to avoid some previous issue.

It was a conflict with the following PRs:

fix(coderd): improve workspace tests #15069

test(coderd): fix flake on TestWorkspaceBuildTimings #15077

dannykopping · 2024-11-01T14:49:02Z

coderd/workspacebuilds_test.go

+		resource := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
+			JobID: build.JobID,
+		})
+		agents := make([]database.WorkspaceAgent, 5)


Reminder about this one

codersdk/workspacebuilds.go

dannykopping · 2024-11-01T14:52:58Z

site/src/modules/workspaces/WorkspaceTiming/StagesChart.tsx

+						<TooltipShortDescription>
+							Time taken to establish an RPC connection with the control plane.
+						</TooltipShortDescription>
+					</>
+				),
+			},
+		},
+		{
+			name: "start",
+			label: "run startup scripts",
+			section,
+			tooltip: {
+				title: (
+					<>
+						<TooltipTitle>Run startup scripts</TooltipTitle>
+						<TooltipShortDescription>
+							Time taken to run each agent startup script.
+						</TooltipShortDescription>


I wonder if we should drop the "time taken" on these stages since the other sections' stages don't have them.

Suggested change

<TooltipShortDescription>

Time taken to establish an RPC connection with the control plane.

</TooltipShortDescription>

</>

),

},

},

{

name: "start",

label: "run startup scripts",

section,

tooltip: {

title: (

<>

<TooltipTitle>Run startup scripts</TooltipTitle>

<TooltipShortDescription>

Time taken to run each agent startup script.

</TooltipShortDescription>

<TooltipShortDescription>

Establish an RPC connection with the control plane.

</TooltipShortDescription>

</>

),

},

},

{

name: "start",

label: "run startup scripts",

section,

tooltip: {

title: (

<>

<TooltipTitle>Run startup scripts</TooltipTitle>

<TooltipShortDescription>

Execute each agent startup script.

</TooltipShortDescription>

dannykopping · 2024-11-01T14:55:51Z

site/src/modules/workspaces/WorkspaceTiming/WorkspaceTimings.stories.tsx

@@ -11,6 +11,7 @@ const meta: Meta<typeof WorkspaceTimings> = {
 		defaultIsOpen: true,
 		provisionerTimings: WorkspaceTimingsResponse.provisioner_timings,
 		agentScriptTimings: WorkspaceTimingsResponse.agent_script_timings,
+		agentConnectionTimings: WorkspaceTimingsResponse.agent_connection_timings,


Do we need some frontend tests for these timings?

Not specifically, if you see the stories, they are already represented there. If the component does not display for any reason, Chromatic will issue a warning.

johnstcn

LGTM 👍

BrunoQuaresma added 6 commits October 25, 2024 16:14

Add agent ID and name on agent script timing response

8f15ca1

Add agent connection timings to the response

8ac895f

Remove unused modules

124d149

Merge branch 'main' of https://github.com/coder/coder into bq/add-age…

0ff2be9

…nt-timings

Display agent timings

d00a00b

Display agent connection timings

69b181c

BrunoQuaresma requested a review from dannykopping October 29, 2024 20:04

BrunoQuaresma self-assigned this Oct 29, 2024

dannykopping reviewed Oct 29, 2024

View reviewed changes

BrunoQuaresma and others added 7 commits October 30, 2024 10:37

Apply suggestions from code review

3e1170c

Co-authored-by: Danny Kopping <danny@coder.com>

Fix fmt

ab8792f

Improve tests

0b685a9

Merge branch 'main' of https://github.com/coder/coder into bq/add-age…

709b669

…nt-timings

Return error if agent or script are not found

d9fc453

Change the agent stage label to agent ()

1bed074

Fix storybook

4bd6538

BrunoQuaresma requested review from dannykopping, a team and aslilac and removed request for a team October 31, 2024 13:23

BrunoQuaresma marked this pull request as ready for review October 31, 2024 13:31

aslilac approved these changes Oct 31, 2024

View reviewed changes

BrunoQuaresma added 5 commits November 1, 2024 13:16

Merge branch 'main' of https://github.com/coder/coder into bq/add-age…

c368a57

…nt-timings

Remove duplicate vscode config

f59a87d

Apply PR suggested changes

91be0dc

Fix chart navigation

044f6e5

Fix fmt

e8bbecd

dannykopping approved these changes Nov 1, 2024

View reviewed changes

johnstcn approved these changes Nov 1, 2024

View reviewed changes

BrunoQuaresma added 2 commits November 1, 2024 16:08

Apply PR reviews

e1a61d8

Fix test

ce6f507

BrunoQuaresma merged commit e232aee into main Nov 1, 2024
30 checks passed

BrunoQuaresma deleted the bq/add-agent-timings branch November 1, 2024 16:29

github-actions bot locked and limited conversation to collaborators Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(site): add agent connection timings #15276

feat(site): add agent connection timings #15276

BrunoQuaresma commented Oct 29, 2024

BrunoQuaresma commented Oct 29, 2024

dannykopping left a comment

dannykopping Oct 29, 2024

BrunoQuaresma Oct 30, 2024

dannykopping Oct 29, 2024

BrunoQuaresma Oct 30, 2024

dannykopping Oct 30, 2024

aslilac Oct 31, 2024

BrunoQuaresma Nov 1, 2024

aslilac Oct 31, 2024

BrunoQuaresma Nov 1, 2024

aslilac Oct 31, 2024

BrunoQuaresma Nov 1, 2024

aslilac Oct 31, 2024

aslilac Oct 31, 2024

dannykopping left a comment

dannykopping Nov 1, 2024

dannykopping Nov 1, 2024

BrunoQuaresma Nov 1, 2024

dannykopping Nov 1, 2024

dannykopping Nov 1, 2024

dannykopping Nov 1, 2024

BrunoQuaresma Nov 1, 2024

johnstcn left a comment

feat(site): add agent connection timings #15276

feat(site): add agent connection timings #15276

Conversation

BrunoQuaresma commented Oct 29, 2024

BrunoQuaresma commented Oct 29, 2024

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment