Path: routes_test.go:734 TestMetricsRouteRequiresCredentials -> middleware.Metrics -> go-http-metrics NewRecorder -> MustRegister (internal/middleware/middleware.go:444). metrics.NewRecorder(metrics.Config{}) leaves Config.Registry defaulted to prometheus.DefaultRegisterer, and MustRegister panics on a duplicate.
TestMetricsRouteRequiresCredentials arrived with #216 — the first internal/server test to construct a metrics-enabled router. One registration happens per test binary today, so the normal gate passes; a second metrics-enabled router test, or -count=2, panics.
Why it is worth fixing rather than tolerating: -count=2 is the ordinary way to smoke out state leaking between runs, and it is currently unavailable for internal/server. It also means any future test that builds a second metrics-enabled router fails for a reason unrelated to what it is testing, which is a trap for whoever writes it.
Remedy (proposed by two independent reviewers): give middleware.New a dedicated prometheus.Registry and serve /metrics through a matching gatherer, instead of the global default registerer. That also decouples the delivery collectors added by #209 from the global default.
Note the delivery collectors in #224 are NOT implicated — they register via sync.OnceValue and pass -count=2.
Definition of done:
go test -count=2 ./internal/server/... passes
two metrics-enabled routers can be constructed in one process without panicking, covered by a test that does exactly that
/metrics still exposes the three existing http_* series and the delivery series from #209
no second /metrics route and no second scrape path
Found during the review of https://git.eeqj.de/sneak/webhooker/pulls/224, then reproduced on plain `origin/next` (`bb30b3a`) with that branch absent. It is a defect in `next`, not in any PR.
```
duplicate metrics collector registration attempted
```
Path: `routes_test.go:734 TestMetricsRouteRequiresCredentials` -> `middleware.Metrics` -> go-http-metrics `NewRecorder` -> `MustRegister` (`internal/middleware/middleware.go:444`). `metrics.NewRecorder(metrics.Config{})` leaves `Config.Registry` defaulted to `prometheus.DefaultRegisterer`, and `MustRegister` panics on a duplicate.
`TestMetricsRouteRequiresCredentials` arrived with https://git.eeqj.de/sneak/webhooker/pulls/216 — the first `internal/server` test to construct a metrics-enabled router. One registration happens per test binary today, so the normal gate passes; a second metrics-enabled router test, or `-count=2`, panics.
Why it is worth fixing rather than tolerating: `-count=2` is the ordinary way to smoke out state leaking between runs, and it is currently unavailable for `internal/server`. It also means any future test that builds a second metrics-enabled router fails for a reason unrelated to what it is testing, which is a trap for whoever writes it.
Remedy (proposed by two independent reviewers): give `middleware.New` a dedicated `prometheus.Registry` and serve `/metrics` through a matching gatherer, instead of the global default registerer. That also decouples the delivery collectors added by https://git.eeqj.de/sneak/webhooker/issues/209 from the global default.
Note the delivery collectors in https://git.eeqj.de/sneak/webhooker/pulls/224 are NOT implicated — they register via `sync.OnceValue` and pass `-count=2`.
Definition of done:
- `go test -count=2 ./internal/server/...` passes
- two metrics-enabled routers can be constructed in one process without panicking, covered by a test that does exactly that
- `/metrics` still exposes the three existing `http_*` series and the delivery series from https://git.eeqj.de/sneak/webhooker/issues/209
- no second `/metrics` route and no second scrape path
Plan. Still present on next (f0adeaf): internal/middleware/metrics.go builds the recorder with prommetrics.NewRecorder(prommetrics.Config{}), which registers on Prometheus's global default registerer. /metrics in internal/server/routes.go serves promhttp.Handler(), which gathers from the global default.
Registry: do what the issue proposes. One prometheus.Registry is created once per process and provided through fx. The HTTP recorder, the delivery collectors and the Go/process collectors that the default registry carried all register on it, and /metrics serves it through promhttp.HandlerFor.
Metrics must not change: check the scrape before and after. It must have the same series names and labels, including the go_* and process_* series the default registry exposed. A series that disappears is a regression.
No leftover globals: nothing in the tree may still register on the global default.
Test: build two metrics-enabled routers in one process, with no panic.
Verify as the issue says, with -count=2 on internal/server run through the repo's own test entrypoint. If script/test cannot take the flag, say how it was run, in one line on the PR.
Model: opus-5-5
Plan. Still present on `next` (`f0adeaf`): `internal/middleware/metrics.go` builds the recorder with `prommetrics.NewRecorder(prommetrics.Config{})`, which registers on Prometheus's global default registerer. `/metrics` in `internal/server/routes.go` serves `promhttp.Handler()`, which gathers from the global default.
- **Registry:** do what the issue proposes. One `prometheus.Registry` is created once per process and provided through fx. The HTTP recorder, the delivery collectors and the Go/process collectors that the default registry carried all register on it, and `/metrics` serves it through `promhttp.HandlerFor`.
- **Metrics must not change:** check the scrape before and after. It must have the same series names and labels, including the `go_*` and `process_*` series the default registry exposed. A series that disappears is a regression.
- **No leftover globals:** nothing in the tree may still register on the global default.
- **Test:** build two metrics-enabled routers in one process, with no panic.
- **Verify** as the issue says, with `-count=2` on `internal/server` run through the repo's own test entrypoint. If `script/test` cannot take the flag, say how it was run, in one line on the PR.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 10:59:21 +02:00
Implemented in #346: one registry provided through fx now carries the HTTP, delivery, Go and process collectors, and /metrics serves it; nothing registers on the global default any more.
Model: opus-5-5
Implemented in https://git.eeqj.de/sneak/webhooker/pulls/346: one registry provided through fx now carries the HTTP, delivery, Go and process collectors, and `/metrics` serves it; nothing registers on the global default any more.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Found during the review of #224, then reproduced on plain
origin/next(bb30b3a) with that branch absent. It is a defect innext, not in any PR.Path:
routes_test.go:734 TestMetricsRouteRequiresCredentials->middleware.Metrics-> go-http-metricsNewRecorder->MustRegister(internal/middleware/middleware.go:444).metrics.NewRecorder(metrics.Config{})leavesConfig.Registrydefaulted toprometheus.DefaultRegisterer, andMustRegisterpanics on a duplicate.TestMetricsRouteRequiresCredentialsarrived with #216 — the firstinternal/servertest to construct a metrics-enabled router. One registration happens per test binary today, so the normal gate passes; a second metrics-enabled router test, or-count=2, panics.Why it is worth fixing rather than tolerating:
-count=2is the ordinary way to smoke out state leaking between runs, and it is currently unavailable forinternal/server. It also means any future test that builds a second metrics-enabled router fails for a reason unrelated to what it is testing, which is a trap for whoever writes it.Remedy (proposed by two independent reviewers): give
middleware.Newa dedicatedprometheus.Registryand serve/metricsthrough a matching gatherer, instead of the global default registerer. That also decouples the delivery collectors added by #209 from the global default.Note the delivery collectors in #224 are NOT implicated — they register via
sync.OnceValueand pass-count=2.Definition of done:
go test -count=2 ./internal/server/...passes/metricsstill exposes the three existinghttp_*series and the delivery series from #209/metricsroute and no second scrape pathPlan. Still present on
next(f0adeaf):internal/middleware/metrics.gobuilds the recorder withprommetrics.NewRecorder(prommetrics.Config{}), which registers on Prometheus's global default registerer./metricsininternal/server/routes.goservespromhttp.Handler(), which gathers from the global default.prometheus.Registryis created once per process and provided through fx. The HTTP recorder, the delivery collectors and the Go/process collectors that the default registry carried all register on it, and/metricsserves it throughpromhttp.HandlerFor.go_*andprocess_*series the default registry exposed. A series that disappears is a regression.-count=2oninternal/serverrun through the repo's own test entrypoint. Ifscript/testcannot take the flag, say how it was run, in one line on the PR.Model: opus-5-5
Implemented in #346: one registry provided through fx now carries the HTTP, delivery, Go and process collectors, and
/metricsserves it; nothing registers on the global default any more.Model: opus-5-5