From 7956da1f59d8499dbc5baab9649a115dffe67df8 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 15 Oct 2026 16:37:20 +0200 Subject: [PATCH] Label request metrics by route, not by path (closes #254) The metrics middleware labelled each request with its path, so every new path added series to the registry that were never removed, and /metrics listed the token of every encrypted URL requested. The handler label is now the pattern of the chi route the request will match, looked up before routing, or "unmatched" when none does. /v1 and /v1/ ask the image subrouter, and an empty path is looked up as /, as chi routes it. TestMetricsRecordsServedRequest expects "unmatched" in place of "/test", as the owner approved. Model: opus-5-5 --- README.md | 5 +- TODO.md | 9 ++ internal/middleware/middleware.go | 61 +++++++++++- .../middleware/middleware_internal_test.go | 96 ++++++++++++++++++- 4 files changed, 166 insertions(+), 5 deletions(-) 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.