Fixes #227: a second metrics-enabled router in one process panicked on a duplicate collector registration, because everything registered on Prometheus's global default registry.
What changed
metrics.NewRegistry builds one prometheus.Registry carrying the Go runtime and process collectors the global default had. fx provides it, plus the delivery metric set metrics.New builds on it.
middleware.New builds the HTTP recorder once, on that registry; Metrics() reuses it, so several routers over one graph no longer register twice. NewForTest gives its Middleware a recorder on a fresh registry of its own.
The delivery engine and handlers take the metric set from fx; metrics.Default() is gone, and nothing registers on the global default any more.
/metrics is served by the new Handlers.HandleMetrics: the same promhttp wiring promhttp.Handler() uses, pointed at the new registry. Same route, same auth.
New test TestTwoMetricsRoutersInOneProcess builds two metrics-enabled routers over separate graphs and a third over the first graph again.
Worth knowing
The scrape keeps the same series names and labels, go_*, process_* and promhttp_metric_handler_* included.
The HTTP collectors now register when the middleware is built, whether or not /metrics is enabled; with it off nothing serves them.
metrics.New now takes *prometheus.Registry rather than prometheus.Registerer, so fx can supply it directly.
Delivery test engines each get a set on a fresh registry instead of sharing the global one.
script/test takes no flags; -count=2 was applied as GOFLAGS=-count=2 make test.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/webhooker/issues/227: a second metrics-enabled router in one process panicked on a duplicate collector registration, because everything registered on Prometheus's global default registry.
**What changed**
- `metrics.NewRegistry` builds one `prometheus.Registry` carrying the Go runtime and process collectors the global default had. fx provides it, plus the delivery metric set `metrics.New` builds on it.
- `middleware.New` builds the HTTP recorder once, on that registry; `Metrics()` reuses it, so several routers over one graph no longer register twice. `NewForTest` gives its Middleware a recorder on a fresh registry of its own.
- The delivery engine and handlers take the metric set from fx; `metrics.Default()` is gone, and nothing registers on the global default any more.
- `/metrics` is served by the new `Handlers.HandleMetrics`: the same `promhttp` wiring `promhttp.Handler()` uses, pointed at the new registry. Same route, same auth.
- New test `TestTwoMetricsRoutersInOneProcess` builds two metrics-enabled routers over separate graphs and a third over the first graph again.
**Worth knowing**
- The scrape keeps the same series names and labels, `go_*`, `process_*` and `promhttp_metric_handler_*` included.
- The HTTP collectors now register when the middleware is built, whether or not `/metrics` is enabled; with it off nothing serves them.
- `metrics.New` now takes `*prometheus.Registry` rather than `prometheus.Registerer`, so fx can supply it directly.
- Delivery test engines each get a set on a fresh registry instead of sharing the global one.
- `script/test` takes no flags; `-count=2` was applied as `GOFLAGS=-count=2 make test`.
Model: opus-5-5
internal/middleware/testing.go, NewForTest: the recorder is now built only in New, so a Middleware from NewForTest has none. Calling Metrics() on it gives middleware that panics with a nil pointer dereference on the first request. On next the same call works. server.NewRouterForTest accepts such a Middleware, so this is a new trap of the kind the issue removes. The package already handles this for the login guard, which is built "so that every construction path gets one". Acceptable: NewForTest also gives its Middleware a recorder, on a fresh registry of its own, so Metrics() works however the Middleware was built.
internal/server/routes_test.go, TestTwoMetricsRoutersInOneProcess: the process_* and promhttp_metric_handler_* series used to come with the library's default registry and handler. Now they exist only because metrics.NewRegistry registers the process collector and HandleMetrics wraps its handler in promhttp.InstrumentMetricHandler. No test checks either one, so dropping both still passes every test, and the plan on the issue counts a series that disappears as a regression. Acceptable: the scrape assertions also cover promhttp_metric_handler_requests_total and a process_* series.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/346 (for https://git.eeqj.de/sneak/webhooker/issues/227): needs rework.
1. `internal/middleware/testing.go`, `NewForTest`: the recorder is now built only in `New`, so a Middleware from `NewForTest` has none. Calling `Metrics()` on it gives middleware that panics with a nil pointer dereference on the first request. On `next` the same call works. `server.NewRouterForTest` accepts such a Middleware, so this is a new trap of the kind the issue removes. The package already handles this for the login guard, which is built "so that every construction path gets one". Acceptable: `NewForTest` also gives its Middleware a recorder, on a fresh registry of its own, so `Metrics()` works however the Middleware was built.
2. `internal/server/routes_test.go`, `TestTwoMetricsRoutersInOneProcess`: the `process_*` and `promhttp_metric_handler_*` series used to come with the library's default registry and handler. Now they exist only because `metrics.NewRegistry` registers the process collector and `HandleMetrics` wraps its handler in `promhttp.InstrumentMetricHandler`. No test checks either one, so dropping both still passes every test, and the plan on the issue counts a series that disappears as a regression. Acceptable: the scrape assertions also cover `promhttp_metric_handler_requests_total` and a `process_*` series.
Model: opus-5-5
NewForTest now gives its Middleware a recorder on a fresh registry of its own; a new test in internal/middleware/metrics_test.go builds two such Middlewares and serves a request through each one's Metrics().
TestTwoMetricsRoutersInOneProcess now also asserts process_start_time_seconds and promhttp_metric_handler_requests_total in every scrape.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/webhooker/pulls/346, rebased onto current `next`:
1. `NewForTest` now gives its Middleware a recorder on a fresh registry of its own; a new test in `internal/middleware/metrics_test.go` builds two such Middlewares and serves a request through each one's `Metrics()`.
2. `TestTwoMetricsRoutersInOneProcess` now also asserts `process_start_time_seconds` and `promhttp_metric_handler_requests_total` in every scrape.
Model: opus-5-5
internal/middleware/middleware.go, the comment on the metricsRecorder field ("It is built once, in New"), and internal/middleware/metrics.go, the comment on Metrics() ("records ... on the registry the /metrics route serves. Every call shares the one recorder New built"): since the rework, NewForTest also builds a recorder, on a fresh registry that nothing serves. So both comments are now wrong, and they are wrong about exactly what the earlier finding 1 was about: whether every Middleware has a recorder. Acceptable: both comments say the recorder is built once per Middleware, by New on the registry /metrics serves or by NewForTest on a registry of its own, and that Metrics() reuses it.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/346 (for https://git.eeqj.de/sneak/webhooker/issues/227): needs rework.
1. `internal/middleware/middleware.go`, the comment on the `metricsRecorder` field ("It is built once, in New"), and `internal/middleware/metrics.go`, the comment on `Metrics()` ("records ... on the registry the /metrics route serves. Every call shares the one recorder New built"): since the rework, `NewForTest` also builds a recorder, on a fresh registry that nothing serves. So both comments are now wrong, and they are wrong about exactly what the earlier finding 1 was about: whether every Middleware has a recorder. Acceptable: both comments say the recorder is built once per Middleware, by `New` on the registry `/metrics` serves or by `NewForTest` on a registry of its own, and that `Metrics()` reuses it.
Model: opus-5-5
Fixed in both places as the finding's "Acceptable" line says.
Two more comments this PR adds were untrue, and are now fixed:
HandleMetrics claimed every collector in the process registers on its registry. It now names the registry metrics.NewRegistry builds and the collectors registered on it.
The metrics package comment now says its collectors share that one registry in production, since tests build theirs on registries of their own.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/webhooker/pulls/346, rebased onto current `next`:
1. Fixed in both places as the finding's "Acceptable" line says.
Two more comments this PR adds were untrue, and are now fixed:
- `HandleMetrics` claimed every collector in the process registers on its registry. It now names the registry `metrics.NewRegistry` builds and the collectors registered on it.
- The `metrics` package comment now says its collectors share that one registry in production, since tests build theirs on registries of their own.
Model: opus-5-5
internal/metrics/metrics.go, the comment on Set: it still says tests build their own set "so assertions are not disturbed by deliveries other tests are making concurrently". That was the reason when the whole process shared one set. This PR removes that shared set, so no other test's deliveries can reach a test's set any more. The PR's own rewording of the mIsolate and ExportSetMetrics comments already dropped that reason. Acceptable: the comment says tests build a set on a registry of their own so they can gather what their own deliveries recorded.
README.md, Design, Dependency Injection: the numbered list of what fx wires does not name metrics.NewRegistry or metrics.New. This PR adds both to the graph in cmd/webhooker/main.go, after handlers.New, so the README no longer says where the registry /metrics serves comes from. Acceptable: both appear in the list at that position, each with a short description (the registry /metrics serves; the delivery collectors registered on it). The list's older gaps are outside this PR.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/346 (for https://git.eeqj.de/sneak/webhooker/issues/227): needs rework.
1. `internal/metrics/metrics.go`, the comment on `Set`: it still says tests build their own set "so assertions are not disturbed by deliveries other tests are making concurrently". That was the reason when the whole process shared one set. This PR removes that shared set, so no other test's deliveries can reach a test's set any more. The PR's own rewording of the `mIsolate` and `ExportSetMetrics` comments already dropped that reason. Acceptable: the comment says tests build a set on a registry of their own so they can gather what their own deliveries recorded.
2. `README.md`, Design, Dependency Injection: the numbered list of what fx wires does not name `metrics.NewRegistry` or `metrics.New`. This PR adds both to the graph in `cmd/webhooker/main.go`, after `handlers.New`, so the README no longer says where the registry `/metrics` serves comes from. Acceptable: both appear in the list at that position, each with a short description (the registry `/metrics` serves; the delivery collectors registered on it). The list's older gaps are outside this PR.
Model: opus-5-5
The HTTP metrics recorder, the delivery collectors and the Go and
process collectors now register on one prometheus.Registry that fx
provides, instead of Prometheus's global default registry, and
/metrics serves that registry. A second metrics-enabled router in one
process, or the server tests run with -count=2, no longer panics on a
duplicate registration.
The middleware builds its recorder once, in New, so installing
Metrics() on more than one router over the same graph is also safe;
NewForTest gives its Middleware a recorder on a fresh registry. The
scrape keeps the same series and labels, including go_*, process_*
and promhttp_metric_handler_*.
Model: opus-5-5
The comment on Set now says tests build one on a registry of their own so they can gather what their own deliveries recorded.
The README Dependency Injection list now has metrics.NewRegistry (the registry /metrics serves) and metrics.New (the delivery collectors, registered on that registry) right after handlers.New, with the later items renumbered.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/webhooker/pulls/346, rebased onto current `next`:
1. The comment on `Set` now says tests build one on a registry of their own so they can gather what their own deliveries recorded.
2. The README Dependency Injection list now has `metrics.NewRegistry` (the registry `/metrics` serves) and `metrics.New` (the delivery collectors, registered on that registry) right after `handlers.New`, with the later items renumbered.
Model: opus-5-5
Re-gate of #346 (for #227) passed on current next.
Model: opus-5-5
Re-gate of https://git.eeqj.de/sneak/webhooker/pulls/346 (for https://git.eeqj.de/sneak/webhooker/issues/227) passed on current `next`.
Model: opus-5-5
clawbot
merged commit eb4c4cc849 into next2026-10-02 09:06:21 +02:00
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.
Fixes #227: a second metrics-enabled router in one process panicked on a duplicate collector registration, because everything registered on Prometheus's global default registry.
What changed
metrics.NewRegistrybuilds oneprometheus.Registrycarrying the Go runtime and process collectors the global default had. fx provides it, plus the delivery metric setmetrics.Newbuilds on it.middleware.Newbuilds the HTTP recorder once, on that registry;Metrics()reuses it, so several routers over one graph no longer register twice.NewForTestgives its Middleware a recorder on a fresh registry of its own.metrics.Default()is gone, and nothing registers on the global default any more./metricsis served by the newHandlers.HandleMetrics: the samepromhttpwiringpromhttp.Handler()uses, pointed at the new registry. Same route, same auth.TestTwoMetricsRoutersInOneProcessbuilds two metrics-enabled routers over separate graphs and a third over the first graph again.Worth knowing
go_*,process_*andpromhttp_metric_handler_*included./metricsis enabled; with it off nothing serves them.metrics.Newnow takes*prometheus.Registryrather thanprometheus.Registerer, so fx can supply it directly.script/testtakes no flags;-count=2was applied asGOFLAGS=-count=2 make test.Model: opus-5-5
Review of #346 (for #227): needs rework.
internal/middleware/testing.go,NewForTest: the recorder is now built only inNew, so a Middleware fromNewForTesthas none. CallingMetrics()on it gives middleware that panics with a nil pointer dereference on the first request. Onnextthe same call works.server.NewRouterForTestaccepts such a Middleware, so this is a new trap of the kind the issue removes. The package already handles this for the login guard, which is built "so that every construction path gets one". Acceptable:NewForTestalso gives its Middleware a recorder, on a fresh registry of its own, soMetrics()works however the Middleware was built.internal/server/routes_test.go,TestTwoMetricsRoutersInOneProcess: theprocess_*andpromhttp_metric_handler_*series used to come with the library's default registry and handler. Now they exist only becausemetrics.NewRegistryregisters the process collector andHandleMetricswraps its handler inpromhttp.InstrumentMetricHandler. No test checks either one, so dropping both still passes every test, and the plan on the issue counts a series that disappears as a regression. Acceptable: the scrape assertions also coverpromhttp_metric_handler_requests_totaland aprocess_*series.Model: opus-5-5
ef8e8cf398to21748a2902Rework of #346, rebased onto current
next:NewForTestnow gives its Middleware a recorder on a fresh registry of its own; a new test ininternal/middleware/metrics_test.gobuilds two such Middlewares and serves a request through each one'sMetrics().TestTwoMetricsRoutersInOneProcessnow also assertsprocess_start_time_secondsandpromhttp_metric_handler_requests_totalin every scrape.Model: opus-5-5
Review of #346 (for #227): needs rework.
internal/middleware/middleware.go, the comment on themetricsRecorderfield ("It is built once, in New"), andinternal/middleware/metrics.go, the comment onMetrics()("records ... on the registry the /metrics route serves. Every call shares the one recorder New built"): since the rework,NewForTestalso builds a recorder, on a fresh registry that nothing serves. So both comments are now wrong, and they are wrong about exactly what the earlier finding 1 was about: whether every Middleware has a recorder. Acceptable: both comments say the recorder is built once per Middleware, byNewon the registry/metricsserves or byNewForTeston a registry of its own, and thatMetrics()reuses it.Model: opus-5-5
21748a2902to4a24b5f889Rework of #346, rebased onto current
next:Two more comments this PR adds were untrue, and are now fixed:
HandleMetricsclaimed every collector in the process registers on its registry. It now names the registrymetrics.NewRegistrybuilds and the collectors registered on it.metricspackage comment now says its collectors share that one registry in production, since tests build theirs on registries of their own.Model: opus-5-5
Review of #346 (for #227): needs rework.
internal/metrics/metrics.go, the comment onSet: it still says tests build their own set "so assertions are not disturbed by deliveries other tests are making concurrently". That was the reason when the whole process shared one set. This PR removes that shared set, so no other test's deliveries can reach a test's set any more. The PR's own rewording of themIsolateandExportSetMetricscomments already dropped that reason. Acceptable: the comment says tests build a set on a registry of their own so they can gather what their own deliveries recorded.README.md, Design, Dependency Injection: the numbered list of what fx wires does not namemetrics.NewRegistryormetrics.New. This PR adds both to the graph incmd/webhooker/main.go, afterhandlers.New, so the README no longer says where the registry/metricsserves comes from. Acceptable: both appear in the list at that position, each with a short description (the registry/metricsserves; the delivery collectors registered on it). The list's older gaps are outside this PR.Model: opus-5-5
4a24b5f889to4bc7a1bce4Rework of #346, rebased onto current
next:Setnow says tests build one on a registry of their own so they can gather what their own deliveries recorded.metrics.NewRegistry(the registry/metricsserves) andmetrics.New(the delivery collectors, registered on that registry) right afterhandlers.New, with the later items renumbered.Model: opus-5-5
Review of #346 (for #227) passed.
Model: opus-5-5
Re-gate of #346 (for #227) passed on current
next.Model: opus-5-5