Skip to content

Commit 40769bd

Browse files
ammarioammario
committed
Use null types instead of -1 for simplicity
1 parent 0ba618d commit 40769bd

File tree

6 files changed

+75
-49
lines changed

6 files changed

+75
-49
lines changed

‎coderd/database/databasefake/databasefake.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ func (q *fakeQuerier) GetTemplateAverageBuildTime(ctx context.Context, arg datab
272272
return -1
273273
}
274274
sort.Float64s(fs)
275-
return fs[len(startTimes)/2]
275+
return fs[len(fs)/2]
276276
}
277277
var row database.GetTemplateAverageBuildTimeRow
278278
row.DeleteMedian = tryMedian(deleteTimes)

‎coderd/metricscache/metricscache.go

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -241,22 +241,32 @@ func (c *Cache) TemplateUniqueUsers(id uuid.UUID) (int, bool) {
241241
return resp, true
242242
}
243243

244-
func (c *Cache) TemplateAverageBuildTime(id uuid.UUID) (*codersdk.TemplateBuildTimeStats, bool) {
244+
func (c *Cache) TemplateBuildTimeStats(id uuid.UUID) codersdk.TemplateBuildTimeStats {
245+
var unknown codersdk.TemplateBuildTimeStats
246+
245247
m := c.templateAverageBuildTime.Load()
246248
if m == nil {
247249
// Data loading.
248-
return nil, false
250+
return unknown
249251
}
250252

251253
resp, ok := (*m)[id]
252-
if !ok || resp.StartMedian <= 0 || resp.StopMedian <= 0 {
254+
if !ok {
253255
// No data or not enough builds.
254-
return nil, false
256+
return unknown
255257
}
256258

257-
return &codersdk.TemplateBuildTimeStats{
258-
StartMillis: int64(resp.StartMedian * 1000),
259-
StopMillis: int64(resp.StopMedian * 1000),
260-
DeleteMillis: int64(resp.DeleteMedian * 1000),
261-
}, true
259+
convertMedian := func(m float64) *int64 {
260+
if m < 1 {
261+
return nil
262+
}
263+
i := int64(m * 1000)
264+
return &i
265+
}
266+
267+
return codersdk.TemplateBuildTimeStats{
268+
StartMillis: convertMedian(resp.StartMedian),
269+
StopMillis: convertMedian(resp.StopMedian),
270+
DeleteMillis: convertMedian(resp.DeleteMedian),
271+
}
262272
}

‎coderd/metricscache/metricscache_test.go

Lines changed: 42 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"time"
88

99
"github.com/google/uuid"
10+
"github.com/stretchr/testify/assert"
1011
"github.com/stretchr/testify/require"
1112

1213
"cdr.dev/slog/sloggers/slogtest"
@@ -214,27 +215,30 @@ func TestCache_BuildTime(t *testing.T) {
214215
}
215216

216217
type args struct {
217-
rows []jobParams
218+
rows []jobParams
219+
transition database.WorkspaceTransition
218220
}
219221
type want struct {
220-
buildTime time.Duration
222+
buildTimeMs int64
223+
loads bool
221224
}
222225
tests := []struct {
223226
name string
224227
args args
225228
want want
226229
}{
227-
{"empty", args{}, want{-1}},
228-
{"one", args{
230+
{"empty", args{}, want{-1, false}},
231+
{"one/start", args{
229232
rows: []jobParams{
230233
{
231234
startedAt: clockTime(someDay, 10, 1, 0),
232235
completedAt: clockTime(someDay, 10, 1, 10),
233236
},
234237
},
235-
}, want{time.Second * 10},
238+
transition: database.WorkspaceTransitionStart,
239+
}, want{10 * 1000, true},
236240
},
237-
{"two", args{
241+
{"two/stop", args{
238242
rows: []jobParams{
239243
{
240244
startedAt: clockTime(someDay, 10, 1, 0),
@@ -245,9 +249,10 @@ func TestCache_BuildTime(t *testing.T) {
245249
completedAt: clockTime(someDay, 10, 1, 50),
246250
},
247251
},
248-
}, want{time.Second * 50},
252+
transition: database.WorkspaceTransitionStop,
253+
}, want{50 * 1000, true},
249254
},
250-
{"three", args{
255+
{"three/delete", args{
251256
rows: []jobParams{
252257
{
253258
startedAt: clockTime(someDay, 10, 1, 0),
@@ -261,7 +266,8 @@ func TestCache_BuildTime(t *testing.T) {
261266
completedAt: clockTime(someDay, 10, 1, 20),
262267
},
263268
},
264-
}, want{time.Second * 20},
269+
transition: database.WorkspaceTransitionDelete,
270+
}, want{20 * 1000, true},
265271
},
266272
}
267273

@@ -289,9 +295,8 @@ func TestCache_BuildTime(t *testing.T) {
289295
})
290296
require.NoError(t, err)
291297

292-
gotBuildTime, ok := cache.TemplateAverageBuildTime(template.ID)
293-
require.False(t, ok, "template shouldn't have loaded yet")
294-
require.EqualValues(t, -1, gotBuildTime)
298+
gotStats := cache.TemplateBuildTimeStats(template.ID)
299+
require.Empty(t, gotStats, "should not have loaded yet")
295300

296301
for _, row := range tt.args.rows {
297302
_, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{
@@ -311,7 +316,7 @@ func TestCache_BuildTime(t *testing.T) {
311316
_, err = db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{
312317
TemplateVersionID: templateVersion.ID,
313318
JobID: job.ID,
314-
Transition: database.WorkspaceTransitionStart,
319+
Transition: tt.args.transition,
315320
})
316321
require.NoError(t, err)
317322

@@ -322,28 +327,38 @@ func TestCache_BuildTime(t *testing.T) {
322327
require.NoError(t, err)
323328
}
324329

325-
if tt.want.buildTime > 0 {
330+
if tt.want.loads {
326331
require.Eventuallyf(t, func() bool {
327-
_, ok := cache.TemplateAverageBuildTime(template.ID)
328-
return ok
332+
stats := cache.TemplateBuildTimeStats(template.ID)
333+
return assert.NotEmpty(t, stats)
329334
}, testutil.WaitShort, testutil.IntervalMedium,
330-
"TemplateDAUs never populated",
335+
"BuildTime never populated",
331336
)
332337

333-
gotBuildTime, ok = cache.TemplateAverageBuildTime(template.ID)
334-
require.True(t, ok)
335-
require.Equal(t, tt.want.buildTime, gotBuildTime)
338+
gotStats = cache.TemplateBuildTimeStats(template.ID)
339+
340+
if tt.args.transition == database.WorkspaceTransitionDelete {
341+
require.Nil(t, gotStats.StopMillis)
342+
require.Nil(t, gotStats.StartMillis)
343+
require.Equal(t, tt.want.buildTimeMs, *gotStats.DeleteMillis)
344+
}
345+
if tt.args.transition == database.WorkspaceTransitionStart {
346+
require.Nil(t, gotStats.StopMillis)
347+
require.Nil(t, gotStats.DeleteMillis)
348+
require.Equal(t, tt.want.buildTimeMs, *gotStats.StartMillis)
349+
}
350+
if tt.args.transition == database.WorkspaceTransitionStop {
351+
require.Nil(t, gotStats.StartMillis)
352+
require.Nil(t, gotStats.DeleteMillis)
353+
require.Equal(t, tt.want.buildTimeMs, *gotStats.StopMillis)
354+
}
336355
} else {
337356
require.Never(t, func() bool {
338-
_, ok := cache.TemplateAverageBuildTime(template.ID)
339-
return ok
357+
stats := cache.TemplateBuildTimeStats(template.ID)
358+
return !assert.Empty(t, stats)
340359
}, testutil.WaitShort/2, testutil.IntervalMedium,
341-
"TemplateDAUs never populated",
360+
"BuildTimeStats populated",
342361
)
343-
344-
gotBuildTime, ok = cache.TemplateAverageBuildTime(template.ID)
345-
require.False(t, ok)
346-
require.Less(t, gotBuildTime, time.Duration(0))
347362
}
348363
})
349364
}

‎coderd/templates.go

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -774,13 +774,7 @@ func (api *API) convertTemplate(
774774
) codersdk.Template {
775775
activeCount, _ := api.metricsCache.TemplateUniqueUsers(template.ID)
776776

777-
var averageBuildTimeMillis int64
778-
averageBuildTime, ok := api.metricsCache.TemplateAverageBuildTime(template.ID)
779-
if !ok {
780-
averageBuildTimeMillis = -1
781-
} else {
782-
averageBuildTimeMillis = int64(averageBuildTime / time.Millisecond)
783-
}
777+
buildTimeStats := api.metricsCache.TemplateBuildTimeStats(template.ID)
784778

785779
return codersdk.Template{
786780
ID: template.ID,
@@ -792,7 +786,7 @@ func (api *API) convertTemplate(
792786
ActiveVersionID: template.ActiveVersionID,
793787
WorkspaceOwnerCount: workspaceOwnerCount,
794788
ActiveUserCount: activeCount,
795-
BuildTimeStats: averageBuildTimeMillis,
789+
BuildTimeStats: buildTimeStats,
796790
Description: template.Description,
797791
Icon: template.Icon,
798792
MaxTTLMillis: time.Duration(template.MaxTtl).Milliseconds(),

‎codersdk/templates.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,9 @@ type Template struct {
3434
}
3535

3636
type TemplateBuildTimeStats struct {
37-
StartMillis int64 `json:"start_ms"`
38-
StopMillis int64 `json:"stop_ms"`
39-
DeleteMillis int64 `json:"delete_ms"`
37+
StartMillis *int64 `json:"start_ms"`
38+
StopMillis *int64 `json:"stop_ms"`
39+
DeleteMillis *int64 `json:"delete_ms"`
4040
}
4141

4242
type UpdateActiveTemplateVersion struct {

‎site/src/api/typesGenerated.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -586,7 +586,7 @@ export interface Template {
586586
readonly active_version_id: string
587587
readonly workspace_owner_count: number
588588
readonly active_user_count: number
589-
readonly average_build_time_ms: number
589+
readonly build_time_stats: TemplateBuildTimeStats
590590
readonly description: string
591591
readonly icon: string
592592
readonly max_ttl_ms: number
@@ -601,6 +601,13 @@ export interface TemplateACL {
601601
readonly group: TemplateGroup[]
602602
}
603603

604+
// From codersdk/templates.go
605+
export interface TemplateBuildTimeStats {
606+
readonly start_ms?: number
607+
readonly stop_ms?: number
608+
readonly delete_ms?: number
609+
}
610+
604611
// From codersdk/templates.go
605612
export interface TemplateDAUsResponse {
606613
readonly entries: DAUEntry[]

0 commit comments

Comments
 (0)