Serve /metrics from a registry of its own (closes #227)

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.
The scrape keeps the same series and labels, including go_*,
process_* and promhttp_metric_handler_*.

Model: opus-5-5

README.md

@@ -2721,7 +2721,7 @@ abuse limit later; they are tracked as future work.
 | ------ | --------------------------- | ----------- |
 | `GET`  | `/`                         | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
 | `GET`  | `/.well-known/healthcheck`  | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
-| any    | `/s/*`                      | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` |
+| `GET`, `HEAD` | `/s/*`              | Static file serving (embedded CSS, JS). `GET` and `HEAD` only — `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` and `CONNECT` are answered `405 Method Not Allowed` with `Allow: GET, HEAD`. Any other method (such as `PROPFIND`) is refused by chi before it reaches this route, and gets `405` without an `Allow` header. Pinned by `TestStaticServesOnlyGetAndHead` |
 | `POST` | `/webhook/{uuid}`           | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) |
 
 #### Authentication Endpoints

cmd/webhooker/main.go

@@ -16,6 +16,7 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
+	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/resetpw"
 	"sneak.berlin/go/webhooker/internal/server"
@@ -177,6 +178,10 @@ func newApp() *fx.App {
 			healthcheck.New,
 			session.New,
 			handlers.New,
+			// The registry /metrics serves, and the delivery
+			// collectors registered on it.
+			metrics.NewRegistry,
+			metrics.New,
 			middleware.New,
 			// The one SSRF guard both target-creation validation
 			// and the delivery dialer consult, so they cannot

internal/delivery/engine.go

@@ -148,6 +148,7 @@ type EngineParams struct {
 	DBManager *database.WebhookDBManager
 	Logger    *logger.Logger
 	SSRFGuard *Guard
+	Metrics   *metrics.Set
 }
 
 // Engine processes queued deliveries in the background
@@ -167,10 +168,10 @@ type Engine struct {
 	retryCh    chan Task
 	workers    int
 
-	// mtr is the delivery metric set. Production wires the
+	// mtr is the delivery metric set. Production wires the one
-	// process-wide one; a test can substitute a set registered on
+	// registered on the registry /metrics serves; a test can
-	// a private registry so its assertions are not disturbed by
+	// substitute a set registered on a registry it holds, so it can
-	// deliveries other tests are making at the same time.
+	// gather what its own deliveries recorded.
 	mtr *metrics.Set
 
 	// targets maps each target type to its implementation.
@@ -204,7 +205,7 @@ func New(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    defaultWorkers,
-		mtr:        metrics.Default(),
+		mtr:        params.Metrics,
 	}
 
 	e.initTargets(&http.Client{

internal/delivery/export_test.go

@@ -9,6 +9,7 @@ import (
 	"net/url"
 	"time"
 
+	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
@@ -389,7 +390,7 @@ func NewTestEngine(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.Default(),
+		mtr:        metrics.New(prometheus.NewRegistry()),
 	}
 	e.initTargets(client)
 
@@ -404,7 +405,7 @@ func NewTestEngineSmallRetry(
 	e := &Engine{
 		log:     log,
 		retryCh: make(chan Task, 1),
-		mtr:     metrics.Default(),
+		mtr:     metrics.New(prometheus.NewRegistry()),
 	}
 	e.initTargets(nil)
 
@@ -427,7 +428,7 @@ func NewTestEngineWithDB(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.Default(),
+		mtr:        metrics.New(prometheus.NewRegistry()),
 	}
 	e.initTargets(client)
 
@@ -435,8 +436,7 @@ func NewTestEngineWithDB(
 }
 
 // ExportSetMetrics substitutes the engine's metric set, so a test can
-// assert on collectors registered on a private registry instead of
+// assert on collectors registered on a registry it holds.
-// the process-wide ones every other test is also moving.
 func (e *Engine) ExportSetMetrics(mtr *metrics.Set) {
 	e.mtr = mtr
 }

internal/delivery/metrics_test.go

@@ -35,9 +35,8 @@ const (
 )
 
 // mIsolate gives the setup's engine a metric set registered on a
-// private registry. The process-wide collectors are moved by every
+// registry this test holds, so its exact assertions can gather from
-// other delivery test running in parallel, so exact assertions are
+// it.
-// only possible against a registry this test owns.
 func mIsolate(
 	t *testing.T, s iSetup,
 ) *prometheus.Registry {

internal/handlers/handlers.go

@@ -12,6 +12,7 @@ import (
 	"net/http"
 	"sync/atomic"
 
+	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
@@ -61,6 +62,8 @@ type HandlersParams struct {
 	Notifier     delivery.Notifier
 	Evictor      delivery.WebhookEvictor
 	SSRFGuard    *delivery.Guard
+	Metrics      *metrics.Set
+	Registry     *prometheus.Registry
 }
 
 // Handlers provides HTTP handler methods for all application
@@ -122,7 +125,7 @@ func New(
 	s.mw = params.Middleware
 	s.notifier = params.Notifier
 	s.evictor = params.Evictor
-	s.mtr = metrics.Default()
+	s.mtr = params.Metrics
 	s.ssrf = params.SSRFGuard
 
 	// Parse all page templates once at startup

internal/handlers/handlers_test.go

@@ -20,6 +20,7 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
+	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/session"
 )
@@ -109,6 +110,8 @@ func newTestApp(
 			func(r *recordingEvictor) delivery.WebhookEvictor {
 				return r
 			},
+			metrics.NewRegistry,
+			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/handlers/metrics.go

@@ -0,0 +1,20 @@
+package handlers
+
+import (
+	"net/http"
+
+	"github.com/prometheus/client_golang/prometheus/promhttp"
+)
+
+// HandleMetrics returns the Prometheus scrape handler for the
+// registry every collector in this process registers on. It is what
+// promhttp.Handler builds for the global default registry, including
+// the promhttp_metric_handler_* series that count scrapes, pointed at
+// that registry instead.
+func (s *Handlers) HandleMetrics() http.HandlerFunc {
+	reg := s.params.Registry
+
+	return promhttp.InstrumentMetricHandler(
+		reg, promhttp.HandlerFor(reg, promhttp.HandlerOpts{}),
+	).ServeHTTP
+}

internal/metrics/metrics.go

@@ -3,17 +3,17 @@
 // deliveries are attempted, how they end, how long they take, how
 // deep the queues are, and how many circuit breakers are open.
 //
-// The inbound HTTP metrics come from the go-http-metrics recorder in
+// It also builds the registry the authenticated /metrics route
-// internal/middleware and land on prometheus.DefaultRegisterer. These
+// serves. These collectors, the inbound HTTP metrics recorded in
-// collectors register there too, so both surfaces are gathered by the
+// internal/middleware, and the Go runtime and process collectors all
-// one promhttp handler mounted on the authenticated /metrics route.
+// register on that one registry, never on Prometheus's global default.
 package metrics
 
 import (
-	"sync"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/collectors"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"sneak.berlin/go/webhooker/internal/database"
 )
@@ -57,25 +57,31 @@ var knownTargetTypes = []database.TargetType{
 	database.TargetTypeSlack,
 }
 
-// defaultSet is the process-wide metric set, registered on the same
+// NewRegistry returns the registry /metrics serves, carrying the Go
-// registry the HTTP middleware and the /metrics handler already use.
+// runtime and process collectors that Prometheus's global default
-// It is built on first use rather than in an init so that a test
+// registry carries, so the go_* and process_* series stay in the
-// binary that never touches metrics never registers them.
+// scrape.
 //
-//nolint:gochecknoglobals // one process-wide registration, by design
+// A registry of its own, rather than the global default, is what lets
-var defaultSet = sync.OnceValue(func() *Set {
+// two dependency graphs in one process — two tests, say — each
-	return New(prometheus.DefaultRegisterer)
+// register their collectors without the second registration
-})
+// panicking.
+func NewRegistry() *prometheus.Registry {
+	reg := prometheus.NewRegistry()
+	reg.MustRegister(
+		collectors.NewGoCollector(),
+		collectors.NewProcessCollector(
+			collectors.ProcessCollectorOpts{},
+		),
+	)
 
-// Default returns the process-wide metric set.
+	return reg
-func Default() *Set {
-	return defaultSet()
 }
 
 // Set is one registered group of webhooker's delivery collectors.
-// Production uses the single Default set; tests build their own
+// Production builds one on the registry /metrics serves; tests build
-// against a private registry so assertions are not disturbed by
+// their own against a private registry so assertions are not
-// deliveries other tests are making concurrently.
+// disturbed by deliveries other tests are making concurrently.
 type Set struct {
 	eventsReceived      prometheus.Counter
 	deliveryAttempts    *prometheus.CounterVec
@@ -93,7 +99,7 @@ type Set struct {
 // New registers a full set of delivery collectors on reg and returns
 // it. It panics if reg already holds them, which is the intended
 // behaviour for a duplicate registration.
-func New(reg prometheus.Registerer) *Set {
+func New(reg *prometheus.Registry) *Set {
 	factory := promauto.With(reg)
 
 	s := &Set{

internal/middleware/export_test.go

@@ -10,8 +10,7 @@ import (
 
 // MetricsMiddlewareForTest builds the metrics recording middleware
 // against a caller-supplied recorder, so a test can gather from its
-// own Prometheus registry rather than the process-wide default one
+// own Prometheus registry without building a whole Middleware.
-// that Middleware.Metrics uses.
 func MetricsMiddlewareForTest(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {

internal/middleware/metrics.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/go-chi/chi"
 	httpmetrics "github.com/slok/go-http-metrics/metrics"
-	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	ghmm "github.com/slok/go-http-metrics/middleware"
 	"github.com/slok/go-http-metrics/middleware/std"
 )
@@ -152,16 +151,14 @@ func (r boundedLabelRecorder) AddInflightRequests(
 var _ httpmetrics.Recorder = boundedLabelRecorder{}
 
 // Metrics returns middleware that records Prometheus HTTP metrics on
-// the default registry, which is the one the /metrics route gathers.
+// the registry the /metrics route serves. Every call shares the one
+// recorder New built, so any number of routers can install it.
 func (s *Middleware) Metrics() func(http.Handler) http.Handler {
-	return metricsMiddleware(
+	return metricsMiddleware(s.metricsRecorder)
-		prommetrics.NewRecorder(prommetrics.Config{}),
-	)
 }
 
 // metricsMiddleware builds the recording middleware against a given
-// recorder, so tests can gather from a registry of their own instead
+// recorder, so tests can gather from a registry of their own.
-// of the process-wide default.
 func metricsMiddleware(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {

internal/middleware/metrics_test.go

@@ -57,9 +57,8 @@ const (
 // Server.setupWebhookRoutes inside it. That ordering is the whole
 // defect, so a test that flattens it would prove nothing.
 //
-// The recorder writes to a registry of the test's own rather than the
+// The recorder writes to a registry of the test's own, so each test
-// process-wide default one, so each test observes only its own
+// observes only its own traffic.
-// traffic.
 func metricsTestRouter(
 	t *testing.T,
 	receiverLimit int,

internal/middleware/middleware.go

@@ -13,6 +13,9 @@ import (
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
 	"github.com/go-chi/cors"
+	"github.com/prometheus/client_golang/prometheus"
+	httpmetrics "github.com/slok/go-http-metrics/metrics"
+	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/globals"
@@ -148,10 +151,11 @@ const (
 type MiddlewareParams struct {
 	fx.In
 
-	Logger  *logger.Logger
+	Logger   *logger.Logger
-	Globals *globals.Globals
+	Globals  *globals.Globals
-	Config  *config.Config
+	Config   *config.Config
-	Session *session.Session
+	Session  *session.Session
+	Registry *prometheus.Registry
 }
 
 // Middleware provides HTTP middleware for logging, CORS, auth, and
@@ -161,6 +165,12 @@ type Middleware struct {
 	params  *MiddlewareParams
 	session *session.Session
 
+	// metricsRecorder records the inbound HTTP metrics on the
+	// registry /metrics serves. It is built once, in New, because
+	// building it registers its collectors, and a second
+	// registration on the same registry panics; see Metrics.
+	metricsRecorder httpmetrics.Recorder
+
 	// loginGuard counts failed credential verifications and bounds
 	// concurrent password hashing. It is built on first use so that
 	// every construction path gets one; see guard().
@@ -179,6 +189,9 @@ func New(
 	s.params = &params
 	s.log = params.Logger.Get()
 	s.session = params.Session
+	s.metricsRecorder = prommetrics.NewRecorder(
+		prommetrics.Config{Registry: params.Registry},
+	)
 
 	return s, nil
 }

internal/resetpw/resetpw_test.go

@@ -24,6 +24,7 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
+	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/resetpw"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -163,6 +164,8 @@ func newServerApp(
 			session.New,
 			func() delivery.Notifier { return &noopNotifier{} },
 			func() delivery.WebhookEvictor { return &noopEvictor{} },
+			metrics.NewRegistry,
+			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/server/routes.go

@@ -7,7 +7,6 @@ import (
 	sentryhttp "github.com/getsentry/sentry-go/http"
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
-	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"sneak.berlin/go/webhooker/static"
 )
 
@@ -92,11 +91,25 @@ func (s *Server) setupGlobalMiddleware() {
 func (s *Server) setupRoutes() {
 	s.router.Get("/", s.h.HandleIndex())
 
-	s.router.Mount(
+	// Static assets answer GET and HEAD only. chi's default 405
-		"/s",
+	// carries no Allow header, so this group supplies its own.
-		http.StripPrefix("/s", http.FileServer(http.FS(static.Static))),
+	staticFiles := http.StripPrefix(
+		"/s", http.FileServer(http.FS(static.Static)),
 	)
 
+	s.router.Route("/s", func(r chi.Router) {
+		r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) {
+			w.Header().Set("Allow", "GET, HEAD")
+			http.Error(
+				w,
+				"Method Not Allowed",
+				http.StatusMethodNotAllowed,
+			)
+		})
+		r.Method(http.MethodGet, "/*", staticFiles)
+		r.Method(http.MethodHead, "/*", staticFiles)
+	})
+
 	s.router.Route("/api/v1", func(_ chi.Router) {
 		// API routes will be added here.
 	})
@@ -116,12 +129,7 @@ func (s *Server) setupRoutes() {
 	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Group(func(r chi.Router) {
 			r.Use(s.mw.MetricsAuth())
-			r.Get(
+			r.Get("/metrics", s.h.HandleMetrics())
-				"/metrics",
-				http.HandlerFunc(
-					promhttp.Handler().ServeHTTP,
-				),
-			)
 		})
 	}
 

internal/server/routes_test.go

@@ -23,6 +23,7 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
+	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/server"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -112,6 +113,8 @@ func newTestEnvWithConfig(
 			session.New,
 			func() delivery.Notifier { return &noopNotifier{} },
 			func() delivery.WebhookEvictor { return &noopEvictor{} },
+			metrics.NewRegistry,
+			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,
@@ -396,13 +399,15 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
 
 // --- /s static group ---
 
-// TestStaticServesEveryMethod pins what the static mount actually
+// TestStaticServesOnlyGetAndHead pins the methods the static group
-// answers. chi's Mount registers the handler for all methods and
+// answers: GET and HEAD are served the asset, and the other methods
-// http.FileServer only special-cases HEAD (by suppressing the body),
+// chi routes (POST, PUT, DELETE and the rest) are refused with 405
-// so a POST or a DELETE to an asset is served the file rather than
+// and an Allow header naming those two. A method chi does not route,
-// refused. The README documents this; the test is what keeps the two
+// such as PROPFIND, is refused with 405 by the top-level router
-// from drifting.
+// before it reaches the static group, so it gets no Allow header.
-func TestStaticServesEveryMethod(t *testing.T) {
+// The README documents this; the test is what keeps the two from
+// drifting.
+func TestStaticServesOnlyGetAndHead(t *testing.T) {
 	t.Parallel()
 
 	env := newTestEnv(t)
@@ -417,6 +422,7 @@ func TestStaticServesEveryMethod(t *testing.T) {
 		http.MethodPost,
 		http.MethodPut,
 		http.MethodDelete,
+		"PROPFIND",
 	} {
 		t.Run(method, func(t *testing.T) {
 			t.Parallel()
@@ -428,18 +434,38 @@ func TestStaticServesEveryMethod(t *testing.T) {
 			w := httptest.NewRecorder()
 			env.router.ServeHTTP(w, req)
 
-			assert.Equal(t, http.StatusOK, w.Code,
+			switch method {
-				"static mount answers every method")
+			case http.MethodGet:
-
+				assert.Equal(t, http.StatusOK, w.Code)
-			if method == http.MethodHead {
+				assert.Equal(t, body, w.Body.Bytes(),
+					"the asset itself is returned")
+			case http.MethodHead:
+				assert.Equal(t, http.StatusOK, w.Code)
 				assert.Empty(t, w.Body.Bytes(),
 					"HEAD must not carry a body")
-
+			case "PROPFIND":
-				return
+				assert.Equal(
+					t, http.StatusMethodNotAllowed, w.Code,
+				)
+				assert.Empty(t, w.Header().Get("Allow"),
+					"chi refuses a method it does not route "+
+						"before the static group runs")
+				assert.NotContains(
+					t, w.Body.String(), string(body),
+					"a refused method must not get the asset",
+				)
+			default:
+				assert.Equal(
+					t, http.StatusMethodNotAllowed, w.Code,
+				)
+				assert.Equal(
+					t, "GET, HEAD", w.Header().Get("Allow"),
+				)
+				assert.NotContains(
+					t, w.Body.String(), string(body),
+					"a refused method must not get the asset",
+				)
 			}
-
-			assert.Equal(t, body, w.Body.Bytes(),
-				"the asset itself is returned")
 		})
 	}
 }
@@ -938,3 +964,43 @@ func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
 		})
 	}
 }
+
+// TestTwoMetricsRoutersInOneProcess pins
+// https://git.eeqj.de/sneak/webhooker/issues/227: a second
+// metrics-enabled router in one process used to panic, because the
+// HTTP metrics registered on Prometheus's global default registry.
+// Two routers are built over separate dependency graphs and a third
+// over the first graph again, and each must still serve the HTTP,
+// delivery and Go runtime series.
+func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
+	t.Parallel()
+
+	first := newTestEnvWithConfig(
+		t, metricsConfig(t, metricsUser, metricsAuthValue),
+	)
+	second := newTestEnvWithConfig(
+		t, metricsConfig(t, metricsUser, metricsAuthValue),
+	)
+	third := &testEnv{
+		router: server.NewRouterForTest(
+			first.log.Get(), first.cfg, first.mw, first.hnd,
+		),
+	}
+
+	for _, env := range []*testEnv{first, second, third} {
+		env.get("/", nil)
+
+		scrape := env.metricsRequest(metricsUser, metricsAuthValue)
+		require.Equal(t, http.StatusOK, scrape.Code)
+
+		for _, series := range []string{
+			"http_request_duration_seconds",
+			"http_response_size_bytes",
+			"http_requests_inflight",
+			"webhooker_events_received_total",
+			"go_goroutines",
+		} {
+			assert.Contains(t, scrape.Body.String(), series)
+		}
+	}
+}