diff --git a/ingestion/cmd/server/routes_test.go b/ingestion/cmd/server/routes_test.go index be538fb..1e8971f 100644 --- a/ingestion/cmd/server/routes_test.go +++ b/ingestion/cmd/server/routes_test.go @@ -159,3 +159,16 @@ func TestBuildMuxObservesRequestsAndServesMetrics(t *testing.T) { require.Equal(t, http.StatusOK, code) assert.Contains(t, body, `path="GET /pending"`) } + +// hyperguild#185: an authenticated request to an unregistered path reached +// the inner REST mux, which left Pattern empty, and the metrics label fell +// back to the raw URL path: one Prometheus series per unique path any token +// holder sends. It is now labelled with a fixed catch-all. +func TestBuildMuxLabelsAnUnregisteredPathWithAFixedLabel(t *testing.T) { + r := testMux(t) + code, _ := do(t, r, "GET", "/zqrandom-185-segment", "Bearer "+routesTok) + require.Equal(t, http.StatusNotFound, code) + _, body := do(t, r, "GET", "/metrics", "") + assert.NotContains(t, body, "zqrandom-185-segment", "the raw path became a metrics label") + assert.Contains(t, body, `path="unmatched"`) +} diff --git a/ingestion/internal/metrics/metrics.go b/ingestion/internal/metrics/metrics.go index 3ace296..9968648 100644 --- a/ingestion/internal/metrics/metrics.go +++ b/ingestion/internal/metrics/metrics.go @@ -83,8 +83,9 @@ func (r *Registry) Observe(path, status string, d time.Duration) { // Middleware wraps next, observing every request's duration + status. // The metric label `path` uses the request's Pattern (Go 1.22+ ServeMux), -// falling back to the URL path if no Pattern is set. Pattern keeps -// cardinality bounded (one series per route, not one per unique URL). +// or the fixed label "unmatched" if no Pattern is set: falling back to the +// raw URL path gave any caller one series per unique path (hyperguild#185). +// Pattern keeps cardinality bounded (one series per route). func (r *Registry) Middleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { rec := &statusRecorder{ResponseWriter: w, code: http.StatusOK} @@ -92,7 +93,7 @@ func (r *Registry) Middleware(next http.Handler) http.Handler { next.ServeHTTP(rec, req) path := req.Pattern if path == "" { - path = req.URL.Path + path = "unmatched" } r.Observe(path, statusClass(rec.code), time.Since(start)) }) diff --git a/ingestion/internal/metrics/metrics_test.go b/ingestion/internal/metrics/metrics_test.go index c437905..e09709d 100644 --- a/ingestion/internal/metrics/metrics_test.go +++ b/ingestion/internal/metrics/metrics_test.go @@ -71,11 +71,16 @@ func TestMiddleware_RecordsTiming(t *testing.T) { t.Parallel() r := New() - handler := r.Middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Routed through a ServeMux, as in production: the mux sets the + // request's Pattern, which is the label (an unrouted request is + // "unmatched", hyperguild#185). + mux := http.NewServeMux() + mux.HandleFunc("/query", func(w http.ResponseWriter, _ *http.Request) { time.Sleep(2 * time.Millisecond) w.WriteHeader(http.StatusOK) _, _ = io.WriteString(w, "ok") - })) + }) + handler := r.Middleware(mux) srv := httptest.NewServer(handler) defer srv.Close() @@ -117,3 +122,19 @@ func TestStatusRecorder_DefaultsTo200(t *testing.T) { t.Errorf("code %d, want 200", rec.Code) } } + +// A request with no matched Pattern is labelled "unmatched", never with its +// raw URL path (unbounded cardinality, hyperguild#185). +func TestMiddleware_LabelsAnUnmatchedRequestWithAFixedLabel(t *testing.T) { + r := New() + h := r.Middleware(http.NotFoundHandler()) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/zqrandom-185-unit", nil)) + rec := httptest.NewRecorder() + r.Handler().ServeHTTP(rec, httptest.NewRequest("GET", "/metrics", nil)) + if strings.Contains(rec.Body.String(), "zqrandom-185-unit") { + t.Errorf("the raw path became a metrics label:\n%s", rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), `path="unmatched"`) { + t.Errorf("no unmatched label:\n%s", rec.Body.String()) + } +}