From 83854e80e0b3f90244f6bd837a5eb40d4849d4de Mon Sep 17 00:00:00 2001 From: Harsha Reddy Date: Fri, 7 Aug 2026 11:23:56 -0400 Subject: [PATCH] fix(telemetry): record route patterns instead of request paths on HTTP metrics (#12557) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Which Problems Are Solved - The `uri` label of `http.server.request_count` and `http.server.return_code_counter` holds the concrete request path, so every object ID served over a REST path becomes its own metric series. On our v4.16.2 instance `/debug/metrics` exposes 8,000 series, 4,467 of which (56%) belong to `http_server_return_code_counter_total` alone, spread over 4,390 distinct `uri` values such as `/v2/sessions/385063742120926058`. The set only grows with the number of objects the installation has served. - Paths matching no route are recorded verbatim, so unauthenticated scanner traffic (`/wp-json/`, `/login.php`, …) keeps adding series. - The grpc-gateway answers `405 Method Not Allowed` without going through the configured `errorHandler`, so those responses never reported a route pattern, not even before the regression. This is a regression, first shipped in v4.11.0; v4.10.1 is unaffected. #9286 introduced the mechanism, #9523 extended it to unknown paths, and #11435 removed it while reorganising the middleware packages: > Removed setting of URI to context in metric middleware. There were only setters and no getters. (Unused value) The getter was `*recorder.RequestURI` in `RegisterRequestCounter` / `RegisterRequestCodeCounter`, which the same PR replaced with `baseURI(r)`. Both ends went at once, so nothing failed. # How the Problems Are Solved - `metrics.WithRequestURIPattern` / `metrics.SetRequestURIPattern` are back, and `metrics.RequestURI` is now the single place deciding what to label with: the pattern a router reported, or the requested path if none did. `UnknownPath` moved to the metrics package so every surface shares one constant. - The HTTP metrics middleware prepares the context before passing the request on, which is the half that got dropped. - `setRequestURIPattern` in the grpc-gateway reports the pattern to metrics again, not only to tracing, and is now also called on the `405` branch. - Requests routed by chi (the OIDC endpoints) fall back to the pattern chi matched. That covers the RFC 7592 client configuration routes added in #12315 (`/oauth/v2/register/{client_id}`), which are templated on the client ID, and collapses unknown paths below the OIDC prefixes to `UNKNOWN_PATH` as well. Against the metrics dump from our instance, `http_server_return_code_counter_total` drops from 4,467 series to roughly 35, and the endpoint as a whole from 8,000 to about 3,570. # Additional Changes - Regression tests, which were missing and are the reason the revert went unnoticed: - `metrics`: resolution of the `uri` label, including that a pattern set on a derived context reaches the middleware. - `gateway`: every way the gateway can answer — success, error, unroutable path, wrong method. - `middleware`: both routing styles, i.e. a router reporting its own pattern and a chi routed request. - A doc comment on `SetRequestURIPattern` explaining that it is written by routers and read back by the middleware through the context, so it does not read as an unused setter again. # Additional Context - Closes #12556 - Restores #9286 and #9523 - Regressed in #11435 Co-authored-by: Cursor --- .../instrumentation/metrics/http_handler.go | 49 ++++++- .../metrics/http_handler_test.go | 91 +++++++++++++ internal/api/grpc/server/gateway.go | 10 +- internal/api/grpc/server/gateway_test.go | 68 ++++++++++ .../http/middleware/metrics_interceptor.go | 26 ++++ .../middleware/metrics_interceptor_test.go | 125 ++++++++++++++++++ 6 files changed, 362 insertions(+), 7 deletions(-) create mode 100644 backend/v3/instrumentation/metrics/http_handler_test.go create mode 100644 internal/api/http/middleware/metrics_interceptor_test.go diff --git a/backend/v3/instrumentation/metrics/http_handler.go b/backend/v3/instrumentation/metrics/http_handler.go index fa0cac642..a66daeb8d 100644 --- a/backend/v3/instrumentation/metrics/http_handler.go +++ b/backend/v3/instrumentation/metrics/http_handler.go @@ -1,6 +1,7 @@ package metrics import ( + "context" "net/http" "strings" @@ -17,6 +18,11 @@ const ( Method = "method" URI = "uri" ReturnCode = "return_code" + + // UnknownPath is recorded as uri for requests that did not match any route. + // Recording the requested path instead would allow anyone to create an unbounded + // number of metric series by calling arbitrary paths. + UnknownPath = "UNKNOWN_PATH" ) type MetricType int32 @@ -31,9 +37,44 @@ type StatusRecorder interface { Status() int } +type requestURIKey struct{} + +// WithRequestURIPattern returns a context that carries the uri label of the HTTP metrics +// recorded for the current request. The metrics middleware must call this before passing +// the request on, so that the router serving it can substitute its route pattern through +// [SetRequestURIPattern]. +func WithRequestURIPattern(ctx context.Context) context.Context { + var pattern string + return context.WithValue(ctx, requestURIKey{}, &pattern) +} + +// SetRequestURIPattern records pattern as the uri label of the current request's HTTP +// metrics, e.g. "/v2/sessions/{session_id}" instead of "/v2/sessions/385063742120926058". +// +// Routers that serve templated paths must call this for every response they produce, +// including errors. Without it every resource ID gets its own metric series and the +// number of series only grows with the number of objects in the installation. +func SetRequestURIPattern(ctx context.Context, pattern string) { + uri, ok := ctx.Value(requestURIKey{}).(*string) + if !ok { + return + } + *uri = pattern +} + +// RequestURI returns the uri label to record for r: the route pattern reported by the +// router through [SetRequestURIPattern], or the requested path if the router did not +// report one. +func RequestURI(r *http.Request) string { + if pattern, ok := r.Context().Value(requestURIKey{}).(*string); ok && *pattern != "" { + return *pattern + } + return strings.Split(r.RequestURI, "?")[0] +} + func RegisterRequestCounter(recorder StatusRecorder, r *http.Request) { var labels = map[string]attribute.Value{ - URI: attribute.StringValue(baseURI(r)), + URI: attribute.StringValue(RequestURI(r)), Method: attribute.StringValue(r.Method), } RegisterCounter(RequestCounter, RequestCountDescription) @@ -47,14 +88,10 @@ func RegisterTotalRequestCounter(r *http.Request) { func RegisterRequestCodeCounter(recorder StatusRecorder, r *http.Request) { var labels = map[string]attribute.Value{ - URI: attribute.StringValue(baseURI(r)), + URI: attribute.StringValue(RequestURI(r)), Method: attribute.StringValue(r.Method), ReturnCode: attribute.IntValue(recorder.Status()), } RegisterCounter(ReturnCodeCounter, ReturnCodeCounterDescription) AddCount(r.Context(), ReturnCodeCounter, 1, labels) } - -func baseURI(r *http.Request) string { - return strings.Split(r.RequestURI, "?")[0] -} diff --git a/backend/v3/instrumentation/metrics/http_handler_test.go b/backend/v3/instrumentation/metrics/http_handler_test.go new file mode 100644 index 000000000..8f6eed79b --- /dev/null +++ b/backend/v3/instrumentation/metrics/http_handler_test.go @@ -0,0 +1,91 @@ +package metrics + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRequestURI(t *testing.T) { + tests := []struct { + name string + requestURI string + // prepare returns the context the request is served with. + prepare func(context.Context) context.Context + want string + }{ + { + name: "no pattern support, path is recorded", + requestURI: "/oauth/v2/token", + prepare: func(ctx context.Context) context.Context { return ctx }, + want: "/oauth/v2/token", + }, + { + name: "no pattern support, query is stripped", + requestURI: "/oauth/v2/authorize?client_id=1234&state=abcd", + prepare: func(ctx context.Context) context.Context { return ctx }, + want: "/oauth/v2/authorize", + }, + { + name: "pattern supported but not set, path is recorded", + requestURI: "/oauth/v2/token", + prepare: WithRequestURIPattern, + want: "/oauth/v2/token", + }, + { + name: "pattern set, pattern is recorded instead of the object id", + requestURI: "/v2/sessions/385063742120926058", + prepare: func(ctx context.Context) context.Context { + ctx = WithRequestURIPattern(ctx) + SetRequestURIPattern(ctx, "/v2/sessions/{session_id}") + return ctx + }, + want: "/v2/sessions/{session_id}", + }, + { + name: "unknown path, no path is recorded", + requestURI: "/junk", + prepare: func(ctx context.Context) context.Context { + ctx = WithRequestURIPattern(ctx) + SetRequestURIPattern(ctx, UnknownPath) + return ctx + }, + want: UnknownPath, + }, + { + name: "pattern set without context support, path is recorded", + requestURI: "/v2/sessions/385063742120926058", + prepare: func(ctx context.Context) context.Context { + SetRequestURIPattern(ctx, "/v2/sessions/{session_id}") + return ctx + }, + want: "/v2/sessions/385063742120926058", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, tt.requestURI, nil) + r = r.WithContext(tt.prepare(r.Context())) + + assert.Equal(t, tt.want, RequestURI(r)) + }) + } +} + +// TestSetRequestURIPattern_derivedContext ensures the pattern set by a handler is visible +// to the middleware that seeded the context, even though the handler received a derived +// context. Routers such as the grpc-gateway only ever report the pattern on their own +// context, so a value copied by [context.WithValue] would not reach the caller. +func TestSetRequestURIPattern_derivedContext(t *testing.T) { + r := httptest.NewRequest(http.MethodDelete, "/v2/sessions/385063742120926058", nil) + r = r.WithContext(WithRequestURIPattern(r.Context())) + + type unrelatedKey struct{} + derived := context.WithValue(r.Context(), unrelatedKey{}, "value") + SetRequestURIPattern(derived, "/v2/sessions/{session_id}") + + assert.Equal(t, "/v2/sessions/{session_id}", RequestURI(r)) +} diff --git a/internal/api/grpc/server/gateway.go b/internal/api/grpc/server/gateway.go index b15ec6791..4e24be8a5 100644 --- a/internal/api/grpc/server/gateway.go +++ b/internal/api/grpc/server/gateway.go @@ -28,7 +28,7 @@ import ( const ( mimeWildcard = "*/*" - UnknownPath = "UNKNOWN_PATH" + UnknownPath = metrics.UnknownPath ) var ( @@ -44,9 +44,13 @@ var ( httpErrorHandler = runtime.RoutingErrorHandlerFunc( func(ctx context.Context, mux *runtime.ServeMux, marshaler runtime.Marshaler, w http.ResponseWriter, r *http.Request, httpStatus int) { if httpStatus != http.StatusMethodNotAllowed { + // Every other routing error is passed on to the configured errorHandler. runtime.DefaultRoutingErrorHandler(ctx, mux, marshaler, w, r, httpStatus) return } + // This branch answers without going through the configured errorHandler, + // so the URI pattern has to be set here as well. + setRequestURIPattern(ctx) // Use HTTPStatusError to customize the DefaultHTTPErrorHandler status code err := &runtime.HTTPStatusError{ @@ -276,6 +280,9 @@ func grpcCredentials(tlsConfig *tls.Config) credentials.TransportCredentials { return creds } +// setRequestURIPattern reports the route pattern that served the request, e.g. +// "/v2/sessions/{session_id}", to tracing and metrics. Both would otherwise fall back to +// the requested path, which embeds resource IDs and therefore grows without bound. func setRequestURIPattern(ctx context.Context) { pattern, ok := runtime.HTTPPathPattern(ctx) if !ok { @@ -287,4 +294,5 @@ func setRequestURIPattern(ctx context.Context) { } span := trace.SpanFromContext(ctx) span.SetName(pattern) + metrics.SetRequestURIPattern(ctx, pattern) } diff --git a/internal/api/grpc/server/gateway_test.go b/internal/api/grpc/server/gateway_test.go index cda52c9ff..36e8fb41c 100644 --- a/internal/api/grpc/server/gateway_test.go +++ b/internal/api/grpc/server/gateway_test.go @@ -8,8 +8,11 @@ import ( "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + + "github.com/zitadel/zitadel/backend/v3/instrumentation/metrics" ) func TestErrorHandler_PreservesUnauthenticatedStatus(t *testing.T) { @@ -22,3 +25,68 @@ func TestErrorHandler_PreservesUnauthenticatedStatus(t *testing.T) { assert.Contains(t, resp.Body.String(), `"code":16`) assert.Contains(t, resp.Body.String(), `"message":"auth header missing"`) } + +// TestGatewayRecordsURIPattern guards the uri label of the HTTP metrics against the object +// IDs in the gateway's paths. A single instance serves an unbounded number of sessions, +// auth requests and users, so recording the requested path produces one metric series per +// object. Every way the gateway can answer a request has to report its route pattern. +func TestGatewayRecordsURIPattern(t *testing.T) { + const ( + pattern = "/v2/sessions/{session_id}" + requestURI = "/v2/sessions/385063742120926058" + ) + tests := []struct { + name string + // respond answers the request the way the gateway would in the tested case. + respond func(ctx context.Context, mux *runtime.ServeMux, w http.ResponseWriter, r *http.Request) + // routed tells whether the request reached a service method. + routed bool + want string + }{ + { + name: "successful response", + respond: func(ctx context.Context, _ *runtime.ServeMux, w http.ResponseWriter, _ *http.Request) { + require.NoError(t, responseForwarder(ctx, w, nil)) + }, + routed: true, + want: pattern, + }, + { + name: "error response", + respond: func(ctx context.Context, mux *runtime.ServeMux, w http.ResponseWriter, r *http.Request) { + errorHandler(ctx, mux, jsonMarshaler, w, r, status.Error(codes.NotFound, "session not found")) + }, + routed: true, + want: pattern, + }, + { + name: "unroutable path", + respond: func(ctx context.Context, mux *runtime.ServeMux, w http.ResponseWriter, r *http.Request) { + httpErrorHandler(ctx, mux, jsonMarshaler, w, r, http.StatusNotFound) + }, + want: metrics.UnknownPath, + }, + { + name: "method not allowed", + respond: func(ctx context.Context, mux *runtime.ServeMux, w http.ResponseWriter, r *http.Request) { + httpErrorHandler(ctx, mux, jsonMarshaler, w, r, http.StatusMethodNotAllowed) + }, + want: metrics.UnknownPath, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // The metrics middleware seeds the request before handing it to the gateway. + req := httptest.NewRequest(http.MethodGet, requestURI, nil) + req = req.WithContext(metrics.WithRequestURIPattern(req.Context())) + + ctx := req.Context() + if tt.routed { + ctx = runtime.WithHTTPPathPattern(pattern)(ctx) + } + tt.respond(ctx, runtime.NewServeMux(serveMuxOptions(nil)...), httptest.NewRecorder(), req) + + assert.Equal(t, tt.want, metrics.RequestURI(req)) + }) + } +} diff --git a/internal/api/http/middleware/metrics_interceptor.go b/internal/api/http/middleware/metrics_interceptor.go index d75994806..e9bc8a052 100644 --- a/internal/api/http/middleware/metrics_interceptor.go +++ b/internal/api/http/middleware/metrics_interceptor.go @@ -3,6 +3,7 @@ package middleware import ( "net/http" + "github.com/go-chi/chi/v5" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "github.com/zitadel/zitadel/backend/v3/instrumentation" @@ -37,7 +38,14 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } recorder := newStatusWriter(w) + // Requested paths contain object IDs and are therefore unbounded, so the router below + // gets the opportunity to report the route pattern it served the request with instead. + ctx := metrics.WithRequestURIPattern(r.Context()) + r = r.WithContext(ctx) h.handler.ServeHTTP(recorder, r) + if pattern := chiRoutePattern(r); pattern != "" { + metrics.SetRequestURIPattern(ctx, pattern) + } if h.containsMetricsMethod(metrics.MetricTypeRequestCount) { metrics.RegisterRequestCounter(recorder, r) } @@ -49,6 +57,24 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } +// chiRoutePattern returns the route pattern chi matched the request against, e.g. +// "/oauth/v2/register/{client_id}", or [metrics.UnknownPath] if chi routed the request but +// found no route for it. Routers that are not chi based report their pattern themselves +// through [metrics.SetRequestURIPattern]; for those an empty string is returned. +// +// chi only knows the pattern once it routed the request, so this must be called after the +// wrapped handler returned. +func chiRoutePattern(r *http.Request) string { + routeCtx := chi.RouteContext(r.Context()) + if routeCtx == nil { + return "" + } + if pattern := routeCtx.RoutePattern(); pattern != "" { + return pattern + } + return metrics.UnknownPath +} + func (h *Handler) containsMetricsMethod(method metrics.MetricType) bool { for _, m := range h.methods { if m == method { diff --git a/internal/api/http/middleware/metrics_interceptor_test.go b/internal/api/http/middleware/metrics_interceptor_test.go new file mode 100644 index 000000000..dc62dbc66 --- /dev/null +++ b/internal/api/http/middleware/metrics_interceptor_test.go @@ -0,0 +1,125 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/zitadel/zitadel/backend/v3/instrumentation/metrics" +) + +// TestMetricsHandler_reportedPattern covers routers that report their route pattern +// themselves, such as the grpc-gateway. Without the context prepared by the handler their +// report is lost and the requested path is recorded, which adds a metric series per object. +func TestMetricsHandler_reportedPattern(t *testing.T) { + tests := []struct { + name string + pattern string + want string + }{ + { + name: "router reports no pattern, path is recorded", + want: "/v2/sessions/385063742120926058", + }, + { + name: "router reports a pattern, pattern is recorded", + pattern: "/v2/sessions/{session_id}", + want: "/v2/sessions/{session_id}", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var served *http.Request + handler := MetricsHandler([]metrics.MetricType{metrics.MetricTypeStatusCode})( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + served = r + if tt.pattern != "" { + metrics.SetRequestURIPattern(r.Context(), tt.pattern) + } + w.WriteHeader(http.StatusNoContent) + }), + ) + + handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodDelete, "/v2/sessions/385063742120926058", nil)) + + require.NotNil(t, served) + assert.Equal(t, tt.want, metrics.RequestURI(served)) + }) + } +} + +// TestMetricsHandler_chiRoutePattern covers the routers that leave the pattern to chi, such +// as the OIDC endpoints. Their client configuration endpoints are templated on the client +// ID and any unknown path below their prefixes ends up here as well. +func TestMetricsHandler_chiRoutePattern(t *testing.T) { + tests := []struct { + name string + method string + requestURI string + want string + }{ + { + name: "static endpoint", + method: http.MethodPost, + requestURI: "/oauth/v2/token", + want: "/oauth/v2/token", + }, + { + name: "static endpoint with query", + method: http.MethodGet, + requestURI: "/oauth/v2/authorize?client_id=1234&state=abcd", + want: "/oauth/v2/authorize", + }, + { + name: "client configuration endpoint", + method: http.MethodGet, + requestURI: "/oauth/v2/register/307534931347210241", + want: "/oauth/v2/register/{client_id}", + }, + { + name: "unknown path", + method: http.MethodGet, + requestURI: "/oauth/v2/junk", + want: metrics.UnknownPath, + }, + { + name: "known path, unsupported method", + method: http.MethodDelete, + requestURI: "/oauth/v2/token", + want: metrics.UnknownPath, + }, + } + + // served is the request as the routed handler saw it. Reading the recorded URI from it + // after the router returned gives the value the metrics handler recorded, because the + // two share the context the metrics handler prepared. + var served *http.Request + respond := func(w http.ResponseWriter, r *http.Request) { + served = r + w.WriteHeader(http.StatusNoContent) + } + + // Mirrors how the OIDC server nests its routes in the metrics handler. + router := chi.NewRouter() + router.Use(MetricsHandler([]metrics.MetricType{metrics.MetricTypeStatusCode})) + router.Post("/oauth/v2/token", respond) + router.Get("/oauth/v2/authorize", respond) + router.Get("/oauth/v2/register/{client_id}", respond) + router.NotFound(respond) + router.MethodNotAllowed(respond) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + served = nil + + router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(tt.method, tt.requestURI, nil)) + + require.NotNil(t, served) + assert.Equal(t, tt.want, metrics.RequestURI(served)) + }) + } +}