From 6dd51f92fbd6132ea4dc1d9c541c322cf2d4effc Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 27 Feb 2025 10:43:51 +0100 Subject: [PATCH] chore: test metricscache on postgres (#16711) metricscache_test has been running tests against dbmem only, instead of against postgres. Unfortunately the implementations of GetTemplateAverageBuildTime have diverged between dbmem and postgres. This change gets the tests working on Postgres and test for the behaviour postgres provides. --- coderd/coderd.go | 1 + coderd/database/dbmem/dbmem.go | 36 +++--- coderd/database/queries.sql.go | 12 +- coderd/database/queries/workspaces.sql | 12 +- coderd/metricscache/metricscache.go | 13 +- coderd/metricscache/metricscache_test.go | 148 +++++++++++++---------- 6 files changed, 126 insertions(+), 96 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 1cb4c0592b..d4c948e346 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -422,6 +422,7 @@ func New(options *Options) *API { metricsCache := metricscache.New( options.Database, options.Logger.Named("metrics_cache"), + options.Clock, metricscache.Intervals{ TemplateBuildTimes: options.MetricsCacheRefreshInterval, DeploymentStats: options.AgentStatsRefreshInterval, diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 23913a55bf..6fbafa562d 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -269,7 +269,7 @@ type data struct { presetParameters []database.TemplateVersionPresetParameter } -func tryPercentile(fs []float64, p float64) float64 { +func tryPercentileCont(fs []float64, p float64) float64 { if len(fs) == 0 { return -1 } @@ -282,6 +282,14 @@ func tryPercentile(fs []float64, p float64) float64 { return fs[lower] + (fs[upper]-fs[lower])*(pos-float64(lower)) } +func tryPercentileDisc(fs []float64, p float64) float64 { + if len(fs) == 0 { + return -1 + } + sort.Float64s(fs) + return fs[max(int(math.Ceil(float64(len(fs))*p/100-1)), 0)] +} + func validateDatabaseTypeWithValid(v reflect.Value) (handled bool, err error) { if v.Kind() == reflect.Struct { return false, nil @@ -2790,8 +2798,8 @@ func (q *FakeQuerier) GetDeploymentWorkspaceAgentStats(_ context.Context, create latencies = append(latencies, agentStat.ConnectionMedianLatencyMS) } - stat.WorkspaceConnectionLatency50 = tryPercentile(latencies, 50) - stat.WorkspaceConnectionLatency95 = tryPercentile(latencies, 95) + stat.WorkspaceConnectionLatency50 = tryPercentileCont(latencies, 50) + stat.WorkspaceConnectionLatency95 = tryPercentileCont(latencies, 95) return stat, nil } @@ -2839,8 +2847,8 @@ func (q *FakeQuerier) GetDeploymentWorkspaceAgentUsageStats(_ context.Context, c stat.WorkspaceTxBytes += agentStat.TxBytes latencies = append(latencies, agentStat.ConnectionMedianLatencyMS) } - stat.WorkspaceConnectionLatency50 = tryPercentile(latencies, 50) - stat.WorkspaceConnectionLatency95 = tryPercentile(latencies, 95) + stat.WorkspaceConnectionLatency50 = tryPercentileCont(latencies, 50) + stat.WorkspaceConnectionLatency95 = tryPercentileCont(latencies, 95) for _, agentStat := range sessions { stat.SessionCountVSCode += agentStat.SessionCountVSCode @@ -4987,9 +4995,9 @@ func (q *FakeQuerier) GetTemplateAverageBuildTime(ctx context.Context, arg datab } var row database.GetTemplateAverageBuildTimeRow - row.Delete50, row.Delete95 = tryPercentile(deleteTimes, 50), tryPercentile(deleteTimes, 95) - row.Stop50, row.Stop95 = tryPercentile(stopTimes, 50), tryPercentile(stopTimes, 95) - row.Start50, row.Start95 = tryPercentile(startTimes, 50), tryPercentile(startTimes, 95) + row.Delete50, row.Delete95 = tryPercentileDisc(deleteTimes, 50), tryPercentileDisc(deleteTimes, 95) + row.Stop50, row.Stop95 = tryPercentileDisc(stopTimes, 50), tryPercentileDisc(stopTimes, 95) + row.Start50, row.Start95 = tryPercentileDisc(startTimes, 50), tryPercentileDisc(startTimes, 95) return row, nil } @@ -6024,8 +6032,8 @@ func (q *FakeQuerier) GetUserLatencyInsights(_ context.Context, arg database.Get Username: user.Username, AvatarURL: user.AvatarURL, TemplateIDs: seenTemplatesByUserID[userID], - WorkspaceConnectionLatency50: tryPercentile(latencies, 50), - WorkspaceConnectionLatency95: tryPercentile(latencies, 95), + WorkspaceConnectionLatency50: tryPercentileCont(latencies, 50), + WorkspaceConnectionLatency95: tryPercentileCont(latencies, 95), } rows = append(rows, row) } @@ -6669,8 +6677,8 @@ func (q *FakeQuerier) GetWorkspaceAgentStats(_ context.Context, createdAfter tim if !ok { continue } - stat.WorkspaceConnectionLatency50 = tryPercentile(latencies, 50) - stat.WorkspaceConnectionLatency95 = tryPercentile(latencies, 95) + stat.WorkspaceConnectionLatency50 = tryPercentileCont(latencies, 50) + stat.WorkspaceConnectionLatency95 = tryPercentileCont(latencies, 95) statByAgent[stat.AgentID] = stat } @@ -6807,8 +6815,8 @@ func (q *FakeQuerier) GetWorkspaceAgentUsageStats(_ context.Context, createdAt t for key, latencies := range latestAgentLatencies { val, ok := latestAgentStats[key] if ok { - val.WorkspaceConnectionLatency50 = tryPercentile(latencies, 50) - val.WorkspaceConnectionLatency95 = tryPercentile(latencies, 95) + val.WorkspaceConnectionLatency50 = tryPercentileCont(latencies, 50) + val.WorkspaceConnectionLatency95 = tryPercentileCont(latencies, 95) } latestAgentStats[key] = val } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 9c9ead1b67..779bbf4b47 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -16253,13 +16253,11 @@ func (q *sqlQuerier) GetWorkspaceByWorkspaceAppID(ctx context.Context, workspace } const getWorkspaceUniqueOwnerCountByTemplateIDs = `-- name: GetWorkspaceUniqueOwnerCountByTemplateIDs :many -SELECT - template_id, COUNT(DISTINCT owner_id) AS unique_owners_sum -FROM - workspaces -WHERE - template_id = ANY($1 :: uuid[]) AND deleted = false -GROUP BY template_id +SELECT templates.id AS template_id, COUNT(DISTINCT workspaces.owner_id) AS unique_owners_sum +FROM templates +LEFT JOIN workspaces ON workspaces.template_id = templates.id AND workspaces.deleted = false +WHERE templates.id = ANY($1 :: uuid[]) +GROUP BY templates.id ` type GetWorkspaceUniqueOwnerCountByTemplateIDsRow struct { diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index cb0d11e8a8..4ec74c066f 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -415,13 +415,11 @@ WHERE ORDER BY created_at DESC; -- name: GetWorkspaceUniqueOwnerCountByTemplateIDs :many -SELECT - template_id, COUNT(DISTINCT owner_id) AS unique_owners_sum -FROM - workspaces -WHERE - template_id = ANY(@template_ids :: uuid[]) AND deleted = false -GROUP BY template_id; +SELECT templates.id AS template_id, COUNT(DISTINCT workspaces.owner_id) AS unique_owners_sum +FROM templates +LEFT JOIN workspaces ON workspaces.template_id = templates.id AND workspaces.deleted = false +WHERE templates.id = ANY(@template_ids :: uuid[]) +GROUP BY templates.id; -- name: InsertWorkspace :one INSERT INTO diff --git a/coderd/metricscache/metricscache.go b/coderd/metricscache/metricscache.go index 3452ef2cce..9a18400c8d 100644 --- a/coderd/metricscache/metricscache.go +++ b/coderd/metricscache/metricscache.go @@ -15,6 +15,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/codersdk" + "github.com/coder/quartz" "github.com/coder/retry" ) @@ -26,6 +27,7 @@ import ( type Cache struct { database database.Store log slog.Logger + clock quartz.Clock intervals Intervals templateWorkspaceOwners atomic.Pointer[map[uuid.UUID]int] @@ -45,7 +47,7 @@ type Intervals struct { DeploymentStats time.Duration } -func New(db database.Store, log slog.Logger, intervals Intervals, usage bool) *Cache { +func New(db database.Store, log slog.Logger, clock quartz.Clock, intervals Intervals, usage bool) *Cache { if intervals.TemplateBuildTimes <= 0 { intervals.TemplateBuildTimes = time.Hour } @@ -55,6 +57,7 @@ func New(db database.Store, log slog.Logger, intervals Intervals, usage bool) *C ctx, cancel := context.WithCancel(context.Background()) c := &Cache{ + clock: clock, database: db, intervals: intervals, log: log, @@ -104,7 +107,7 @@ func (c *Cache) refreshTemplateBuildTimes(ctx context.Context) error { Valid: true, }, StartTime: sql.NullTime{ - Time: dbtime.Time(time.Now().AddDate(0, 0, -30)), + Time: dbtime.Time(c.clock.Now().AddDate(0, 0, -30)), Valid: true, }, }) @@ -131,7 +134,7 @@ func (c *Cache) refreshTemplateBuildTimes(ctx context.Context) error { func (c *Cache) refreshDeploymentStats(ctx context.Context) error { var ( - from = dbtime.Now().Add(-15 * time.Minute) + from = c.clock.Now().Add(-15 * time.Minute) agentStats database.GetDeploymentWorkspaceAgentStatsRow err error ) @@ -155,8 +158,8 @@ func (c *Cache) refreshDeploymentStats(ctx context.Context) error { } c.deploymentStatsResponse.Store(&codersdk.DeploymentStats{ AggregatedFrom: from, - CollectedAt: dbtime.Now(), - NextUpdateAt: dbtime.Now().Add(c.intervals.DeploymentStats), + CollectedAt: dbtime.Time(c.clock.Now()), + NextUpdateAt: dbtime.Time(c.clock.Now().Add(c.intervals.DeploymentStats)), Workspaces: codersdk.WorkspaceDeploymentStats{ Pending: workspaceStats.PendingWorkspaces, Building: workspaceStats.BuildingWorkspaces, diff --git a/coderd/metricscache/metricscache_test.go b/coderd/metricscache/metricscache_test.go index 24b22d012c..b825bc6454 100644 --- a/coderd/metricscache/metricscache_test.go +++ b/coderd/metricscache/metricscache_test.go @@ -4,42 +4,68 @@ import ( "context" "database/sql" "encoding/json" + "sync/atomic" "testing" "time" "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" + "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbgen" - "github.com/coder/coder/v2/coderd/database/dbmem" - "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/metricscache" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" + "github.com/coder/quartz" ) func date(year, month, day int) time.Time { return time.Date(year, time.Month(month), day, 0, 0, 0, 0, time.UTC) } +func newMetricsCache(t *testing.T, log slog.Logger, clock quartz.Clock, intervals metricscache.Intervals, usage bool) (*metricscache.Cache, database.Store) { + t.Helper() + + accessControlStore := &atomic.Pointer[dbauthz.AccessControlStore]{} + var acs dbauthz.AccessControlStore = dbauthz.AGPLTemplateAccessControlStore{} + accessControlStore.Store(&acs) + + var ( + auth = rbac.NewStrictCachingAuthorizer(prometheus.NewRegistry()) + db, _ = dbtestutil.NewDB(t) + dbauth = dbauthz.New(db, auth, log, accessControlStore) + cache = metricscache.New(dbauth, log, clock, intervals, usage) + ) + + t.Cleanup(func() { cache.Close() }) + + return cache, db +} + func TestCache_TemplateWorkspaceOwners(t *testing.T) { t.Parallel() var () var ( - db = dbmem.New() - cache = metricscache.New(db, testutil.Logger(t), metricscache.Intervals{ + log = testutil.Logger(t) + clock = quartz.NewReal() + cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{ TemplateBuildTimes: testutil.IntervalFast, }, false) ) - defer cache.Close() - + org := dbgen.Organization(t, db, database.Organization{}) user1 := dbgen.User(t, db, database.User{}) user2 := dbgen.User(t, db, database.User{}) template := dbgen.Template(t, db, database.Template{ - Provisioner: database.ProvisionerTypeEcho, + OrganizationID: org.ID, + Provisioner: database.ProvisionerTypeEcho, + CreatedBy: user1.ID, }) require.Eventuallyf(t, func() bool { count, ok := cache.TemplateWorkspaceOwners(template.ID) @@ -49,8 +75,9 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) { ) dbgen.Workspace(t, db, database.WorkspaceTable{ - TemplateID: template.ID, - OwnerID: user1.ID, + OrganizationID: org.ID, + TemplateID: template.ID, + OwnerID: user1.ID, }) require.Eventuallyf(t, func() bool { @@ -61,8 +88,9 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) { ) workspace2 := dbgen.Workspace(t, db, database.WorkspaceTable{ - TemplateID: template.ID, - OwnerID: user2.ID, + OrganizationID: org.ID, + TemplateID: template.ID, + OwnerID: user2.ID, }) require.Eventuallyf(t, func() bool { @@ -74,8 +102,9 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) { // 3rd workspace should not be counted since we have the same owner as workspace2. dbgen.Workspace(t, db, database.WorkspaceTable{ - TemplateID: template.ID, - OwnerID: user1.ID, + OrganizationID: org.ID, + TemplateID: template.ID, + OwnerID: user1.ID, }) db.UpdateWorkspaceDeletedByID(context.Background(), database.UpdateWorkspaceDeletedByIDParams{ @@ -149,7 +178,7 @@ func TestCache_BuildTime(t *testing.T) { }, }, transition: database.WorkspaceTransitionStop, - }, want{30 * 1000, true}, + }, want{10 * 1000, true}, }, { "three/delete", args{ @@ -176,67 +205,57 @@ func TestCache_BuildTime(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - ctx := context.Background() var ( - db = dbmem.New() - cache = metricscache.New(db, testutil.Logger(t), metricscache.Intervals{ + log = testutil.Logger(t) + clock = quartz.NewMock(t) + cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{ TemplateBuildTimes: testutil.IntervalFast, }, false) ) - defer cache.Close() + clock.Set(someDay) - id := uuid.New() - err := db.InsertTemplate(ctx, database.InsertTemplateParams{ - ID: id, - Provisioner: database.ProvisionerTypeEcho, - MaxPortSharingLevel: database.AppSharingLevelOwner, - }) - require.NoError(t, err) - template, err := db.GetTemplateByID(ctx, id) - require.NoError(t, err) + org := dbgen.Organization(t, db, database.Organization{}) + user := dbgen.User(t, db, database.User{}) - templateVersionID := uuid.New() - err = db.InsertTemplateVersion(ctx, database.InsertTemplateVersionParams{ - ID: templateVersionID, - TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + template := dbgen.Template(t, db, database.Template{ + CreatedBy: user.ID, + OrganizationID: org.ID, + }) + + templateVersion := dbgen.TemplateVersion(t, db, database.TemplateVersion{ + OrganizationID: org.ID, + CreatedBy: user.ID, + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + }) + + workspace := dbgen.Workspace(t, db, database.WorkspaceTable{ + OrganizationID: org.ID, + OwnerID: user.ID, + TemplateID: template.ID, }) - require.NoError(t, err) gotStats := cache.TemplateBuildTimeStats(template.ID) requireBuildTimeStatsEmpty(t, gotStats) - for _, row := range tt.args.rows { - _, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ - ID: uuid.New(), - Provisioner: database.ProvisionerTypeEcho, - StorageMethod: database.ProvisionerStorageMethodFile, - Type: database.ProvisionerJobTypeWorkspaceBuild, + for buildNumber, row := range tt.args.rows { + job := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: user.ID, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StartedAt: sql.NullTime{Time: row.startedAt, Valid: true}, + CompletedAt: sql.NullTime{Time: row.completedAt, Valid: true}, }) - require.NoError(t, err) - job, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ - StartedAt: sql.NullTime{Time: row.startedAt, Valid: true}, - Types: []database.ProvisionerType{ - database.ProvisionerTypeEcho, - }, - }) - require.NoError(t, err) - - err = db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{ - TemplateVersionID: templateVersionID, + dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + BuildNumber: int32(1 + buildNumber), + WorkspaceID: workspace.ID, + InitiatorID: user.ID, + TemplateVersionID: templateVersion.ID, JobID: job.ID, Transition: tt.args.transition, - Reason: database.BuildReasonInitiator, }) - require.NoError(t, err) - - err = db.UpdateProvisionerJobWithCompleteByID(ctx, database.UpdateProvisionerJobWithCompleteByIDParams{ - ID: job.ID, - CompletedAt: sql.NullTime{Time: row.completedAt, Valid: true}, - }) - require.NoError(t, err) } if tt.want.loads { @@ -274,15 +293,18 @@ func TestCache_BuildTime(t *testing.T) { func TestCache_DeploymentStats(t *testing.T) { t.Parallel() - db := dbmem.New() - cache := metricscache.New(db, testutil.Logger(t), metricscache.Intervals{ - DeploymentStats: testutil.IntervalFast, - }, false) - defer cache.Close() + + var ( + log = testutil.Logger(t) + clock = quartz.NewMock(t) + cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{ + DeploymentStats: testutil.IntervalFast, + }, false) + ) err := db.InsertWorkspaceAgentStats(context.Background(), database.InsertWorkspaceAgentStatsParams{ ID: []uuid.UUID{uuid.New()}, - CreatedAt: []time.Time{dbtime.Now()}, + CreatedAt: []time.Time{clock.Now()}, WorkspaceID: []uuid.UUID{uuid.New()}, UserID: []uuid.UUID{uuid.New()}, TemplateID: []uuid.UUID{uuid.New()},