Label request metrics by route, not by path (closes #254)
check / check (push) Waiting to run
check / check (push) Waiting to run
The metrics middleware labelled each request with its path, so every new path added series to the registry that were never removed, and /metrics listed the token of every encrypted URL requested. The handler label is now the pattern of the chi route the request will match, looked up before routing, or "unmatched" when none does. /v1 and /v1/ ask the image subrouter, and an empty path is looked up as /, as chi routes it. TestMetricsRecordsServedRequest expects "unmatched" in place of "/test", as the owner approved. Model: opus-5-5
This commit was merged in pull request #260.
This commit is contained in:
@@ -3,12 +3,15 @@ package middleware
|
||||
import (
|
||||
"bytes"
|
||||
"log/slog"
|
||||
"maps"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/prometheus/client_golang/prometheus"
|
||||
"github.com/prometheus/client_golang/prometheus/promhttp"
|
||||
|
||||
"sneak.berlin/go/pixa/internal/config"
|
||||
@@ -198,7 +201,7 @@ func TestMetricsRecordsServedRequest(t *testing.T) {
|
||||
|
||||
// The line /metrics shows once one GET /test has been served.
|
||||
const want = `http_request_duration_seconds_count{` +
|
||||
`code="200",handler="/test",method="GET",service=""} 1`
|
||||
`code="200",handler="unmatched",method="GET",service=""} 1`
|
||||
|
||||
mw := &Middleware{log: slog.Default(), config: &config.Config{}}
|
||||
|
||||
@@ -220,6 +223,97 @@ func TestMetricsRecordsServedRequest(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestMetricsLabelsRequestsByRoute checks that the metrics middleware labels
|
||||
// each request with the pattern of the route that answers it, never with its
|
||||
// path: two paths one route answers share that route's pattern, and requests
|
||||
// no route answers, a 404 or a 405, share "unmatched", among them requests for
|
||||
// the path the subrouter is mounted at, /v1 and /v1/. A request with an empty
|
||||
// path is answered by the / route, as chi routes it as /, and is labelled /.
|
||||
// With a label per path, any client could add series to the registry without
|
||||
// limit. The middleware registers with a registry of its own here, as another
|
||||
// test in this package registers with Prometheus' default registry, which can
|
||||
// be done only once.
|
||||
func TestMetricsLabelsRequestsByRoute(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
registry := prometheus.NewRegistry()
|
||||
mw := &Middleware{
|
||||
log: slog.Default(),
|
||||
config: &config.Config{},
|
||||
metricsRegistry: registry,
|
||||
}
|
||||
|
||||
// The image route is in a subrouter, as in pixa's router.
|
||||
router := chi.NewRouter()
|
||||
router.Use(mw.Metrics())
|
||||
router.Get("/", func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.WriteHeader(http.StatusOK)
|
||||
})
|
||||
router.Route("/v1", func(r chi.Router) {
|
||||
r.Get("/image/*", func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.WriteHeader(http.StatusOK)
|
||||
})
|
||||
})
|
||||
|
||||
for _, req := range []struct{ method, path string }{
|
||||
{http.MethodGet, "/v1/image/a.example.com/one.jpg/1x1.png"},
|
||||
{http.MethodGet, "/v1/image/b.example.com/two.jpg/1x1.png"},
|
||||
{http.MethodGet, "/no-such-route"},
|
||||
{http.MethodGet, "/v1/no-such-route"},
|
||||
{http.MethodGet, "/v1"},
|
||||
{http.MethodGet, "/v1/"},
|
||||
{http.MethodPost, "/v1/image/a.example.com/one.jpg/1x1.png"},
|
||||
// The request line GET http://pixa.example HTTP/1.1 leaves both
|
||||
// r.URL.Path and r.URL.RawPath empty.
|
||||
{http.MethodGet, "http://pixa.example"},
|
||||
} {
|
||||
r := httptest.NewRequestWithContext(
|
||||
t.Context(), req.method, req.path, nil)
|
||||
t.Logf("%s %s: r.URL.Path %q, r.URL.RawPath %q",
|
||||
req.method, req.path, r.URL.Path, r.URL.RawPath)
|
||||
router.ServeHTTP(httptest.NewRecorder(), r)
|
||||
}
|
||||
|
||||
got := requestCountsByHandler(t, registry)
|
||||
t.Logf("requests counted by handler label: %v", got)
|
||||
|
||||
want := map[string]uint64{"/": 1, "/v1/image/*": 2, "unmatched": 5}
|
||||
if !maps.Equal(got, want) {
|
||||
t.Errorf("requests counted by handler label = %v, want %v", got, want)
|
||||
}
|
||||
}
|
||||
|
||||
// requestCountsByHandler returns how many requests the request duration
|
||||
// histogram in registry counted under each value of its handler label.
|
||||
func requestCountsByHandler(
|
||||
t *testing.T, registry *prometheus.Registry,
|
||||
) map[string]uint64 {
|
||||
t.Helper()
|
||||
|
||||
families, err := registry.Gather()
|
||||
if err != nil {
|
||||
t.Fatalf("registry.Gather() error = %v", err)
|
||||
}
|
||||
|
||||
counts := map[string]uint64{}
|
||||
|
||||
for _, family := range families {
|
||||
if family.GetName() != "http_request_duration_seconds" {
|
||||
continue
|
||||
}
|
||||
|
||||
for _, metric := range family.GetMetric() {
|
||||
for _, label := range metric.GetLabel() {
|
||||
if label.GetName() == "handler" {
|
||||
counts[label.GetValue()] += metric.GetHistogram().GetSampleCount()
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return counts
|
||||
}
|
||||
|
||||
// TestLoggingLeavesOutSubmittedSigningKey checks that a login, a POST /
|
||||
// whose form carries the signing key, leaves no trace of the key in the
|
||||
// request's log line.
|
||||
|
||||
Reference in New Issue
Block a user