From 645da33767057ff0585703510d76bd8cb8030f8d Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Tue, 2 Dec 2025 10:53:36 +1100 Subject: [PATCH] test: fix TestDescCacheTimestampUpdate flake (#20975) ## 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 https://github.com/coder/internal/issues/1146 --- coderd/prometheusmetrics/aggregator.go | 32 ++++++++++++++----- .../aggregator_internal_test.go | 7 +++- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/coderd/prometheusmetrics/aggregator.go b/coderd/prometheusmetrics/aggregator.go index f3693137d3..f11468a3d9 100644 --- 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 index cd5f4432dc..0efb1cf530 100644 --- 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)