fix(metrics): make tracker swap atomic and Enable idempotent for test isolation - #106
Open
mhenrixon wants to merge 1 commit into
Open
fix(metrics): make tracker swap atomic and Enable idempotent for test isolation#106mhenrixon wants to merge 1 commit into
mhenrixon wants to merge 1 commit into
Conversation
… isolation ## Summary metrics.Enable() overwrote the package-global Tracker and re-ran prometheus.MustRegister against the default registry on every call, so a second metrics-enabled server in the same process (go test -count=2) panicked on duplicate registration, and any Enable call orphaned the test suite's switchable fake tracker -- making cache/cert metric assertions depend on which tests ran before them. Tracker is now a delegating tracker whose delegate swaps atomically via SetTracker, and Enable registers the Prometheus collectors exactly once per process without clobbering a tracker installed after it. The server test suite's local switchableTracker duplicated exactly this mechanism and is deleted in favor of metrics.SetTracker. ## Test Coverage - TestEnable_IsIdempotent: two Enable calls, no duplicate-registration panic - TestEnable_DoesNotReplaceATrackerInstalledAfterIt: fakes survive later Enable calls - TestSetTracker_DelegatesEventsAndRestores: swap semantics + restore - TestSetTracker_IsSafeUnderConcurrentEmission: swap under -race while emitting - go test -race -count=2 ./internal/server/: 3660 passed (previously panicked) ## Verification - [x] gofmt -l internal/ cmd/ clean - [x] make test passes - [x] go vet ./... and make lint (golangci-lint) clean - [x] go test -race ./... clean Closes #104
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
internal/metrics/metrics.go:Trackeris now a delegating tracker whose delegate swaps atomically via a newSetTracker(EventTracker) EventTracker;Enable()registers the Prometheus collectors under async.Onceand never clobbers a tracker installed after it.internal/metrics/metrics_test.go: pins idempotentEnable, swap/restore semantics, fake survival acrossEnablecalls, and race-safety of swapping while emitting.internal/server/cert_metrics_test.go: deletes the test-localswitchableTracker(it duplicated exactly this mechanism);installFakeTrackernow usesmetrics.SetTrackerwith at.Cleanuprestore.Root cause:
Enable()didTracker = NewPrometheusTracker()on every call — the second call in one process re-ranprometheus.MustRegisteragainst the default registry (the-count=2panic), and the first call orphaned the test suite's switchable fake by replacing the wholeTrackervar (the cache-metrics flake, plus an unsynchronized write racing background emitters under-race).Closes #104
Test plan
go test -race -count=2 ./internal/server/— 3660 passed (previously deterministic panic)go test -race -count=3 ./internal/server/ -run 'TestServer_AppliesConfiguredTimeoutsToEveryListener|TestCacheMetrics'— cache-metric deltas isolated from a metrics-enabled server in the same processmake test,go test -race ./...(2151 passed, 8 packages)gofmt -l internal/ cmd/clean,go vet ./...clean,make lint0 issuesDeviations & judgment calls
internal/server/cert_metrics_test.goalready carried aswitchableTracker(atomic delegate installed asmetrics.Trackerininit()) precisely to avoid the tracker-swap data race — the bug wasEnable()orphaning it. The fix promotes that same pattern into themetricspackage and deletes the test-local copy rather than adding a second layer of indirection.sync.Once(the issue's first option) instead of threading an injectableprometheus.RegistererthroughConfig/Server(the second). Once is sufficient for both acceptance criteria; per-server registries would also drop the default registry'sgo_*/process_*collectors from/metricsunless re-added — a behavior change the issue didn't ask for.metrics.EventTrackersoSetTrackerdoesn't take/return an unexported type (revive/staticcheck flag that).Summary by cubic
Make the process-wide metrics tracker swap atomic and make
metrics.Enable()idempotent to prevent duplicate Prometheus registration and fix test isolation races. PreviouslyEnable()replacedmetrics.Trackerand re-registered collectors on each call (panic under-count=2) and orphaned test fakes; now a delegating tracker forwards to an atomically swapped delegate andEnable()registers once and does not overwrite a tracker installed after it.metrics.EventTrackerand a delegatingmetrics.Tracker; addsmetrics.SetTracker(EventTracker) EventTrackerto atomically swap and restore the previous tracker.metrics.Enable()is guarded bysync.Once; returns the Prometheus handler without re-registration and without clobbering a post-Enable()tracker.switchableTrackerininternal/server/cert_metrics_test.gowithmetrics.SetTrackerandt.Cleanup.Enable(), and race-safety while swapping.metrics.Trackerdirectly should callmetrics.SetTracker(fake)and restore with the returned previous tracker.Written for commit 09a5abd. Summary will update on new commits.