test: fix TestDescCacheTimestampUpdate flake (#20975)

ethanndickson · web-flow · commit 645da3376705 · 2025-12-02T10:53:36.000+11:00
## Problem `TestDescCacheTimestampUpdate` was flaky on Windows CI because `time.Now()` has ~15.6ms resolution, causing consecutive calls to return identical timestamps. ## Solution Inject `quartz.Clock` into `MetricsAggregator` using an options pattern, making the test deterministic by using a mock clock with explicit time advancement. ### Changes - Add `clock quartz.Clock` field to `MetricsAggregator` struct - Add `WithClock()` option for dependency injection - Replace all `time.Now()` calls with `ma.clock.Now()` - Update test to use mock clock with `mClock.Advance(time.Second)` --- This PR was fully generated by [`mux`](https://github.com/coder/mux) using Claude Opus 4.5, and reviewed by me. Closes coder/internal#1146
diff --git a/coderd/prometheusmetrics/aggregator.go b/coderd/prometheusmetrics/aggregator.go
@@ -16,6 +16,8 @@ import (
 	agentproto "github.com/coder/coder/v2/agent/proto"
 	"github.com/coder/coder/v2/coderd/agentmetrics"
 	"github.com/coder/coder/v2/coderd/pproflabel"
+
+	"github.com/coder/quartz"
 )
 
 const (
@@ -47,6 +49,7 @@ type MetricsAggregator struct {
 
 	log                    slog.Logger
 	metricsCleanupInterval time.Duration
+	clock                  quartz.Clock
 
 	collectCh chan (chan []prometheus.Metric)
 	updateCh  chan updateRequest
@@ -151,7 +154,7 @@ func (am *annotatedMetric) shallowCopy() annotatedMetric {
 	}
 }
 
-func NewMetricsAggregator(logger slog.Logger, registerer prometheus.Registerer, duration time.Duration, aggregateByLabels []string) (*MetricsAggregator, error) {
+func NewMetricsAggregator(logger slog.Logger, registerer prometheus.Registerer, duration time.Duration, aggregateByLabels []string, options ...func(*MetricsAggregator)) (*MetricsAggregator, error) {
 	metricsCleanupInterval := defaultMetricsCleanupInterval
 	if duration > 0 {
 		metricsCleanupInterval = duration
@@ -192,9 +195,10 @@ func NewMetricsAggregator(logger slog.Logger, registerer prometheus.Registerer,
 		return nil, err
 	}
 
-	return &MetricsAggregator{
+	ma := &MetricsAggregator{
 		log:                    logger.Named(loggerName),
 		metricsCleanupInterval: metricsCleanupInterval,
+		clock:                  quartz.NewReal(),
 
 		store: map[metricKey]annotatedMetric{},
 
@@ -206,7 +210,19 @@ func NewMetricsAggregator(logger slog.Logger, registerer prometheus.Registerer,
 		cleanupHistogram: cleanupHistogram,
 
 		aggregateByLabels: aggregateByLabels,
-	}, nil
+	}
+
+	for _, option := range options {
+		option(ma)
+	}
+
+	return ma, nil
+}
+
+func WithClock(clock quartz.Clock) func(*MetricsAggregator) {
+	return func(ma *MetricsAggregator) {
+		ma.clock = clock
+	}
 }
 
 // labelAggregator is used to control cardinality of collected Prometheus metrics by pre-aggregating series based on given labels.
@@ -349,7 +365,7 @@ func (ma *MetricsAggregator) Run(ctx context.Context) func() {
 				ma.log.Debug(ctx, "clean expired metrics")
 
 				timer := prometheus.NewTimer(ma.cleanupHistogram)
-				now := time.Now()
+				now := ma.clock.Now()
 
 				for key, val := range ma.store {
 					if now.After(val.expiryDate) {
@@ -407,7 +423,7 @@ func (ma *MetricsAggregator) getOrCreateDesc(name string, help string, baseLabel
 	}
 	key := cacheKeyForDesc(name, baseLabelNames, extraLabels)
 	if d, ok := ma.descCache[key]; ok {
-		d.lastUsed = time.Now()
+		d.lastUsed = ma.clock.Now()
 		ma.descCache[key] = d
 		return d.desc
 	}
@@ -419,7 +435,7 @@ func (ma *MetricsAggregator) getOrCreateDesc(name string, help string, baseLabel
 		labels[nBase+i] = l.Name
 	}
 	d := prometheus.NewDesc(name, help, labels, nil)
-	ma.descCache[key] = descCacheEntry{d, time.Now()}
+	ma.descCache[key] = descCacheEntry{d, ma.clock.Now()}
 	return d
 }
 
@@ -497,7 +513,7 @@ func (ma *MetricsAggregator) Update(ctx context.Context, labels AgentMetricLabel
 		templateName:  labels.TemplateName,
 		metrics:       metrics,
 
-		timestamp: time.Now(),
+		timestamp: ma.clock.Now(),
 	}:
 	case <-ctx.Done():
 		ma.log.Debug(ctx, "update request is canceled")
@@ -508,7 +524,7 @@ func (ma *MetricsAggregator) Update(ctx context.Context, labels AgentMetricLabel
 
 // Move to a function for testability
 func (ma *MetricsAggregator) cleanupDescCache() {
-	now := time.Now()
+	now := ma.clock.Now()
 	for key, entry := range ma.descCache {
 		if now.Sub(entry.lastUsed) > ma.metricsCleanupInterval {
 			delete(ma.descCache, key)
diff --git a/coderd/prometheusmetrics/aggregator_internal_test.go b/coderd/prometheusmetrics/aggregator_internal_test.go
@@ -11,6 +11,7 @@ import (
 	agentproto "github.com/coder/coder/v2/agent/proto"
 	"github.com/coder/coder/v2/coderd/agentmetrics"
 	"github.com/coder/coder/v2/testutil"
+	"github.com/coder/quartz"
 )
 
 func TestDescCache_DescExpire(t *testing.T) {
@@ -62,8 +63,9 @@ func TestDescCache_DescExpire(t *testing.T) {
 func TestDescCacheTimestampUpdate(t *testing.T) {
 	t.Parallel()
 
+	mClock := quartz.NewMock(t)
 	registry := prometheus.NewRegistry()
-	ma, err := NewMetricsAggregator(slogtest.Make(t, nil), registry, time.Hour, nil)
+	ma, err := NewMetricsAggregator(slogtest.Make(t, nil), registry, time.Hour, nil, WithClock(mClock))
 	require.NoError(t, err)
 
 	baseLabelNames := []string{"label1", "label2"}
@@ -78,6 +80,9 @@ func TestDescCacheTimestampUpdate(t *testing.T) {
 	initialEntry := ma.descCache[key]
 	initialTime := initialEntry.lastUsed
 
+	// Advance the mock clock to ensure a different timestamp
+	mClock.Advance(time.Second)
+
 	desc2 := ma.getOrCreateDesc("test_metric", "help text", baseLabelNames, extraLabels)
 	require.NotNil(t, desc2)
 
