Skip to content

Commit f2eb6d5

Browse files
authored
fix: prevent emitting build duration metric for devcontainer subagents (#22929)
1 parent e7f8dfb commit f2eb6d5

2 files changed

Lines changed: 64 additions & 3 deletions

File tree

coderd/agentapi/lifecycle.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,9 +134,12 @@ func (a *LifecycleAPI) UpdateLifecycle(ctx context.Context, req *agentproto.Upda
134134
case database.WorkspaceAgentLifecycleStateReady,
135135
database.WorkspaceAgentLifecycleStateStartTimeout,
136136
database.WorkspaceAgentLifecycleStateStartError:
137-
a.emitMetricsOnce.Do(func() {
138-
a.emitBuildDurationMetric(ctx, workspaceAgent.ResourceID)
139-
})
137+
// Only emit metrics for the parent agent, this metric is not intended to measure devcontainer durations.
138+
if !workspaceAgent.ParentID.Valid {
139+
a.emitMetricsOnce.Do(func() {
140+
a.emitBuildDurationMetric(ctx, workspaceAgent.ResourceID)
141+
})
142+
}
140143
}
141144

142145
return req.Lifecycle, nil

coderd/agentapi/lifecycle_test.go

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,64 @@ func TestUpdateLifecycle(t *testing.T) {
582582
require.Equal(t, uint64(1), got.GetSampleCount())
583583
require.Equal(t, expectedDuration, got.GetSampleSum())
584584
})
585+
586+
t.Run("SubAgentDoesNotEmitMetric", func(t *testing.T) {
587+
t.Parallel()
588+
parentID := uuid.New()
589+
subAgent := database.WorkspaceAgent{
590+
ID: uuid.New(),
591+
ParentID: uuid.NullUUID{UUID: parentID, Valid: true},
592+
LifecycleState: database.WorkspaceAgentLifecycleStateStarting,
593+
StartedAt: sql.NullTime{Valid: true, Time: someTime},
594+
ReadyAt: sql.NullTime{Valid: false},
595+
}
596+
lifecycle := &agentproto.Lifecycle{
597+
State: agentproto.Lifecycle_READY,
598+
ChangedAt: timestamppb.New(now),
599+
}
600+
dbM := dbmock.NewMockStore(gomock.NewController(t))
601+
dbM.EXPECT().UpdateWorkspaceAgentLifecycleStateByID(gomock.Any(), database.UpdateWorkspaceAgentLifecycleStateByIDParams{
602+
ID: subAgent.ID,
603+
LifecycleState: database.WorkspaceAgentLifecycleStateReady,
604+
StartedAt: subAgent.StartedAt,
605+
ReadyAt: sql.NullTime{
606+
Time: now,
607+
Valid: true,
608+
},
609+
}).Return(nil)
610+
// GetWorkspaceBuildMetricsByResourceID should NOT be called
611+
// because sub-agents should be skipped before querying.
612+
reg := prometheus.NewRegistry()
613+
metrics := agentapi.NewLifecycleMetrics(reg)
614+
api := &agentapi.LifecycleAPI{
615+
AgentFn: func(ctx context.Context) (database.WorkspaceAgent, error) {
616+
return subAgent, nil
617+
},
618+
WorkspaceID: workspaceID,
619+
Database: dbM,
620+
Log: testutil.Logger(t),
621+
Metrics: metrics,
622+
PublishWorkspaceUpdateFn: nil,
623+
}
624+
resp, err := api.UpdateLifecycle(context.Background(), &agentproto.UpdateLifecycleRequest{
625+
Lifecycle: lifecycle,
626+
})
627+
require.NoError(t, err)
628+
require.Equal(t, lifecycle, resp)
629+
630+
// We don't expect the metric to be emitted for sub-agents, by default this will fail anyway but it doesn't hurt
631+
// to document the test explicitly.
632+
dbM.EXPECT().GetWorkspaceBuildMetricsByResourceID(gomock.Any(), gomock.Any()).Times(0)
633+
634+
// If we were emitting the metric we would have failed by now since it would include a call to the database that we're not expecting.
635+
pm, err := reg.Gather()
636+
require.NoError(t, err)
637+
for _, m := range pm {
638+
if m.GetName() == fullMetricName {
639+
t.Fatal("metric should not be emitted for sub-agent")
640+
}
641+
}
642+
})
585643
}
586644

587645
func TestUpdateStartup(t *testing.T) {

0 commit comments

Comments
 (0)