fix(metrics): label an unmatched request "unmatched", never its raw path (#185)
An authenticated REST request to an unregistered path reached the inner mux, which left Pattern empty, and metrics.Middleware fell back to the raw URL path: any token holder could create one Prometheus series per unique path. The fallback is now the fixed label "unmatched"; real routes keep their pattern. Tests: buildMux with an authenticated random path, and the middleware unit test; the timing test now routes through a ServeMux as production does. Fixes #185. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -159,3 +159,16 @@ func TestBuildMuxObservesRequestsAndServesMetrics(t *testing.T) {
|
|||||||
require.Equal(t, http.StatusOK, code)
|
require.Equal(t, http.StatusOK, code)
|
||||||
assert.Contains(t, body, `path="GET /pending"`)
|
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"`)
|
||||||
|
}
|
||||||
|
|||||||
@@ -83,8 +83,9 @@ func (r *Registry) Observe(path, status string, d time.Duration) {
|
|||||||
|
|
||||||
// Middleware wraps next, observing every request's duration + status.
|
// Middleware wraps next, observing every request's duration + status.
|
||||||
// The metric label `path` uses the request's Pattern (Go 1.22+ ServeMux),
|
// 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
|
// or the fixed label "unmatched" if no Pattern is set: falling back to the
|
||||||
// cardinality bounded (one series per route, not one per unique URL).
|
// 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 {
|
func (r *Registry) Middleware(next http.Handler) http.Handler {
|
||||||
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
|
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
|
||||||
rec := &statusRecorder{ResponseWriter: w, code: http.StatusOK}
|
rec := &statusRecorder{ResponseWriter: w, code: http.StatusOK}
|
||||||
@@ -92,7 +93,7 @@ func (r *Registry) Middleware(next http.Handler) http.Handler {
|
|||||||
next.ServeHTTP(rec, req)
|
next.ServeHTTP(rec, req)
|
||||||
path := req.Pattern
|
path := req.Pattern
|
||||||
if path == "" {
|
if path == "" {
|
||||||
path = req.URL.Path
|
path = "unmatched"
|
||||||
}
|
}
|
||||||
r.Observe(path, statusClass(rec.code), time.Since(start))
|
r.Observe(path, statusClass(rec.code), time.Since(start))
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -71,11 +71,16 @@ func TestMiddleware_RecordsTiming(t *testing.T) {
|
|||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
r := New()
|
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)
|
time.Sleep(2 * time.Millisecond)
|
||||||
w.WriteHeader(http.StatusOK)
|
w.WriteHeader(http.StatusOK)
|
||||||
_, _ = io.WriteString(w, "ok")
|
_, _ = io.WriteString(w, "ok")
|
||||||
}))
|
})
|
||||||
|
handler := r.Middleware(mux)
|
||||||
|
|
||||||
srv := httptest.NewServer(handler)
|
srv := httptest.NewServer(handler)
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
@@ -117,3 +122,19 @@ func TestStatusRecorder_DefaultsTo200(t *testing.T) {
|
|||||||
t.Errorf("code %d, want 200", rec.Code)
|
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())
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user