diff --git a/cmd/webhooker/main.go b/cmd/webhooker/main.go index 0da8fa0..1b81b2e 100644 --- a/cmd/webhooker/main.go +++ b/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 diff --git a/internal/delivery/engine.go b/internal/delivery/engine.go index 4786f08..c718853 100644 --- a/internal/delivery/engine.go +++ b/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{ diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index 9dd2531..db2d000 100644 --- a/internal/delivery/export_test.go +++ b/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 } diff --git a/internal/delivery/metrics_test.go b/internal/delivery/metrics_test.go index c9d95a2..e7dccba 100644 --- a/internal/delivery/metrics_test.go +++ b/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 { diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 349771e..34d8623 100644 --- a/internal/handlers/handlers.go +++ b/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 diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 3e9874a..efbe1c7 100644 --- a/internal/handlers/handlers_test.go +++ b/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, diff --git a/internal/handlers/metrics.go b/internal/handlers/metrics.go new file mode 100644 index 0000000..c6f3f37 --- /dev/null +++ b/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 +} diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 1b3d68e..92f1d31 100644 --- a/internal/metrics/metrics.go +++ b/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 -// 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. 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 +57,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 +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{ diff --git a/internal/middleware/export_test.go b/internal/middleware/export_test.go index 76ed406..457bae9 100644 --- a/internal/middleware/export_test.go +++ b/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 { diff --git a/internal/middleware/metrics.go b/internal/middleware/metrics.go index 6ac39be..a599e2c 100644 --- a/internal/middleware/metrics.go +++ b/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( - 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 { diff --git a/internal/middleware/metrics_test.go b/internal/middleware/metrics_test.go index d19c30c..44986f0 100644 --- a/internal/middleware/metrics_test.go +++ b/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, diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 98140fb..dfc47fc 100644 --- a/internal/middleware/middleware.go +++ b/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,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 = ¶ms s.log = params.Logger.Get() s.session = params.Session + s.metricsRecorder = prommetrics.NewRecorder( + prommetrics.Config{Registry: params.Registry}, + ) return s, nil } diff --git a/internal/resetpw/resetpw_test.go b/internal/resetpw/resetpw_test.go index cca9107..1ebead5 100644 --- a/internal/resetpw/resetpw_test.go +++ b/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, diff --git a/internal/server/routes.go b/internal/server/routes.go index 7f3699a..0c8a6f7 100644 --- a/internal/server/routes.go +++ b/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( - "/metrics", - http.HandlerFunc( - promhttp.Handler().ServeHTTP, - ), - ) + r.Get("/metrics", s.h.HandleMetrics()) }) } diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index 429a7f7..c39f498 100644 --- a/internal/server/routes_test.go +++ b/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) + } + } +}