diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 3c27ff9..4587a1a 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -3,13 +3,17 @@ package metrics import ( "net/http" "strconv" + "sync" + "sync/atomic" "time" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" ) -type tracker interface { +// EventTracker is the sink for metric emissions. The process-wide instance is +// reached through Tracker and swapped with SetTracker. +type EventTracker interface { TrackRequest(service, method string, status int, duration time.Duration) AddInflightRequest(service string) SubtractInflightRequest(service string) @@ -27,13 +31,112 @@ type tracker interface { TrackDenial(service, rule string) } -var Tracker tracker = &nullTracker{} +// Tracker delegates every emission to the tracker SetTracker installed last (a +// no-op until Enable runs). The delegate is behind an atomic pointer because +// installation must not race with request goroutines emitting through it -- +// and in tests, background work outlives the test that started it. +var Tracker = &delegatingTracker{} +var enableOnce sync.Once + +// Enable installs the Prometheus tracker and returns the handler serving its +// collectors. Registration happens once per process: the collectors live in +// the default registry, which panics on re-registration, and a second +// metrics-enabled server (or `go test -count=2`) must reuse them. Later calls +// also leave the active tracker alone, so a tracker installed in between (a +// test fake) keeps receiving events. func Enable() http.Handler { - Tracker = NewPrometheusTracker() + enableOnce.Do(func() { + SetTracker(NewPrometheusTracker()) + }) return promhttp.Handler() } +// SetTracker atomically installs t as the destination for all metric events +// and returns the tracker it replaced, so tests can restore it. +func SetTracker(t EventTracker) EventTracker { + previous := Tracker.delegate.Swap(&trackerBox{t}) + if previous == nil { + return &nullTracker{} + } + return previous.t +} + +// trackerBox keeps the atomic pointer to a single concrete type while the +// tracked value stays an interface. +type trackerBox struct{ t EventTracker } + +type delegatingTracker struct { + delegate atomic.Pointer[trackerBox] +} + +func (d *delegatingTracker) active() EventTracker { + if box := d.delegate.Load(); box != nil { + return box.t + } + return nullTracker{} +} + +func (d *delegatingTracker) TrackRequest(service, method string, status int, duration time.Duration) { + d.active().TrackRequest(service, method, status, duration) +} + +func (d *delegatingTracker) AddInflightRequest(service string) { + d.active().AddInflightRequest(service) +} + +func (d *delegatingTracker) SubtractInflightRequest(service string) { + d.active().SubtractInflightRequest(service) +} + +func (d *delegatingTracker) SetCertificateExpiry(domain string, isWildcard bool, expiryTime time.Time) { + d.active().SetCertificateExpiry(domain, isWildcard, expiryTime) +} + +func (d *delegatingTracker) IncCertificateRenewals(domain string, success bool) { + d.active().IncCertificateRenewals(domain, success) +} + +func (d *delegatingTracker) SetCertificateCount(total, wildcard, http01 int) { + d.active().SetCertificateCount(total, wildcard, http01) +} + +func (d *delegatingTracker) TrackCacheEvent(service, result string) { + d.active().TrackCacheEvent(service, result) +} + +func (d *delegatingTracker) TrackCacheRefusal(service, reason string) { + d.active().TrackCacheRefusal(service, reason) +} + +func (d *delegatingTracker) TrackCacheLease(service, outcome string) { + d.active().TrackCacheLease(service, outcome) +} + +func (d *delegatingTracker) TrackCacheLeaseWait(service, outcome string) { + d.active().TrackCacheLeaseWait(service, outcome) +} + +func (d *delegatingTracker) TrackCacheEviction(service, state string) { + d.active().TrackCacheEviction(service, state) +} + +func (d *delegatingTracker) SetDynamicRedirects(service string, hosts, rules int) { + d.active().SetDynamicRedirects(service, hosts, rules) +} + +func (d *delegatingTracker) TrackDynamicRedirectPoll(service, outcome string) { + d.active().TrackDynamicRedirectPoll(service, outcome) +} + +func (d *delegatingTracker) TrackDynamicRedirect(service string, status int) { + d.active().TrackDynamicRedirect(service, status) +} + +func (d *delegatingTracker) TrackDenial(service, rule string) { + d.active().TrackDenial(service, rule) +} + type nullTracker struct{} func (nullTracker) TrackRequest(service, method string, status int, dur time.Duration) {} diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go index e6aa78a..bee2884 100644 --- a/internal/metrics/metrics_test.go +++ b/internal/metrics/metrics_test.go @@ -1,11 +1,96 @@ package metrics import ( + "sync" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +// recordingTracker captures cache events; everything else is a no-op. +type recordingTracker struct { + nullTracker + mu sync.Mutex + events map[string]int +} + +func (r *recordingTracker) TrackCacheEvent(service, result string) { + r.mu.Lock() + defer r.mu.Unlock() + if r.events == nil { + r.events = map[string]int{} + } + r.events[service+":"+result]++ +} + +func (r *recordingTracker) eventCount(service, result string) int { + r.mu.Lock() + defer r.mu.Unlock() + return r.events[service+":"+result] +} + +func TestSetTracker_DelegatesEventsAndRestores(t *testing.T) { + recording := &recordingTracker{} + previous := SetTracker(recording) + t.Cleanup(func() { SetTracker(previous) }) + + Tracker.TrackCacheEvent("service1", "hit") + assert.Equal(t, 1, recording.eventCount("service1", "hit")) + + restored := SetTracker(previous) + assert.Same(t, recording, restored, "SetTracker returns the tracker it replaced") + + Tracker.TrackCacheEvent("service1", "hit") + assert.Equal(t, 1, recording.eventCount("service1", "hit"), "events after restore must not reach the removed tracker") +} + +// Two Enable calls happen whenever a second metrics-enabled Server starts in +// the same process -- most importantly `go test -count=2`. The second call must +// reuse the registered collectors instead of panicking on re-registration. +func TestEnable_IsIdempotent(t *testing.T) { + require.NotPanics(t, func() { + require.NotNil(t, Enable()) + require.NotNil(t, Enable()) + }) +} + +// A tracker installed after Enable (a test fake) must survive further Enable +// calls -- Enable clobbering it is what made cache-metric assertions depend on +// which tests ran before them. +func TestEnable_DoesNotReplaceATrackerInstalledAfterIt(t *testing.T) { + Enable() + + recording := &recordingTracker{} + previous := SetTracker(recording) + t.Cleanup(func() { SetTracker(previous) }) + + Enable() + + Tracker.TrackCacheEvent("service1", "miss") + assert.Equal(t, 1, recording.eventCount("service1", "miss")) +} + +// Swapping the tracker while request goroutines emit through it must be free +// of data races (run with -race). +func TestSetTracker_IsSafeUnderConcurrentEmission(t *testing.T) { + done := make(chan struct{}) + go func() { + defer close(done) + for range 1000 { + Tracker.TrackRequest("service1", "GET", 200, time.Millisecond) + } + }() + + original := SetTracker(&recordingTracker{}) + for range 100 { + SetTracker(&recordingTracker{}) + } + SetTracker(original) + <-done +} + func TestNormalizeMethod(t *testing.T) { assert.Equal(t, "GET", normalizeMethod("GET")) assert.Equal(t, "POST", normalizeMethod("POST")) diff --git a/internal/server/cert_metrics_test.go b/internal/server/cert_metrics_test.go index db350c3..db2875b 100644 --- a/internal/server/cert_metrics_test.go +++ b/internal/server/cert_metrics_test.go @@ -4,7 +4,6 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "testing" "time" @@ -230,112 +229,15 @@ func (f *fakeTracker) renewalCount(domain string, success bool) int { return f.renewals[key] } -// switchableTracker is installed into metrics.Tracker exactly once, before any -// test runs, and thereafter only its delegate changes. -// -// Swapping metrics.Tracker itself is a data race: it is a package-level variable -// that every request path reads, and background work outlives the test that -// started it -- a stale revalidation goroutine reading the tracker while the -// next test installs its own is a genuine concurrent write. Swapping an atomic -// delegate instead removes the write entirely. -type switchableTracker struct { - delegate atomic.Pointer[fakeTracker] -} - -var activeTracker = &switchableTracker{} - -func init() { - // In init rather than in installFakeTracker: at this point no goroutine - // exists that could be reading the variable. - metrics.Tracker = activeTracker -} - -func (s *switchableTracker) current() *fakeTracker { return s.delegate.Load() } - -func (s *switchableTracker) TrackRequest(service, method string, status int, dur time.Duration) {} -func (s *switchableTracker) AddInflightRequest(service string) {} -func (s *switchableTracker) SubtractInflightRequest(service string) {} - -func (s *switchableTracker) SetCertificateExpiry(domain string, isWildcard bool, expiry time.Time) { - if fake := s.current(); fake != nil { - fake.SetCertificateExpiry(domain, isWildcard, expiry) - } -} - -func (s *switchableTracker) IncCertificateRenewals(domain string, success bool) { - if fake := s.current(); fake != nil { - fake.IncCertificateRenewals(domain, success) - } -} - -func (s *switchableTracker) SetCertificateCount(total, wildcard, http01 int) { - if fake := s.current(); fake != nil { - fake.SetCertificateCount(total, wildcard, http01) - } -} - -func (s *switchableTracker) TrackCacheEvent(service, result string) { - if fake := s.current(); fake != nil { - fake.TrackCacheEvent(service, result) - } -} - -func (s *switchableTracker) TrackCacheRefusal(service, reason string) { - if fake := s.current(); fake != nil { - fake.TrackCacheRefusal(service, reason) - } -} - -func (s *switchableTracker) TrackCacheLease(service, outcome string) { - if fake := s.current(); fake != nil { - fake.TrackCacheLease(service, outcome) - } -} - -func (s *switchableTracker) TrackCacheLeaseWait(service, outcome string) { - if fake := s.current(); fake != nil { - fake.TrackCacheLeaseWait(service, outcome) - } -} - -func (s *switchableTracker) TrackCacheEviction(service, state string) { - if fake := s.current(); fake != nil { - fake.TrackCacheEviction(service, state) - } -} - -func (s *switchableTracker) SetDynamicRedirects(service string, hosts, rules int) { - if fake := s.current(); fake != nil { - fake.SetDynamicRedirects(service, hosts, rules) - } -} - -func (s *switchableTracker) TrackDynamicRedirectPoll(service, outcome string) { - if fake := s.current(); fake != nil { - fake.TrackDynamicRedirectPoll(service, outcome) - } -} - -func (s *switchableTracker) TrackDynamicRedirect(service string, status int) { - if fake := s.current(); fake != nil { - fake.TrackDynamicRedirect(service, status) - } -} - -func (s *switchableTracker) TrackDenial(service, rule string) { - if fake := s.current(); fake != nil { - fake.TrackDenial(service, rule) - } -} - // installFakeTracker points the tracker at a fresh capturing tracker for the -// duration of one test. +// duration of one test. metrics.SetTracker swaps atomically, so background +// work left over from other tests can keep emitting while it happens. func installFakeTracker(t *testing.T) *fakeTracker { t.Helper() fake := newFakeTracker() - previous := activeTracker.delegate.Swap(fake) - t.Cleanup(func() { activeTracker.delegate.Store(previous) }) + previous := metrics.SetTracker(fake) + t.Cleanup(func() { metrics.SetTracker(previous) }) return fake }