Skip to content

Commit e45f3b3

Browse files
authored
fix(observability): label HTTP metrics by route pattern, not raw path (#1791)
## Bug With `observability.metrics.enabled`, `MetricsManager.HTTPMiddleware` is the first middleware on the REST API chi router (`setupRoutes`), ahead of `apiKeyAuthMiddleware`. It labels `mcpproxy_http_requests_total` and `mcpproxy_http_request_duration_seconds` with `r.URL.Path`. So every distinct path creates a new counter series and a new histogram series (11 buckets plus sum and count), and they are never removed. That includes unauthenticated requests that end in 401 or 404, and every server name in `/api/v1/servers/{name}/…`. ## Trigger With metrics enabled, any client that can reach the listener sends requests to distinct paths, for example `GET /api/v1/<random>` in a loop. A web page open in the user's browser can do this against `127.0.0.1` with `fetch(..., {mode: "no-cors"})`, and no API key is needed. The daemon's memory and the size of every `/metrics` scrape then grow without bound. ## Fix Label requests with the chi route pattern they matched (`/api/v1/servers/{name}`). A request that matched no route gets one fixed `unmatched` label. Outside a chi router, which no production caller uses, the raw path is kept. The label name (`path`) and the other labels are unchanged, so the Grafana dashboard, which sums by `status`, keeps working. ## Test `TestHTTPMiddleware_PathLabelIsRoutePattern`: a chi router with one `/api/v1/servers/{name}` route. It receives requests for two server names and two unknown paths. It must end up with exactly two series: the route pattern with count 2, and `unmatched` with count 2. ## Commands run - `CGO_ENABLED=0 go test -count=1 -run 'Test' ./internal/observability/`: ok - On current `main`, and again with the fix reverted, the new test fails with `expected: 2, actual: 4` series. - `go vet ./internal/observability/`: ok
1 parent 8b8a88f commit e45f3b3

2 files changed

Lines changed: 44 additions & 1 deletion

File tree

‎internal/observability/metrics.go‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"net/http"
55
"time"
66

7+
"github.com/go-chi/chi/v5"
78
"github.com/prometheus/client_golang/prometheus"
89
"github.com/prometheus/client_golang/prometheus/collectors"
910
"github.com/prometheus/client_golang/prometheus/promhttp"
@@ -431,11 +432,30 @@ func (mm *MetricsManager) HTTPMiddleware() func(http.Handler) http.Handler {
431432

432433
// Record metrics
433434
duration := time.Since(start)
434-
mm.RecordHTTPRequest(r.Method, r.URL.Path, http.StatusText(ww.statusCode), duration)
435+
mm.RecordHTTPRequest(r.Method, routeLabel(r), http.StatusText(ww.statusCode), duration)
435436
})
436437
}
437438
}
438439

440+
// unmatchedRouteLabel is the path label for a request no route matched.
441+
const unmatchedRouteLabel = "unmatched"
442+
443+
// routeLabel is the path label for r: the chi route pattern it matched
444+
// ("/api/v1/servers/{name}"), never the raw path. The middleware runs before
445+
// authentication, so a raw-path label would let any caller add a counter and
446+
// a histogram series per distinct path it sends. Outside a chi router the raw
447+
// path is kept.
448+
func routeLabel(r *http.Request) string {
449+
rctx := chi.RouteContext(r.Context())
450+
if rctx == nil {
451+
return r.URL.Path
452+
}
453+
if pattern := rctx.RoutePattern(); pattern != "" {
454+
return pattern
455+
}
456+
return unmatchedRouteLabel
457+
}
458+
439459
// responseWriter wraps http.ResponseWriter to capture status code
440460
type responseWriter struct {
441461
http.ResponseWriter

‎internal/observability/metrics_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
package observability
22

33
import (
4+
"net/http"
5+
"net/http/httptest"
46
"strings"
57
"testing"
68
"time"
79

10+
"github.com/go-chi/chi/v5"
811
"github.com/prometheus/client_golang/prometheus/testutil"
912
"github.com/stretchr/testify/assert"
1013
"github.com/stretchr/testify/require"
@@ -55,6 +58,26 @@ func TestToolCallMetric_NoPerUserCardinalityLabels(t *testing.T) {
5558
assert.NotContains(t, body, "profile=", "tool-call metric must not carry a profile label")
5659
}
5760

61+
// The HTTP middleware runs before authentication, so its path label must be
62+
// the matched route, not the raw request path: otherwise every distinct path
63+
// anyone sends adds a counter and a histogram series that is never freed.
64+
func TestHTTPMiddleware_PathLabelIsRoutePattern(t *testing.T) {
65+
mm := NewMetricsManager(zap.NewNop().Sugar())
66+
r := chi.NewRouter()
67+
r.Use(mm.HTTPMiddleware())
68+
r.Get("/api/v1/servers/{name}", func(w http.ResponseWriter, _ *http.Request) {
69+
w.WriteHeader(http.StatusOK)
70+
})
71+
72+
for _, p := range []string{"/api/v1/servers/a", "/api/v1/servers/b", "/api/v1/nope-1", "/api/v1/nope-2"} {
73+
r.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, p, nil))
74+
}
75+
76+
assert.Equal(t, 2, testutil.CollectAndCount(mm.httpRequests), "one series per route plus one for unmatched paths")
77+
assert.InDelta(t, 2.0, testutil.ToFloat64(mm.httpRequests.WithLabelValues(http.MethodGet, "/api/v1/servers/{name}", "OK")), 1e-9)
78+
assert.InDelta(t, 2.0, testutil.ToFloat64(mm.httpRequests.WithLabelValues(http.MethodGet, "unmatched", "Not Found")), 1e-9)
79+
}
80+
5881
func scrapeMetrics(t *testing.T, mm *MetricsManager) string {
5982
t.Helper()
6083
families, err := mm.registry.Gather()

0 commit comments

Comments
 (0)