Skip to content

Commit 15282bb

Browse files
dannykoppingdannykopping
committed
Review comments
Signed-off-by: Danny Kopping <danny@coder.com>
1 parent 0d68e69 commit 15282bb

File tree

8 files changed

+33
-30
lines changed

8 files changed

+33
-30
lines changed

‎coderd/database/dbtime/dbtime.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ func Now() time.Time {
99

1010
// Time returns a time compatible with Postgres. Postgres only stores dates with
1111
// microsecond precision.
12+
// FIXME(dannyk): refactor all calls to Time() to expect the input time to be modified to UTC; there are currently a
13+
// few calls whose behaviour would change subtly.
14+
// See https://github.com/coder/coder/pull/14274#discussion_r1718427461
1215
func Time(t time.Time) time.Time {
13-
return t.UTC().Round(time.Microsecond)
16+
return t.Round(time.Microsecond)
1417
}

‎coderd/database/queries.sql.go

Lines changed: 6 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/provisionerjobs.sql

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -149,10 +149,10 @@ WHERE
149149
INSERT INTO provisioner_job_timings (job_id, started_at, ended_at, stage, source, action, resource)
150150
SELECT
151151
@job_id::uuid AS provisioner_job_id,
152-
unnest(@started_at::timestamptz[]) AS started_at,
153-
unnest(@ended_at::timestamptz[]) AS ended_at,
154-
unnest(@stage::provisioner_job_timing_stage[]) AS context,
155-
unnest(@source::text[]) AS context,
156-
unnest(@action::text[]) AS action,
157-
unnest(@resource::text[]) AS resource
152+
unnest(@started_at::timestamptz[]),
153+
unnest(@ended_at::timestamptz[]),
154+
unnest(@stage::provisioner_job_timing_stage[]),
155+
unnest(@source::text[]),
156+
unnest(@action::text[]),
157+
unnest(@resource::text[])
158158
RETURNING *;

‎coderd/provisionerdserver/provisionerdserver.go

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1447,12 +1447,9 @@ func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob)
14471447
JobID: jobID,
14481448
}
14491449
for _, t := range completed.GetWorkspaceBuild().GetTimings() {
1450-
var start, end time.Time
1451-
if t.Start != nil {
1452-
start = t.Start.AsTime()
1453-
}
1454-
if t.End != nil {
1455-
end = t.End.AsTime()
1450+
if t.Start == nil || t.End == nil {
1451+
s.Logger.Warn(ctx, "timings entry has nil start or end time", slog.F("entry", t.String()))
1452+
continue
14561453
}
14571454

14581455
var stg database.ProvisionerJobTimingStage
@@ -1465,8 +1462,8 @@ func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob)
14651462
params.Source = append(params.Source, t.Source)
14661463
params.Resource = append(params.Resource, t.Resource)
14671464
params.Action = append(params.Action, t.Action)
1468-
params.StartedAt = append(params.StartedAt, start)
1469-
params.EndedAt = append(params.EndedAt, end)
1465+
params.StartedAt = append(params.StartedAt, t.Start.AsTime())
1466+
params.EndedAt = append(params.EndedAt, t.End.AsTime())
14701467
}
14711468
_, err = s.Database.InsertProvisionerJobTimings(ctx, params)
14721469
if err != nil {

‎coderd/workspacebuilds.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -635,7 +635,6 @@ func (api *API) workspaceBuildState(rw http.ResponseWriter, r *http.Request) {
635635
return
636636
}
637637

638-
// TODO: why?
639638
// You must have update permissions on the template to get the state.
640639
// This matches a push!
641640
if !api.Authorize(r, policy.ActionUpdate, template.RBACObject()) {

‎provisioner/terraform/timings.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,10 @@ const (
4949
)
5050

5151
type timingAggregator struct {
52-
mu sync.Mutex
53-
5452
stage database.ProvisionerJobTimingStage
53+
54+
// Protects the stateLookup map.
55+
lookupMu sync.Mutex
5556
stateLookup map[uint64]*timingSpan
5657
}
5758

@@ -81,7 +82,7 @@ func (t *timingAggregator) ingest(ts time.Time, s *timingSpan) {
8182
}
8283

8384
s.stage = t.stage
84-
ts = dbtime.Time(ts)
85+
ts = dbtime.Time(ts.UTC())
8586

8687
switch s.kind {
8788
case timingApplyStart, timingProvisionStart, timingRefreshStart, timingInitStart, timingGraphStart:
@@ -98,21 +99,22 @@ func (t *timingAggregator) ingest(ts time.Time, s *timingSpan) {
9899
return
99100
}
100101

101-
t.mu.Lock()
102+
t.lookupMu.Lock()
102103
// Memoize this span by its unique attributes and the determined state.
103104
// This will be used in aggregate() to determine the duration of the resource action.
104105
t.stateLookup[s.hashByState(s.state)] = s
105-
t.mu.Unlock()
106+
t.lookupMu.Unlock()
106107
}
107108

108109
// aggregate performs a pass through all memoized events to build up a slice of *proto.Timing instances which represent
109110
// the total time taken to perform a certain action.
110111
// The resulting slice of *proto.Timing is NOT sorted.
111112
func (t *timingAggregator) aggregate() []*proto.Timing {
112-
t.mu.Lock()
113-
defer t.mu.Unlock()
113+
t.lookupMu.Lock()
114+
defer t.lookupMu.Unlock()
114115

115-
out := make([]*proto.Timing, 0, len(t.stateLookup))
116+
// Pre-allocate len(measurements)/2 since each timing will have one STARTED and one FAILED/COMPLETED entry.
117+
out := make([]*proto.Timing, 0, len(t.stateLookup)/2)
116118

117119
for _, s := range t.stateLookup {
118120
// We are only concerned here with failed or completed events.

‎site/.eslintignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ result
9090

9191
# Generated files shouldn't be formatted.
9292
e2e/provisionerGenerated.ts
93+
site/e2e/google/protobuf/timestampGenerated.ts
9394

9495
**/pnpm-lock.yaml
9596

‎site/.prettierignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ result
9090

9191
# Generated files shouldn't be formatted.
9292
e2e/provisionerGenerated.ts
93+
site/e2e/google/protobuf/timestampGenerated.ts
9394

9495
**/pnpm-lock.yaml
9596

0 commit comments

Comments
 (0)