diff --git a/README.md b/README.md index 9dc720d..b39b3ff 100644 --- a/README.md +++ b/README.md @@ -640,7 +640,10 @@ See `configs/config.example.yml` for all options with defaults. - **Metrics**: Prometheus, at `/metrics`: generic HTTP request metrics (duration, response size, requests in flight) and the Go runtime and process metrics; requests are measured and `/metrics` is served only when - `metrics.username` and `metrics.password` are set + `metrics.username` and `metrics.password` are set. A request's `handler` label + is the pattern of the route it matches, such as `/v1/image/*`, never its path. + Requests that match no route share the label `unmatched`: those the router + answers 404 or 405, and CORS preflights - **Logging**: stdlib slog ## Entrypoints diff --git a/TODO.md b/TODO.md index 8d09a38..893924a 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,15 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-15 the request metrics are labelled by route, not by path (closes + #254): their `handler` label was each request's path, so every new path, a 404 + or an encrypted URL, added series to the Prometheus registry that were never + removed, and `/metrics` listed every encrypted URL's token. The label is now + the pattern of the chi route the request matches, such as `/v1/image/*`, or + `unmatched` for a request that matches none: one the router answers 404 or + 405, or a CORS preflight. The metrics library takes the label before the + request is routed, so the metrics middleware asks the router which route + matches. - 2026-10-15 expired negative cache rows are removed on a schedule (closes #244): the row of a failed fetch, which makes pixa answer 502 for that source URL for 5 minutes, was deleted only when the same URL was requested again diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index f6eb83d..a35c23e 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -8,12 +8,15 @@ import ( "net/http" "net/netip" "regexp" + "strings" "time" basicauth "github.com/99designs/basicauth-go" + "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" "github.com/go-chi/cors" "github.com/go-chi/httprate" + "github.com/prometheus/client_golang/prometheus" metrics "github.com/slok/go-http-metrics/metrics/prometheus" ghmm "github.com/slok/go-http-metrics/middleware" "github.com/slok/go-http-metrics/middleware/std" @@ -63,6 +66,10 @@ type Middleware struct { log *slog.Logger config *config.Config clientIP *clientip.Resolver + + // metricsRegistry is the registry Metrics registers its metrics with. + // nil is Prometheus' default registry, the one /metrics serves. + metricsRegistry prometheus.Registerer } // New creates a new Middleware instance. @@ -209,17 +216,65 @@ func (s *Middleware) CORS() func(http.Handler) http.Handler { }) } -// Metrics returns a Prometheus metrics middleware. +// Metrics returns a Prometheus metrics middleware. Its handler label is the +// pattern of the chi route a request matches, such as /v1/image/*, or +// "unmatched" for a request that matches none, so the routes fix how many +// values the label has. It is never the path, with which any client could +// add series to the registry without limit. func (s *Middleware) Metrics() func(http.Handler) http.Handler { mdlw := ghmm.New(ghmm.Config{ - Recorder: metrics.NewRecorder(metrics.Config{}), + Recorder: metrics.NewRecorder(metrics.Config{ + Registry: s.metricsRegistry, + }), }) return func(next http.Handler) http.Handler { - return std.Handler("", mdlw, next) + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + std.Handler(routePattern(r), mdlw, next).ServeHTTP(w, r) + }) } } +// routePattern returns the pattern of the chi route r matches, or +// "unmatched" when it matches none or is not served by a chi router. The +// metrics library takes the label before the request is routed, so the +// router is asked which route matches, with the path it routes on: +// r.URL.RawPath when set, else r.URL.Path, else /. +func routePattern(r *http.Request) string { + pattern := "" + + rctx := chi.RouteContext(r.Context()) + if rctx != nil { + path := r.URL.RawPath + if path == "" { + path = r.URL.Path + } + + if path == "" { + path = "/" + } + + pattern = rctx.Routes.Find(chi.NewRouteContext(), r.Method, path) + + // For the path a subrouter is mounted at, such as /v1 or /v1/, Find + // answers that path without asking the subrouter, which chi hands + // the request as /. + for _, route := range rctx.Routes.Routes() { + if route.SubRoutes != nil && + route.Pattern == strings.TrimSuffix(pattern, "/")+"/*" && + !route.SubRoutes.Match(chi.NewRouteContext(), r.Method, "/") { + pattern = "" + } + } + } + + if pattern == "" { + return "unmatched" + } + + return pattern +} + // MetricsAuth returns a basic auth middleware for the metrics endpoint. func (s *Middleware) MetricsAuth() func(http.Handler) http.Handler { return basicauth.New( diff --git a/internal/middleware/middleware_internal_test.go b/internal/middleware/middleware_internal_test.go index 91ba3f3..0e17a05 100644 --- a/internal/middleware/middleware_internal_test.go +++ b/internal/middleware/middleware_internal_test.go @@ -3,12 +3,15 @@ package middleware import ( "bytes" "log/slog" + "maps" "net/http" "net/http/httptest" "net/url" "strings" "testing" + "github.com/go-chi/chi/v5" + "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" "sneak.berlin/go/pixa/internal/config" @@ -198,7 +201,7 @@ func TestMetricsRecordsServedRequest(t *testing.T) { // The line /metrics shows once one GET /test has been served. const want = `http_request_duration_seconds_count{` + - `code="200",handler="/test",method="GET",service=""} 1` + `code="200",handler="unmatched",method="GET",service=""} 1` mw := &Middleware{log: slog.Default(), config: &config.Config{}} @@ -220,6 +223,97 @@ func TestMetricsRecordsServedRequest(t *testing.T) { } } +// TestMetricsLabelsRequestsByRoute checks that the metrics middleware labels +// each request with the pattern of the route that answers it, never with its +// path: two paths one route answers share that route's pattern, and requests +// no route answers, a 404 or a 405, share "unmatched", among them requests for +// the path the subrouter is mounted at, /v1 and /v1/. A request with an empty +// path is answered by the / route, as chi routes it as /, and is labelled /. +// With a label per path, any client could add series to the registry without +// limit. The middleware registers with a registry of its own here, as another +// test in this package registers with Prometheus' default registry, which can +// be done only once. +func TestMetricsLabelsRequestsByRoute(t *testing.T) { + t.Parallel() + + registry := prometheus.NewRegistry() + mw := &Middleware{ + log: slog.Default(), + config: &config.Config{}, + metricsRegistry: registry, + } + + // The image route is in a subrouter, as in pixa's router. + router := chi.NewRouter() + router.Use(mw.Metrics()) + router.Get("/", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + router.Route("/v1", func(r chi.Router) { + r.Get("/image/*", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + }) + + for _, req := range []struct{ method, path string }{ + {http.MethodGet, "/v1/image/a.example.com/one.jpg/1x1.png"}, + {http.MethodGet, "/v1/image/b.example.com/two.jpg/1x1.png"}, + {http.MethodGet, "/no-such-route"}, + {http.MethodGet, "/v1/no-such-route"}, + {http.MethodGet, "/v1"}, + {http.MethodGet, "/v1/"}, + {http.MethodPost, "/v1/image/a.example.com/one.jpg/1x1.png"}, + // The request line GET http://pixa.example HTTP/1.1 leaves both + // r.URL.Path and r.URL.RawPath empty. + {http.MethodGet, "http://pixa.example"}, + } { + r := httptest.NewRequestWithContext( + t.Context(), req.method, req.path, nil) + t.Logf("%s %s: r.URL.Path %q, r.URL.RawPath %q", + req.method, req.path, r.URL.Path, r.URL.RawPath) + router.ServeHTTP(httptest.NewRecorder(), r) + } + + got := requestCountsByHandler(t, registry) + t.Logf("requests counted by handler label: %v", got) + + want := map[string]uint64{"/": 1, "/v1/image/*": 2, "unmatched": 5} + if !maps.Equal(got, want) { + t.Errorf("requests counted by handler label = %v, want %v", got, want) + } +} + +// requestCountsByHandler returns how many requests the request duration +// histogram in registry counted under each value of its handler label. +func requestCountsByHandler( + t *testing.T, registry *prometheus.Registry, +) map[string]uint64 { + t.Helper() + + families, err := registry.Gather() + if err != nil { + t.Fatalf("registry.Gather() error = %v", err) + } + + counts := map[string]uint64{} + + for _, family := range families { + if family.GetName() != "http_request_duration_seconds" { + continue + } + + for _, metric := range family.GetMetric() { + for _, label := range metric.GetLabel() { + if label.GetName() == "handler" { + counts[label.GetValue()] += metric.GetHistogram().GetSampleCount() + } + } + } + } + + return counts +} + // TestLoggingLeavesOutSubmittedSigningKey checks that a login, a POST / // whose form carries the signing key, leaves no trace of the key in the // request's log line.