diff --git a/agent/server/http/http_server.go b/agent/server/http/http_server.go index e5b681d..7cca6d7 100644 --- a/agent/server/http/http_server.go +++ b/agent/server/http/http_server.go @@ -223,8 +223,9 @@ func (h *httpServer) requestMiddleware(next http.Handler) http.Handler { ) next.ServeHTTP(rec, r) duration := time.Since(start) - h.requestCounter.WithLabelValues(r.Method, r.URL.Path, fmt.Sprintf("%d", rec.statusCode)).Inc() - h.requestLatency.WithLabelValues(r.Method, r.URL.Path, fmt.Sprintf("%d", rec.statusCode)).Observe(duration.Seconds()) + path := metricPath(r) + h.requestCounter.WithLabelValues(r.Method, path, fmt.Sprintf("%d", rec.statusCode)).Inc() + h.requestLatency.WithLabelValues(r.Method, path, fmt.Sprintf("%d", rec.statusCode)).Observe(duration.Seconds()) h.logger.Info("<== HTTP request", zap.String("method", r.Method), zap.String("path", r.URL.Path), @@ -236,6 +237,26 @@ func (h *httpServer) requestMiddleware(next http.Handler) http.Handler { }) } +// unmatchedMetricPath is the "path" metric label used when a request has no +// resolvable route template. It is a fixed value so it can never contribute to +// unbounded label cardinality. +const unmatchedMetricPath = "" + +// metricPath returns the value to use for the "path" label of the request +// metrics. It uses the matched mux route template (e.g. "/webhook/" or +// "/webhook/{id}") rather than the raw request path, so that requests carrying +// unique path segments (webhook ids, resource ids, uuids, ...) collapse to a +// single bounded series instead of leaking one permanent counter + histogram +// per distinct URL. +func metricPath(r *http.Request) string { + if route := mux.CurrentRoute(r); route != nil { + if tmpl, err := route.GetPathTemplate(); err == nil && tmpl != "" { + return tmpl + } + } + return unmatchedMetricPath +} + var defaultReadTimeout = time.Second func (h *httpServer) Start() (int, error) { diff --git a/agent/server/http/http_server_metrics_test.go b/agent/server/http/http_server_metrics_test.go new file mode 100644 index 0000000..a6b7729 --- /dev/null +++ b/agent/server/http/http_server_metrics_test.go @@ -0,0 +1,113 @@ +package http + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/cortexapps/axon/config" + "github.com/gorilla/mux" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +// routeHandler registers a single route with the given pattern and always +// responds 200. It lets tests drive the request middleware through mux the +// same way production handlers do. +type routeHandler struct { + register func(m *mux.Router, h http.Handler) +} + +func (h *routeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) +} + +func (h *routeHandler) RegisterRoutes(m *mux.Router) error { + h.register(m, h) + return nil +} + +func newMetricsTestServer(t *testing.T, registry *prometheus.Registry, handlers ...RegisterableHandler) *httpServer { + t.Helper() + params := HttpServerParams{ + Logger: zap.NewNop(), + Config: config.AgentConfig{}, + Handlers: handlers, + Registry: registry, + } + srv := NewHttpServer(params).(*httpServer) + t.Cleanup(func() { _ = srv.Close() }) + return srv +} + +// pathLabelValues returns the "path" label value of every series of metricName. +func pathLabelValues(t *testing.T, registry *prometheus.Registry, metricName string) []string { + t.Helper() + families, err := registry.Gather() + require.NoError(t, err) + + var values []string + for _, fam := range families { + if fam.GetName() != metricName { + continue + } + for _, m := range fam.GetMetric() { + for _, lp := range m.GetLabel() { + if lp.GetName() == "path" { + values = append(values, lp.GetValue()) + } + } + } + } + return values +} + +// A PathPrefix route (the shape used by the webhook handler) is the source of +// the cardinality explosion: each distinct /webhook/ URL used to create its +// own permanent counter + histogram series. All distinct paths must collapse to +// the single registered route template. +func TestRequestMetricsCollapsePrefixRouteToTemplate(t *testing.T) { + registry := prometheus.NewRegistry() + srv := newMetricsTestServer(t, registry, &routeHandler{ + register: func(m *mux.Router, h http.Handler) { m.PathPrefix("/webhook/").Handler(h) }, + }) + + ts := httptest.NewServer(srv.mux) + defer ts.Close() + + for i := 0; i < 25; i++ { + resp, err := http.Get(fmt.Sprintf("%s/webhook/id-%d", ts.URL, i)) + require.NoError(t, err) + _ = resp.Body.Close() + } + + for _, metric := range []string{"axon_http_requests", "axon_http_request_latency_seconds"} { + require.Equal(t, []string{"/webhook/"}, pathLabelValues(t, registry, metric), + "%s: distinct request paths must collapse to a single route-template series", metric) + } +} + +// A route with a path variable must record the template (/webhook/{id}), not +// each concrete value. +func TestRequestMetricsCollapseVariableRouteToTemplate(t *testing.T) { + registry := prometheus.NewRegistry() + srv := newMetricsTestServer(t, registry, &routeHandler{ + register: func(m *mux.Router, h http.Handler) { m.Handle("/webhook/{id}", h) }, + }) + + ts := httptest.NewServer(srv.mux) + defer ts.Close() + + for i := 0; i < 25; i++ { + resp, err := http.Get(fmt.Sprintf("%s/webhook/id-%d", ts.URL, i)) + require.NoError(t, err) + _ = resp.Body.Close() + } + + for _, metric := range []string{"axon_http_requests", "axon_http_request_latency_seconds"} { + require.Equal(t, []string{"/webhook/{id}"}, pathLabelValues(t, registry, metric), + "%s: distinct request paths must collapse to a single route-template series", metric) + } +}