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;
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

README.md

@@ -145,11 +145,6 @@ TTY detection, and security headers are always applied.
 | `TRUSTED_PROXIES`       | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket; a correct login password is never throttled either way) | `""` (none) |
 | `ALLOWED_EGRESS_CIDRS`  | CIDRs that delivery targets may reach despite the SSRF blocklist. Read [Allowing egress to your own network](#allowing-egress-to-your-own-network) before setting it | `""` (none) |
 
-The Settings page of the web UI (`/settings`, behind the login) lists
-every one of these with the value the running server loaded. It is
-read-only, and it shows `METRICS_PASSWORD` and `SENTRY_DSN` only as
-set or not set, never their values.
-
 #### Allowing egress to your own network
 
 By default every delivery target must resolve to a public address. The
@@ -2707,7 +2702,6 @@ abuse limit later; they are tracked as future work.
 | ------ | ------------------------ | ----------- |
 | `GET`  | `/user/{username}`       | User profile page |
 | `POST` | `/user/{username}/password` | Change the user's password (5 per minute per bucket, then `429`; `503` if no verification slot frees up within 5s, or immediately if 16 requests are already queued for one) |
-| `GET`  | `/settings`              | Read-only list of the configuration the server is running with; `METRICS_PASSWORD` and `SENTRY_DSN` show only as set or not set |
 | `GET`  | `/sources`               | List user's webhooks |
 | `GET`  | `/sources/new`           | Create webhook form |
 | `POST` | `/sources/new`           | Create webhook submission |
@@ -2820,7 +2814,6 @@ webhooker/
 │   │   ├── healthcheck.go     # Health check handler
 │   │   ├── index.go           # Index page handler
 │   │   ├── profile.go         # User profile handler
-│   │   ├── settings.go        # Read-only Settings page handler
 │   │   ├── source_management.go  # Webhook CRUD handlers
 │   │   └── webhook.go         # Webhook receiver handler
 │   ├── healthcheck/

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
-	// process-wide one; a test can substitute a set registered on
-	// a private registry so its assertions are not disturbed by
-	// deliveries other tests are making at the same time.
+	// mtr is the delivery metric set. Production wires the one
+	// registered on the registry /metrics serves; a test can
+	// substitute a set registered on a registry it holds, so it can
+	// 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
-// the process-wide ones every other test is also moving.
+// assert on collectors registered on a registry it holds.
 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
-// other delivery test running in parallel, so exact assertions are
-// only possible against a registry this test owns.
+// registry this test holds, so its exact assertions can gather from
+// it.
 func mIsolate(
 	t *testing.T, s iSetup,
 ) *prometheus.Registry {

internal/handlers/handlers.go

@@ -12,8 +12,8 @@ import (
 	"net/http"
 	"sync/atomic"
 
+	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
-	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
 	"sneak.berlin/go/webhooker/internal/globals"
@@ -54,7 +54,6 @@ type HandlersParams struct {
 
 	Logger       *logger.Logger
 	Globals      *globals.Globals
-	Config       *config.Config
 	Database     *database.Database
 	WebhookDBMgr *database.WebhookDBManager
 	Healthcheck  *healthcheck.Healthcheck
@@ -63,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
@@ -124,14 +125,13 @@ 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
 	s.templates = map[string]*template.Template{
 		"login.html":         parsePageTemplate("login.html"),
 		"profile.html":       parsePageTemplate("profile.html"),
-		"settings.html":      parsePageTemplate("settings.html"),
 		"sources_list.html":  parsePageTemplate("sources_list.html"),
 		"sources_new.html":   parsePageTemplate("sources_new.html"),
 		"source_detail.html": parsePageTemplate("source_detail.html"),

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"
 )
@@ -83,25 +84,16 @@ func newTestApp(
 ) *fxtest.App {
 	t.Helper()
 
-	return newTestAppWithConfig(
-		t, &config.Config{DataDir: t.TempDir()}, targets...,
-	)
-}
-
-// newTestAppWithConfig is newTestApp over a caller-supplied Config.
-func newTestAppWithConfig(
-	t *testing.T,
-	cfg *config.Config,
-	targets ...any,
-) *fxtest.App {
-	t.Helper()
-
 	return fxtest.New(
 		t,
 		fx.Provide(
 			globals.New,
 			logger.New,
-			func() *config.Config { return cfg },
+			func() *config.Config {
+				return &config.Config{
+					DataDir: t.TempDir(),
+				}
+			},
 			database.New,
 			database.NewWebhookDBManager,
 			healthcheck.New,
@@ -118,6 +110,8 @@ func newTestAppWithConfig(
 			func(r *recordingEvictor) delivery.WebhookEvictor {
 				return r
 			},
+			metrics.NewRegistry,
+			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/handlers/metrics.go

@@ -0,0 +1,21 @@
+package handlers
+
+import (
+	"net/http"
+
+	"github.com/prometheus/client_golang/prometheus/promhttp"
+)
+
+// HandleMetrics returns the Prometheus scrape handler for the
+// registry built by metrics.NewRegistry, which the HTTP, delivery, Go
+// runtime and process collectors register 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/handlers/settings.go

@@ -1,130 +0,0 @@
-package handlers
-
-import (
-	"net/http"
-	"net/netip"
-	"strconv"
-	"strings"
-
-	"sneak.berlin/go/webhooker/internal/config"
-)
-
-// notSet is what the Settings page shows for a value that is empty.
-const notSet = "not set"
-
-// settingRow is one line of the Settings page: an environment
-// variable, what it controls, and the value the server loaded for it.
-type settingRow struct {
-	Name        string
-	Description string
-	Value       string
-}
-
-// HandleSettings returns a handler for the read-only Settings page,
-// which lists the configuration the server started with.
-func (h *Handlers) HandleSettings() http.HandlerFunc {
-	return func(w http.ResponseWriter, r *http.Request) {
-		h.renderTemplate(w, r, "settings.html", map[string]any{
-			"Settings": settingRows(h.params.Config),
-		})
-	}
-}
-
-// settingRows lists every field of cfg under the environment variable
-// it is read from, in the order of the README's configuration table.
-// METRICS_PASSWORD and SENTRY_DSN are credentials, so their values
-// never reach the page: only whether they are set.
-func settingRows(cfg *config.Config) []settingRow {
-	metricsUsername := cfg.MetricsUsername
-	if metricsUsername == "" {
-		metricsUsername = notSet
-	}
-
-	return []settingRow{
-		{"WEBHOOKER_ENVIRONMENT", "dev or prod", cfg.Environment},
-		{"PORT", "HTTP listen port", strconv.Itoa(cfg.Port)},
-		{
-			"BIND_ADDRESS",
-			"IP address the HTTP listener binds",
-			cfg.BindAddress,
-		},
-		{
-			"DATA_DIR",
-			"Directory for all SQLite databases",
-			cfg.DataDir,
-		},
-		{
-			"DEBUG",
-			"Enable debug logging",
-			strconv.FormatBool(cfg.Debug),
-		},
-		{
-			"MAINTENANCE_MODE",
-			"Report maintenanceMode: true in the healthcheck JSON. " +
-				"It does not change how any request is served",
-			strconv.FormatBool(cfg.MaintenanceMode),
-		},
-		{
-			"METRICS_USERNAME",
-			"Basic auth username for /metrics",
-			metricsUsername,
-		},
-		{
-			"METRICS_PASSWORD",
-			"Basic auth password for /metrics",
-			setOrNotSet(cfg.MetricsPassword),
-		},
-		{
-			"SENTRY_DSN",
-			"Error reporting DSN. Unset leaves error reporting off",
-			setOrNotSet(cfg.SentryDSN),
-		},
-		{
-			"RETENTION_SWEEP_INTERVAL",
-			"How often the retention reaper and archive sweeper run",
-			cfg.RetentionSweepInterval.String(),
-		},
-		{
-			"SESSION_IDLE_TIMEOUT",
-			"Idle session timeout. Zero or negative disables idle " +
-				"expiry",
-			cfg.SessionIdleTimeout.String(),
-		},
-		{
-			"RECEIVER_RATE_LIMIT",
-			"Receiver requests per minute per IP per entrypoint " +
-				"(10x that per IP across the route)",
-			strconv.Itoa(cfg.ReceiverRateLimit),
-		},
-		{
-			"TRUSTED_PROXIES",
-			"CIDRs whose forwarded headers are trusted",
-			cidrList(cfg.TrustedProxies),
-		},
-		{
-			"ALLOWED_EGRESS_CIDRS",
-			"CIDRs that delivery targets may reach despite the " +
-				"SSRF blocklist",
-			cidrList(cfg.AllowedEgressCIDRs),
-		},
-	}
-}
-
-// setOrNotSet is how the Settings page shows a credential: whether it
-// has a value, never the value itself.
-func setOrNotSet(value string) string {
-	if value == "" {
-		return notSet
-	}
-
-	return "set"
-}
-
-// cidrList renders a CIDR list setting for the Settings page.
-func cidrList(prefixes []netip.Prefix) string {
-	if len(prefixes) == 0 {
-		return "none"
-	}
-
-	return strings.Join(config.PrefixStrings(prefixes), ", ")
-}

internal/handlers/settings_test.go

@@ -1,130 +0,0 @@
-package handlers_test
-
-import (
-	"context"
-	"html"
-	"net/http"
-	"net/http/httptest"
-	"net/netip"
-	"regexp"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
-	"sneak.berlin/go/webhooker/internal/handlers"
-	"sneak.berlin/go/webhooker/internal/session"
-)
-
-// settingsShown renders the Settings page over cfg as a logged-in user
-// and returns the value it shows for each variable name, plus the
-// whole page.
-func settingsShown(
-	t *testing.T, cfg *config.Config,
-) (map[string]string, string) {
-	t.Helper()
-
-	var h *handlers.Handlers
-
-	var sess *session.Session
-
-	app := newTestAppWithConfig(t, cfg, &h, &sess)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodGet, "/settings", nil,
-	)
-	for _, c := range authenticatedCookies(t, sess, "id", "admin") {
-		req.AddCookie(c)
-	}
-
-	w := httptest.NewRecorder()
-	h.HandleSettings().ServeHTTP(w, req)
-	require.Equal(t, http.StatusOK, w.Code)
-
-	body := w.Body.String()
-
-	row := regexp.MustCompile(
-		`<code[^>]*>([A-Z_]+)</code>\s*<code[^>]*>([^<]*)</code>`,
-	)
-
-	shown := map[string]string{}
-	for _, match := range row.FindAllStringSubmatch(body, -1) {
-		shown[match[1]] = html.UnescapeString(match[2])
-	}
-
-	return shown, body
-}
-
-func TestSettingsPageShowsLoadedConfiguration(t *testing.T) {
-	t.Parallel()
-
-	const (
-		metricsPassword = "metrics-password-1f9a"
-		sentryKey       = "dsnkey7c2e"
-		sentryDSN       = "https://" + sentryKey + "@errors.example.com/42"
-	)
-
-	cfg := &config.Config{
-		DataDir:                t.TempDir(),
-		Debug:                  true,
-		MaintenanceMode:        true,
-		Environment:            config.EnvironmentDev,
-		MetricsUsername:        "scraper",
-		MetricsPassword:        metricsPassword,
-		Port:                   9123,
-		SentryDSN:              sentryDSN,
-		BindAddress:            "192.0.2.10",
-		RetentionSweepInterval: 17 * time.Minute,
-		SessionIdleTimeout:     3 * time.Hour,
-		ReceiverRateLimit:      77,
-		TrustedProxies: []netip.Prefix{
-			netip.MustParsePrefix("10.1.0.0/16"),
-		},
-		AllowedEgressCIDRs: []netip.Prefix{
-			netip.MustParsePrefix("192.168.5.0/24"),
-			netip.MustParsePrefix("fd00::/8"),
-		},
-	}
-
-	shown, body := settingsShown(t, cfg)
-
-	assert.Equal(t, map[string]string{
-		"WEBHOOKER_ENVIRONMENT":    "dev",
-		"PORT":                     "9123",
-		"BIND_ADDRESS":             "192.0.2.10",
-		"DATA_DIR":                 cfg.DataDir,
-		"DEBUG":                    "true",
-		"MAINTENANCE_MODE":         "true",
-		"METRICS_USERNAME":         "scraper",
-		"METRICS_PASSWORD":         "set",
-		"SENTRY_DSN":               "set",
-		"RETENTION_SWEEP_INTERVAL": "17m0s",
-		"SESSION_IDLE_TIMEOUT":     "3h0m0s",
-		"RECEIVER_RATE_LIMIT":      "77",
-		"TRUSTED_PROXIES":          "10.1.0.0/16",
-		"ALLOWED_EGRESS_CIDRS":     "192.168.5.0/24, fd00::/8",
-	}, shown)
-
-	assert.NotContains(t, body, metricsPassword)
-	assert.NotContains(t, body, sentryKey)
-	assert.Contains(
-		t, body, `href="/settings"`,
-		"the navigation bar links to the page",
-	)
-}
-
-func TestSettingsPageShowsUnsetValues(t *testing.T) {
-	t.Parallel()
-
-	shown, _ := settingsShown(t, &config.Config{DataDir: t.TempDir()})
-
-	assert.Equal(t, "not set", shown["METRICS_USERNAME"])
-	assert.Equal(t, "not set", shown["METRICS_PASSWORD"])
-	assert.Equal(t, "not set", shown["SENTRY_DSN"])
-	assert.Equal(t, "none", shown["TRUSTED_PROXIES"])
-	assert.Equal(t, "none", shown["ALLOWED_EGRESS_CIDRS"])
-}

internal/metrics/metrics.go

@@ -3,17 +3,18 @@
 // 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
-// internal/middleware and land on prometheus.DefaultRegisterer. These
-// collectors register there too, so both surfaces are gathered by the
-// one promhttp handler mounted on the authenticated /metrics route.
+// It also builds the registry the authenticated /metrics route
+// serves. In production, these collectors, the inbound HTTP metrics
+// recorded in internal/middleware, and the Go runtime and process
+// collectors all 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 +58,31 @@ var knownTargetTypes = []database.TargetType{
 	database.TargetTypeSlack,
 }
 
-// defaultSet is the process-wide metric set, registered on the same
-// registry the HTTP middleware and the /metrics handler already use.
-// It is built on first use rather than in an init so that a test
-// binary that never touches metrics never registers them.
+// NewRegistry returns the registry /metrics serves, carrying the Go
+// runtime and process collectors that Prometheus's global default
+// registry carries, so the go_* and process_* series stay in the
+// scrape.
 //
-//nolint:gochecknoglobals // one process-wide registration, by design
-var defaultSet = sync.OnceValue(func() *Set {
-	return New(prometheus.DefaultRegisterer)
-})
+// A registry of its own, rather than the global default, is what lets
+// two dependency graphs in one process — two tests, say — each
+// 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.
-func Default() *Set {
-	return defaultSet()
+	return reg
 }
 
 // Set is one registered group of webhooker's delivery collectors.
-// Production uses the single Default set; tests build their own
-// against a private registry so assertions are not disturbed by
-// deliveries other tests are making concurrently.
+// Production builds one on the registry /metrics serves; tests build
+// their own against a private registry so assertions are not
+// disturbed by deliveries other tests are making concurrently.
 type Set struct {
 	eventsReceived      prometheus.Counter
 	deliveryAttempts    *prometheus.CounterVec
@@ -93,7 +100,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
-// that Middleware.Metrics uses.
+// own Prometheus registry without building a whole Middleware.
 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"
 )
@@ -151,17 +150,17 @@ 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.
+// Metrics returns middleware that records Prometheus HTTP metrics
+// with the Middleware's one recorder, which New builds on the registry
+// the /metrics route serves and NewForTest on a registry of its own.
+// Every call reuses that recorder, so any number of routers can
+// install it.
 func (s *Middleware) Metrics() func(http.Handler) http.Handler {
-	return metricsMiddleware(
-		prommetrics.NewRecorder(prommetrics.Config{}),
-	)
+	return metricsMiddleware(s.metricsRecorder)
 }
 
 // metricsMiddleware builds the recording middleware against a given
-// recorder, so tests can gather from a registry of their own instead
-// of the process-wide default.
+// recorder, so tests can gather from a registry of their own.
 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
-// process-wide default one, so each test observes only its own
-// traffic.
+// The recorder writes to a registry of the test's own, so each test
+// observes only its own traffic.
 func metricsTestRouter(
 	t *testing.T,
 	receiverLimit int,
@@ -455,3 +454,29 @@ func TestMetrics_StatusAndSizeStillRecorded(t *testing.T) {
 		"the interceptor must still count written bytes",
 	)
 }
+
+// TestMetrics_WorksOnNewForTestMiddleware pins that a Middleware built
+// by NewForTest has a recorder of its own: its Metrics() serves a
+// request instead of panicking, and a second one does not collide
+// with the first.
+func TestMetrics_WorksOnNewForTestMiddleware(t *testing.T) {
+	t.Parallel()
+
+	log := slog.New(slog.DiscardHandler)
+	cfg := &config.Config{Environment: "prod"}
+	ok := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(okBody))
+	})
+
+	for range 2 {
+		h := middleware.NewForTest(log, cfg, nil).Metrics()(ok)
+
+		req := httptest.NewRequestWithContext(
+			t.Context(), http.MethodGet, okRoute, nil,
+		)
+		w := httptest.NewRecorder()
+		h.ServeHTTP(w, req)
+
+		assert.Equal(t, http.StatusOK, w.Code)
+	}
+}

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
-	Globals *globals.Globals
-	Config  *config.Config
-	Session *session.Session
+	Logger   *logger.Logger
+	Globals  *globals.Globals
+	Config   *config.Config
+	Session  *session.Session
+	Registry *prometheus.Registry
 }
 
 // Middleware provides HTTP middleware for logging, CORS, auth, and
@@ -161,6 +165,14 @@ type Middleware struct {
 	params  *MiddlewareParams
 	session *session.Session
 
+	// metricsRecorder records the inbound HTTP metrics. New builds
+	// it on the registry /metrics serves, NewForTest on a registry
+	// of its own. Either way it is built once per Middleware and
+	// Metrics reuses it, because building it registers its
+	// collectors, and a second registration on the same registry
+	// panics.
+	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 +191,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/middleware/testing.go

@@ -3,12 +3,17 @@ package middleware
 import (
 	"log/slog"
 
+	"github.com/prometheus/client_golang/prometheus"
+	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/session"
 )
 
 // NewForTest creates a Middleware with the minimum dependencies
 // needed for testing. This bypasses the fx lifecycle.
+//
+// Its metrics recorder writes to a fresh registry of its own, so
+// Metrics() works on it and two of them never collide.
 func NewForTest(
 	log *slog.Logger,
 	cfg *config.Config,
@@ -20,5 +25,8 @@ func NewForTest(
 			Config: cfg,
 		},
 		session: sess,
+		metricsRecorder: prommetrics.NewRecorder(
+			prommetrics.Config{Registry: prometheus.NewRegistry()},
+		),
 	}
 }

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,18 +129,12 @@ func (s *Server) setupRoutes() {
 	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Group(func(r chi.Router) {
 			r.Use(s.mw.MetricsAuth())
-			r.Get(
-				"/metrics",
-				http.HandlerFunc(
-					promhttp.Handler().ServeHTTP,
-				),
-			)
+			r.Get("/metrics", s.h.HandleMetrics())
 		})
 	}
 
 	s.setupPageRoutes()
 	s.setupUserRoutes()
-	s.setupSettingsRoutes()
 	s.setupSourceRoutes()
 	s.setupWebhookRoutes()
 }
@@ -182,21 +175,6 @@ func (s *Server) setupUserRoutes() {
 	})
 }
 
-// setupSettingsRoutes serves the Settings page. It is GET only:
-// configuration comes from the environment and nothing here changes
-// it.
-func (s *Server) setupSettingsRoutes() {
-	s.router.Route("/settings", func(r chi.Router) {
-		// MaxBodySize precedes CSRF and RequireAuth deliberately;
-		// see maxFormBodySize for why, and for what it costs.
-		r.Use(s.mw.MaxBodySize(maxFormBodySize))
-		r.Use(s.mw.CSRF())
-		r.Use(s.mw.NoCache())
-		r.Use(s.mw.RequireAuth())
-		r.Get("/", s.h.HandleSettings())
-	})
-}
-
 func (s *Server) setupSourceRoutes() {
 	s.router.Route("/sources", func(r chi.Router) {
 		// MaxBodySize precedes CSRF and RequireAuth deliberately;

internal/server/routes_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/server"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -113,6 +114,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,
@@ -1027,3 +1030,46 @@ 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, Go runtime and process series, and the series counting
+// scrapes of /metrics itself.
+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",
+			"process_start_time_seconds",
+			"promhttp_metric_handler_requests_total",
+		} {
+			assert.Contains(t, scrape.Body.String(), series)
+		}
+	}
+}

internal/server/settings_test.go

@@ -1,22 +0,0 @@
-package server_test
-
-import (
-	"net/http"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestSettingsPageIsBehindLogin(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnv(t)
-
-	w := env.get("/settings", nil)
-	assert.Equal(t, http.StatusSeeOther, w.Code)
-	assert.Equal(t, "/pages/login", w.Header().Get("Location"))
-
-	w = env.get("/settings", env.authCookies(t, "id", "admin"))
-	assert.Equal(t, http.StatusOK, w.Code)
-	assert.Contains(t, w.Body.String(), "WEBHOOKER_ENVIRONMENT")
-}

templates/navbar.html

@@ -17,7 +17,6 @@
         <div class="hidden md:flex items-center gap-4">
             {{if .User}}
             <a href="/sources" class="btn-text">Webhooks</a>
-            <a href="/settings" class="btn-text">Settings</a>
             <a href="/user/{{.User.Username}}" class="btn-text">
                 <svg class="w-5 h-5 mr-1" fill="currentColor" viewBox="0 0 16 16">
                     <path d="M11 6a3 3 0 1 1-6 0 3 3 0 0 1 6 0z"/>
@@ -40,7 +39,6 @@
         <div class="flex flex-col gap-2">
             {{if .User}}
             <a href="/sources" class="btn-text w-full text-left">Webhooks</a>
-            <a href="/settings" class="btn-text w-full text-left">Settings</a>
             <a href="/user/{{.User.Username}}" class="btn-text w-full text-left">Profile</a>
             <form method="POST" action="/pages/logout">
                 <input type="hidden" name="csrf_token" value="{{.CSRFToken}}">

templates/settings.html

@@ -1,24 +0,0 @@
-{{template "base" .}}
-
-{{define "title"}}Settings - Webhooker{{end}}
-
-{{define "content"}}
-<div class="max-w-6xl mx-auto px-6 py-8">
-    <h1 class="text-2xl font-medium text-gray-900">Settings</h1>
-    <p class="text-sm text-gray-500 mt-1 mb-6">The configuration this server started with. It is set in the server's environment and cannot be changed here.</p>
-
-    <div class="card">
-        <div class="divide-y divide-gray-100">
-            {{range .Settings}}
-            <div class="p-4">
-                <div class="flex justify-between items-start gap-4">
-                    <code class="text-sm font-medium text-gray-900">{{.Name}}</code>
-                    <code class="text-sm text-gray-900 break-all">{{.Value}}</code>
-                </div>
-                <p class="text-sm text-gray-500 mt-1">{{.Description}}</p>
-            </div>
-            {{end}}
-        </div>
-    </div>
-</div>
-{{end}}