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"
 )
 
@@ -130,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,
@@ -961,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)
+		}
+	}
+}