diff --git a/internal/observability/metrics.go b/internal/observability/metrics.go index d7cc32e19..e922e3a45 100644 --- a/internal/observability/metrics.go +++ b/internal/observability/metrics.go @@ -4,6 +4,7 @@ import ( "net/http" "time" + "github.com/go-chi/chi/v5" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/collectors" "github.com/prometheus/client_golang/prometheus/promhttp" @@ -431,11 +432,30 @@ func (mm *MetricsManager) HTTPMiddleware() func(http.Handler) http.Handler { // Record metrics duration := time.Since(start) - mm.RecordHTTPRequest(r.Method, r.URL.Path, http.StatusText(ww.statusCode), duration) + mm.RecordHTTPRequest(r.Method, routeLabel(r), http.StatusText(ww.statusCode), duration) }) } } +// unmatchedRouteLabel is the path label for a request no route matched. +const unmatchedRouteLabel = "unmatched" + +// routeLabel is the path label for r: the chi route pattern it matched +// ("/api/v1/servers/{name}"), never the raw path. The middleware runs before +// authentication, so a raw-path label would let any caller add a counter and +// a histogram series per distinct path it sends. Outside a chi router the raw +// path is kept. +func routeLabel(r *http.Request) string { + rctx := chi.RouteContext(r.Context()) + if rctx == nil { + return r.URL.Path + } + if pattern := rctx.RoutePattern(); pattern != "" { + return pattern + } + return unmatchedRouteLabel +} + // responseWriter wraps http.ResponseWriter to capture status code type responseWriter struct { http.ResponseWriter diff --git a/internal/observability/metrics_test.go b/internal/observability/metrics_test.go index b4e7dbcca..653c02929 100644 --- a/internal/observability/metrics_test.go +++ b/internal/observability/metrics_test.go @@ -1,10 +1,13 @@ package observability import ( + "net/http" + "net/http/httptest" "strings" "testing" "time" + "github.com/go-chi/chi/v5" "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -55,6 +58,26 @@ func TestToolCallMetric_NoPerUserCardinalityLabels(t *testing.T) { assert.NotContains(t, body, "profile=", "tool-call metric must not carry a profile label") } +// The HTTP middleware runs before authentication, so its path label must be +// the matched route, not the raw request path: otherwise every distinct path +// anyone sends adds a counter and a histogram series that is never freed. +func TestHTTPMiddleware_PathLabelIsRoutePattern(t *testing.T) { + mm := NewMetricsManager(zap.NewNop().Sugar()) + r := chi.NewRouter() + r.Use(mm.HTTPMiddleware()) + r.Get("/api/v1/servers/{name}", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + for _, p := range []string{"/api/v1/servers/a", "/api/v1/servers/b", "/api/v1/nope-1", "/api/v1/nope-2"} { + r.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, p, nil)) + } + + assert.Equal(t, 2, testutil.CollectAndCount(mm.httpRequests), "one series per route plus one for unmatched paths") + assert.InDelta(t, 2.0, testutil.ToFloat64(mm.httpRequests.WithLabelValues(http.MethodGet, "/api/v1/servers/{name}", "OK")), 1e-9) + assert.InDelta(t, 2.0, testutil.ToFloat64(mm.httpRequests.WithLabelValues(http.MethodGet, "unmatched", "Not Found")), 1e-9) +} + func scrapeMetrics(t *testing.T, mm *MetricsManager) string { t.Helper() families, err := mm.registry.Gather()