Bound the /metrics method label (closes #261)
All checks were successful
check / check (push) Successful in 3m6s
All checks were successful
check / check (push) Successful in 3m6s
The `method` label was recorded as `r.Method` verbatim. net/http
accepts any RFC 9110 token as a method and passes it through, so the
label was bounded at nothing: 300 requests to `/` carrying random
12-character method tokens took a live scrape from 81 lines to 7,606,
unauthenticated and on a route with no rate limiter. This is the same
remote memory-exhaustion vector the handler label carried, reached
through a second dimension.
The fix goes in the same recorder seam that bounds the handler label,
which is renamed to reflect that it now bounds both. A method the
router can route is kept verbatim, so real methods stay
distinguishable; anything else carries the existing `(unmatched)`
sentinel, deliberately the same spelling rather than a second one for
the same idea. The bound is ten values.
The retained set is restated against the net/http constants because
chi's own methodMap is unexported. A token outside it can only ever
produce chi's 405, so folding those together loses nothing a scrape
could have used.
README's Metrics section gains the inbound HTTP metrics, the bound on
each of their labels, and the aggregate
`http_requests_inflight{handler="(all)"}` semantics.
This commit is contained in:
309
internal/middleware/metrics_method_test.go
Normal file
309
internal/middleware/metrics_method_test.go
Normal file
@@ -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",
|
||||
)
|
||||
}
|
||||
Reference in New Issue
Block a user