Bound the /metrics method label (closes #261)

internal/middleware/export_test.go

@@ -26,6 +26,10 @@ const UnmatchedRouteConst = unmatchedRoute
 // inflight gauge.
 const InflightHandlerConst = inflightHandler
 
+// UnmatchedMethodConst exposes the sentinel that stands in for a
+// method the router can never route.
+const UnmatchedMethodConst = unmatchedMethod
+
 // NewLoggingResponseWriterForTest wraps newLoggingResponseWriter
 // for use in external test packages.
 func NewLoggingResponseWriterForTest(

internal/middleware/metrics.go

@@ -26,6 +26,15 @@ import (
 // counting the requests in flight across the whole service.
 const inflightHandler = "(all)"
 
+// unmatchedMethod is the `method` label for a request whose method
+// the router can never route.
+//
+// It is deliberately the same sentinel as unmatchedRoute rather than
+// a spelling of its own: both stand for a client-chosen token that
+// matched nothing this service registers, and giving one idea two
+// spellings would read in a scrape as two different unmatched states.
+const unmatchedMethod = unmatchedRoute
+
 // routePatternID is the `handler` label for a request: the chi route
 // pattern, never the concrete path.
 //
@@ -50,9 +59,45 @@ func routePatternID(ctx context.Context) string {
 	return unmatchedRoute
 }
 
-// routePatternRecorder wraps a go-http-metrics recorder and replaces
-// the handler id on every observation with the request's route
-// pattern.
+// methodID is the `method` label for a request: the request method
+// when the router can route it, and the unmatched sentinel otherwise.
+//
+// net/http accepts any RFC 9110 token as a method and hands it
+// through verbatim, so the raw method is client-chosen bytes and
+// bounds the label at nothing — the same unauthenticated
+// series-minting the handler label carried, reached through a second
+// dimension. What bounds it is the set chi's router will match a
+// route for: its methodMap, which is unexported, so it is restated
+// here against the net/http constants it is built from. A token
+// outside that set can only ever produce chi's 405, so folding every
+// one of them onto a single series loses no information a scrape
+// could have used, while the nine methods that can reach a handler
+// stay distinguishable.
+//
+// chi.RegisterMethod would extend the router's set at runtime; this
+// service never calls it, and a caller that started to would have to
+// extend this switch with it.
+func methodID(method string) string {
+	switch method {
+	case http.MethodConnect,
+		http.MethodDelete,
+		http.MethodGet,
+		http.MethodHead,
+		http.MethodOptions,
+		http.MethodPatch,
+		http.MethodPost,
+		http.MethodPut,
+		http.MethodTrace:
+		return method
+	default:
+		return unmatchedMethod
+	}
+}
+
+// boundedLabelRecorder wraps a go-http-metrics recorder and replaces
+// the request-controlled labels on every observation with bounded
+// ones: the handler id becomes the request's route pattern, and the
+// method becomes one the router can route.
 //
 // This is the seam that makes the pattern usable at all. The metrics
 // middleware is global (see Server.setupGlobalMiddleware), so it is
@@ -71,29 +116,31 @@ func routePatternID(ctx context.Context) string {
 // Those never reach a handler, but chi has already matched the route
 // by the time the limiter runs, so their 429s land on the pattern
 // like any other response.
-type routePatternRecorder struct {
+type boundedLabelRecorder struct {
 	inner httpmetrics.Recorder
 }
 
-func (r routePatternRecorder) ObserveHTTPRequestDuration(
+func (r boundedLabelRecorder) ObserveHTTPRequestDuration(
 	ctx context.Context,
 	props httpmetrics.HTTPReqProperties,
 	duration time.Duration,
 ) {
 	props.ID = routePatternID(ctx)
+	props.Method = methodID(props.Method)
 	r.inner.ObserveHTTPRequestDuration(ctx, props, duration)
 }
 
-func (r routePatternRecorder) ObserveHTTPResponseSize(
+func (r boundedLabelRecorder) ObserveHTTPResponseSize(
 	ctx context.Context,
 	props httpmetrics.HTTPReqProperties,
 	sizeBytes int64,
 ) {
 	props.ID = routePatternID(ctx)
+	props.Method = methodID(props.Method)
 	r.inner.ObserveHTTPResponseSize(ctx, props, sizeBytes)
 }
 
-func (r routePatternRecorder) AddInflightRequests(
+func (r boundedLabelRecorder) AddInflightRequests(
 	ctx context.Context,
 	props httpmetrics.HTTPProperties,
 	quantity int,
@@ -102,7 +149,7 @@ func (r routePatternRecorder) AddInflightRequests(
 	r.inner.AddInflightRequests(ctx, props, quantity)
 }
 
-var _ httpmetrics.Recorder = routePatternRecorder{}
+var _ httpmetrics.Recorder = boundedLabelRecorder{}
 
 // Metrics returns middleware that records Prometheus HTTP metrics on
 // the default registry, which is the one the /metrics route gathers.
@@ -119,14 +166,14 @@ func metricsMiddleware(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {
 	mdlw := ghmm.New(ghmm.Config{
-		Recorder: routePatternRecorder{inner: rec},
+		Recorder: boundedLabelRecorder{inner: rec},
 	})
 
 	return func(next http.Handler) http.Handler {
 		// The handler id is unmatchedRoute rather than "" so that
 		// the client-chosen URL path never enters the metrics
 		// pipeline at all: an empty id is the library's signal to
-		// substitute it. routePatternRecorder overwrites this value
+		// substitute it. boundedLabelRecorder overwrites this value
 		// on every observation, so it is reachable only if that
 		// decorator is removed — in which case the metrics collapse
 		// to one series instead of leaking again.

internal/middleware/metrics_method_test.go

@@ -0,0 +1,309 @@
+package middleware_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/promhttp"
+	dto "github.com/prometheus/client_model/go"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/middleware"
+)
+
+const (
+	// metricsProbeMethods is how many distinct invented method tokens
+	// each cardinality assertion drives. The measurement on the issue
+	// took 300 tokens from 106 exposition lines to 7,631 — about 25
+	// permanent lines per token, never reclaimed — so a probe of this
+	// size puts a regression thousands of lines over the bound rather
+	// than leaving it to a rounding argument.
+	metricsProbeMethods = 300
+
+	// probeMethodLen is how many characters each invented method
+	// token carries, matching the 12 the issue measured with.
+	probeMethodLen = 12
+
+	// methodLabel is the label these tests are about.
+	methodLabel = "method"
+)
+
+// realMethods is the positive control's domain: the methods chi's
+// router can match a route for, every one of which a client
+// legitimately sends and every one of which must keep a series of its
+// own. Bounding the label by collapsing these into one bucket would
+// destroy the metric it is meant to protect.
+func realMethods() []string {
+	return []string{
+		http.MethodConnect, http.MethodDelete, http.MethodGet,
+		http.MethodHead, http.MethodOptions, http.MethodPatch,
+		http.MethodPost, http.MethodPut, http.MethodTrace,
+	}
+}
+
+// methodProbePath returns the one receiver path a method probe
+// targets. Holding the path fixed leaves the method as the only
+// dimension varying, so any series growth a probe produces is the
+// method label's and nothing else's.
+func methodProbePath() string {
+	return "/webhook/" + uuid.NewString()
+}
+
+// inventedMethods returns n distinct RFC 9110 method tokens that no
+// router will ever match: uppercase hex from a fresh UUID, which is
+// both the shape and the length an unauthenticated flood would send.
+// net/http accepts any token as a method, so every one of these
+// reaches the metrics pipeline exactly as a real method does.
+func inventedMethods(n int) []string {
+	methods := make([]string, 0, n)
+
+	for range n {
+		token := strings.ToUpper(
+			strings.ReplaceAll(uuid.NewString(), "-", ""),
+		)
+		methods = append(methods, token[:probeMethodLen])
+	}
+
+	return methods
+}
+
+// driveMethods sends one request per supplied method to a single
+// fixed path.
+func driveMethods(
+	t *testing.T,
+	h http.Handler,
+	path string,
+	methods []string,
+) map[int]int {
+	t.Helper()
+
+	probes := make([]probe, 0, len(methods))
+
+	for _, m := range methods {
+		probes = append(probes, probe{method: m, path: path})
+	}
+
+	return drive(t, h, probes)
+}
+
+// methodLabels returns the set of distinct `method` values across
+// every gathered series that carries the label at all. The inflight
+// gauge does not carry it, and so contributes nothing rather than an
+// empty-string member.
+func methodLabels(families []*dto.MetricFamily) map[string]struct{} {
+	seen := make(map[string]struct{})
+
+	for _, fam := range families {
+		for _, m := range fam.GetMetric() {
+			for _, pair := range m.GetLabel() {
+				if pair.GetName() == methodLabel {
+					seen[pair.GetValue()] = struct{}{}
+				}
+			}
+		}
+	}
+
+	return seen
+}
+
+// scrapeLines renders the registry through the same promhttp handler
+// /metrics is mounted on and counts the sample lines it produced.
+//
+// This is the quantity the issue measured and the one a Prometheus
+// server pays for on every scrape: one histogram label set is a
+// single gathered series but around 25 lines of exposition, which is
+// why 300 method tokens cost thousands of lines rather than hundreds.
+func scrapeLines(t *testing.T, reg *prometheus.Registry) int {
+	t.Helper()
+
+	h := promhttp.HandlerFor(reg, promhttp.HandlerOpts{})
+	req := httptest.NewRequestWithContext(
+		t.Context(), http.MethodGet, "/metrics", nil,
+	)
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, req)
+	require.Equal(t, http.StatusOK, w.Code)
+
+	lines := 0
+
+	for line := range strings.SplitSeq(w.Body.String(), "\n") {
+		if line == "" || strings.HasPrefix(line, "#") {
+			continue
+		}
+
+		lines++
+	}
+
+	return lines
+}
+
+// TestMetrics_MethodSentinelIsTheRouteSentinel pins the convention
+// rather than the mechanism. An unroutable method and an unmatched
+// path are the same fact — a client-chosen token matching nothing
+// this service registers — so they carry one spelling. Two spellings
+// would read in a scrape as two different unmatched states.
+func TestMetrics_MethodSentinelIsTheRouteSentinel(t *testing.T) {
+	t.Parallel()
+
+	assert.Equal(
+		t,
+		middleware.UnmatchedRouteConst,
+		middleware.UnmatchedMethodConst,
+		"the unmatched sentinel must have exactly one spelling",
+	)
+}
+
+// TestMetrics_InventedMethodsMintOneLabelSet is the direct assertion
+// the issue asks for: N requests carrying N distinct invented method
+// tokens must produce exactly ONE method label. Before the fix this
+// produced N of them, on an unauthenticated route with no rate
+// limiter.
+func TestMetrics_InventedMethodsMintOneLabelSet(t *testing.T) {
+	t.Parallel()
+
+	h, reg := metricsTestRouter(t, generousReceiverLimit)
+
+	methods := inventedMethods(metricsProbeMethods)
+
+	codes := driveMethods(t, h, methodProbePath(), methods)
+	require.Equal(
+		t, metricsProbeMethods, codes[http.StatusMethodNotAllowed],
+		"every invented token should have been unroutable",
+	)
+
+	labels := methodLabels(gatherMetrics(t, reg))
+
+	// Asserted on the count rather than on the set, so that a
+	// regression reports one number instead of dumping every token it
+	// minted.
+	distinct := len(labels)
+
+	assert.Equal(
+		t, 1, distinct,
+		"invented methods must collapse onto one label",
+	)
+	assert.Contains(
+		t, keys(labels), middleware.UnmatchedMethodConst,
+		"that one label must be the unmatched sentinel",
+	)
+
+	// The scrape must not republish the tokens it was driven with
+	// either: a label that merely looks bounded while still echoing
+	// client bytes is the same defect wearing a different name.
+	echoed := 0
+
+	for _, m := range methods {
+		for label := range labels {
+			if strings.Contains(label, m) {
+				echoed++
+			}
+		}
+	}
+
+	assert.Equal(
+		t, 0, echoed,
+		"invented method tokens reached the metrics labels",
+	)
+}
+
+// TestMetrics_MethodSeriesCountIsFlatUnderAFlood reproduces the
+// measurement on the issue in miniature: scrape, drive several
+// hundred distinct method tokens, scrape again, and require the
+// second scrape to be no larger than the first. The first batch
+// establishes every label set the route can produce; a flood five
+// times its size must land on exactly those.
+func TestMetrics_MethodSeriesCountIsFlatUnderAFlood(t *testing.T) {
+	t.Parallel()
+
+	h, reg := metricsTestRouter(t, generousReceiverLimit)
+
+	path := methodProbePath()
+
+	driveMethods(t, h, path, inventedMethods(metricsProbeMethods))
+	seededSeries := seriesCount(gatherMetrics(t, reg))
+	seededLines := scrapeLines(t, reg)
+
+	driveMethods(t, h, path, inventedMethods(metricsProbeMethods*4))
+	floodedSeries := seriesCount(gatherMetrics(t, reg))
+	floodedLines := scrapeLines(t, reg)
+
+	t.Logf(
+		"after %d invented methods: %d series, %d lines; "+
+			"after %d more: %d series, %d lines",
+		metricsProbeMethods, seededSeries, seededLines,
+		metricsProbeMethods*4, floodedSeries, floodedLines,
+	)
+
+	assert.Equal(
+		t, seededSeries, floodedSeries,
+		"a flood of invented methods must not mint series",
+	)
+	assert.Equal(
+		t, seededLines, floodedLines,
+		"a flood of invented methods must not grow the scrape",
+	)
+}
+
+// TestMetrics_RealMethodsStayDistinct is the positive control. The
+// bound is worth nothing if it is bought by flattening the metric:
+// every method the router can route must still carry a series of its
+// own, one sample each, under the route pattern it was sent to.
+func TestMetrics_RealMethodsStayDistinct(t *testing.T) {
+	t.Parallel()
+
+	h, reg := metricsTestRouter(t, generousReceiverLimit)
+
+	methods := realMethods()
+
+	codes := driveMethods(t, h, methodProbePath(), methods)
+	require.Equal(
+		t, len(methods), codes[http.StatusNotFound],
+		"every real method should have reached the receiver",
+	)
+
+	families := gatherMetrics(t, reg)
+
+	want := make(map[string]struct{}, len(methods))
+	for _, m := range methods {
+		want[m] = struct{}{}
+	}
+
+	assert.Equal(
+		t, want, methodLabels(families),
+		"real methods must remain distinguishable",
+	)
+
+	// Appearing somewhere in the scrape is not enough: each method
+	// must own its duration series, holding the one sample it sent.
+	observed := 0
+
+	for _, fam := range families {
+		if !strings.HasSuffix(fam.GetName(), "request_duration_seconds") {
+			continue
+		}
+
+		for _, m := range fam.GetMetric() {
+			observed++
+
+			assert.Equal(
+				t, receiverRoutePattern,
+				labelValue(m, "handler"),
+			)
+			assert.Equal(
+				t, uint64(1),
+				m.GetHistogram().GetSampleCount(),
+				"method %q shares a series",
+				labelValue(m, methodLabel),
+			)
+		}
+	}
+
+	assert.Equal(
+		t, len(methods), observed,
+		"one duration series per routable method",
+	)
+}

internal/middleware/metrics_test.go

@@ -97,20 +97,24 @@ func metricsTestRouter(
 	return r, reg
 }
 
-// drivePaths sends one POST per supplied path and returns how many
-// responses carried each status code.
-func drivePaths(
-	t *testing.T,
-	h http.Handler,
-	paths []string,
-) map[int]int {
+// probe is one request a cardinality assertion sends. Both label
+// dimensions that have leaked are request-controlled — the path and
+// the method — so both vary here and one driver sends them.
+type probe struct {
+	method string
+	path   string
+}
+
+// drive sends every probe and returns how many responses carried each
+// status code.
+func drive(t *testing.T, h http.Handler, probes []probe) map[int]int {
 	t.Helper()
 
 	codes := make(map[int]int)
 
-	for _, p := range paths {
+	for _, p := range probes {
 		req := httptest.NewRequestWithContext(
-			t.Context(), http.MethodPost, p, nil,
+			t.Context(), p.method, p.path, nil,
 		)
 		w := httptest.NewRecorder()
 		h.ServeHTTP(w, req)
@@ -120,6 +124,25 @@ func drivePaths(
 	return codes
 }
 
+// drivePaths sends one POST per supplied path.
+func drivePaths(
+	t *testing.T,
+	h http.Handler,
+	paths []string,
+) map[int]int {
+	t.Helper()
+
+	probes := make([]probe, 0, len(paths))
+
+	for _, p := range paths {
+		probes = append(
+			probes, probe{method: http.MethodPost, path: p},
+		)
+	}
+
+	return drive(t, h, probes)
+}
+
 // receiverPaths returns n distinct /webhook/ paths, each naming a
 // fresh UUID exactly as an unauthenticated flood would.
 func receiverPaths(n int) []string {