Skip to content

Commit 9cc5049

Browse files
dannykoppingdannykopping
committed
chore: clear up metrics tests
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
1 parent b539ddc commit 9cc5049

File tree

1 file changed

+61
-66
lines changed

1 file changed

+61
-66
lines changed

‎enterprise/coderd/prebuilds/metricscollector_test.go

Lines changed: 61 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/coder/coder/v2/coderd/database/dbauthz"
2020
"github.com/coder/coder/v2/coderd/database/dbgen"
2121
"github.com/coder/coder/v2/coderd/database/dbtestutil"
22+
"github.com/coder/coder/v2/coderd/database/dbtime"
2223
agplprebuilds "github.com/coder/coder/v2/coderd/prebuilds"
2324
"github.com/coder/coder/v2/codersdk"
2425
"github.com/coder/coder/v2/enterprise/coderd/prebuilds"
@@ -165,7 +166,7 @@ func TestMetricsCollector(t *testing.T) {
165166
eligible: []bool{false},
166167
},
167168
{
168-
name: "deleted templates never desire prebuilds",
169+
name: "deleted templates should not be included in exported metrics",
169170
transitions: allTransitions,
170171
jobStatuses: allJobStatuses,
171172
initiatorIDs: []uuid.UUID{agplprebuilds.SystemUserID},
@@ -174,16 +175,6 @@ func TestMetricsCollector(t *testing.T) {
174175
templateDeleted: []bool{true},
175176
eligible: []bool{false},
176177
},
177-
{
178-
name: "running prebuilds for deleted templates are still counted, so that they can be deleted",
179-
transitions: []database.WorkspaceTransition{database.WorkspaceTransitionStart},
180-
jobStatuses: []database.ProvisionerJobStatus{database.ProvisionerJobStatusSucceeded},
181-
initiatorIDs: []uuid.UUID{agplprebuilds.SystemUserID},
182-
ownerIDs: []uuid.UUID{agplprebuilds.SystemUserID},
183-
metrics: nil,
184-
templateDeleted: []bool{true},
185-
eligible: []bool{false},
186-
},
187178
}
188179
for _, test := range tests {
189180
test := test // capture for parallel
@@ -308,6 +299,9 @@ func TestMetricsCollector(t *testing.T) {
308299
}
309300
}
310301

302+
// TestMetricsCollector_DuplicateTemplateNames validates a bug that we saw previously which caused duplicate metric series
303+
// registration when a template was deleted and a new one created with the same name (and preset name).
304+
// We are now excluding deleted templates from our metric collection.
311305
func TestMetricsCollector_DuplicateTemplateNames(t *testing.T) {
312306
t.Parallel()
313307

@@ -352,81 +346,82 @@ func TestMetricsCollector_DuplicateTemplateNames(t *testing.T) {
352346
reconciler := prebuilds.NewStoreReconciler(db, pubsub, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer())
353347
ctx := testutil.Context(t, testutil.WaitLong)
354348

355-
createdUsers := []uuid.UUID{agplprebuilds.SystemUserID}
356-
for _, user := range []uuid.UUID{test.ownerID, test.initiatorID} {
357-
if !slices.Contains(createdUsers, user) {
358-
dbgen.User(t, db, database.User{
359-
ID: user,
360-
})
361-
createdUsers = append(createdUsers, user)
362-
}
363-
}
364-
365349
collector := prebuilds.NewMetricsCollector(db, logger, reconciler)
366350
registry := prometheus.NewPedanticRegistry()
367351
registry.Register(collector)
368352

369-
defaultOrgName := "default-org"
370-
defaultTemplateName := "default-template"
371-
defaultPresetName := "default-preset"
372-
defaultOrg := dbgen.Organization(t, db, database.Organization{
373-
Name: defaultOrgName,
374-
})
375-
setupTemplateWithDeps := func(templateDeleted bool) database.Template {
376-
template := setupTestDBTemplateWithinOrg(t, db, test.ownerID, templateDeleted, defaultTemplateName, defaultOrg)
353+
presetName := "default-preset"
354+
defaultOrg := dbgen.Organization(t, db, database.Organization{})
355+
setupTemplateWithDeps := func() database.Template {
356+
template := setupTestDBTemplateWithinOrg(t, db, test.ownerID, false, "default-template", defaultOrg)
377357
templateVersionID := setupTestDBTemplateVersion(ctx, t, clock, db, pubsub, defaultOrg.ID, test.ownerID, template.ID)
378-
preset := setupTestDBPreset(t, db, templateVersionID, 1, defaultPresetName)
358+
preset := setupTestDBPreset(t, db, templateVersionID, 1, "default-preset")
379359
workspace, _ := setupTestDBWorkspace(
380360
t, clock, db, pubsub,
381361
test.transition, test.jobStatus, defaultOrg.ID, preset, template.ID, templateVersionID, test.initiatorID, test.ownerID,
382362
)
383363
setupTestDBWorkspaceAgent(t, db, workspace.ID, test.eligible)
384364
return template
385365
}
386-
// Simulates creating and then deleting a template.
387-
setupTemplateWithDeps(true)
388-
// Simulates creating a template with the same org, template name, and preset name.
389-
newTemplate := setupTemplateWithDeps(false)
390366

391-
// Force an update to the metrics state to allow the collector to collect fresh metrics.
367+
// When: starting with a regular template.
368+
template := setupTemplateWithDeps()
369+
labels := map[string]string{
370+
"template_name": template.Name,
371+
"preset_name": presetName,
372+
"organization_name": defaultOrg.Name,
373+
}
374+
392375
// nolint:gocritic // Authz context needed to retrieve state.
393-
require.NoError(t, collector.UpdateState(dbauthz.AsPrebuildsOrchestrator(ctx), testutil.WaitLong))
376+
ctx = dbauthz.AsPrebuildsOrchestrator(ctx)
394377

378+
// Then: metrics collect successfully.
379+
require.NoError(t, collector.UpdateState(ctx, testutil.WaitLong))
395380
metricsFamilies, err := registry.Gather()
396381
require.NoError(t, err)
397-
398-
templateVersions, err := db.GetTemplateVersionsByTemplateID(ctx, database.GetTemplateVersionsByTemplateIDParams{
399-
TemplateID: newTemplate.ID,
400-
})
382+
require.NotEmpty(t, findAllMetricSeries(metricsFamilies, labels))
383+
384+
// When: the template is deleted.
385+
require.NoError(t, db.UpdateTemplateDeletedByID(ctx, database.UpdateTemplateDeletedByIDParams{
386+
ID: template.ID,
387+
Deleted: true,
388+
UpdatedAt: dbtime.Now(),
389+
}))
390+
391+
// Then: metrics collect successfully but are empty because the template is deleted.
392+
require.NoError(t, collector.UpdateState(ctx, testutil.WaitLong))
393+
metricsFamilies, err = registry.Gather()
401394
require.NoError(t, err)
402-
require.Equal(t, 1, len(templateVersions))
395+
require.Empty(t, findAllMetricSeries(metricsFamilies, labels))
396+
397+
// When: a new template is created with the same name as the deleted template.
398+
newTemplate := setupTemplateWithDeps()
399+
400+
// Ensure the database has both the new and old (delete) template.
401+
{
402+
deleted, err := db.GetTemplateByOrganizationAndName(ctx, database.GetTemplateByOrganizationAndNameParams{
403+
OrganizationID: template.OrganizationID,
404+
Deleted: true,
405+
Name: template.Name,
406+
})
407+
require.NoError(t, err)
408+
require.Equal(t, template.ID, deleted.ID)
409+
410+
current, err := db.GetTemplateByOrganizationAndName(ctx, database.GetTemplateByOrganizationAndNameParams{
411+
// Use details from deleted template to ensure they're aligned.
412+
OrganizationID: template.OrganizationID,
413+
Deleted: false,
414+
Name: template.Name,
415+
})
416+
require.NoError(t, err)
417+
require.Equal(t, newTemplate.ID, current.ID)
418+
}
403419

404-
presets, err := db.GetPresetsByTemplateVersionID(ctx, templateVersions[0].ID)
420+
// Then: metrics collect successfully.
421+
require.NoError(t, collector.UpdateState(ctx, testutil.WaitLong))
422+
metricsFamilies, err = registry.Gather()
405423
require.NoError(t, err)
406-
require.Equal(t, 1, len(presets))
407-
408-
for _, preset := range presets {
409-
labels := map[string]string{
410-
"template_name": newTemplate.Name,
411-
"preset_name": preset.Name,
412-
"organization_name": defaultOrg.Name,
413-
}
414-
415-
for _, check := range test.metrics {
416-
metric := findMetric(metricsFamilies, check.name, labels)
417-
if check.value == nil {
418-
continue
419-
}
420-
421-
require.NotNil(t, metric, "metric %s should exist", check.name)
422-
423-
if check.isCounter {
424-
require.Equal(t, *check.value, metric.GetCounter().GetValue(), "counter %s value mismatch", check.name)
425-
} else {
426-
require.Equal(t, *check.value, metric.GetGauge().GetValue(), "gauge %s value mismatch", check.name)
427-
}
428-
}
429-
}
424+
require.NotEmpty(t, findAllMetricSeries(metricsFamilies, labels))
430425
}
431426

432427
func findMetric(metricsFamilies []*prometheus_client.MetricFamily, name string, labels map[string]string) *prometheus_client.Metric {

0 commit comments

Comments
 (0)