mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
fix(telemetry): record route patterns instead of request paths on HTTP metrics (#12557)
# 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 <[email protected]>
This commit is contained in:
@@ -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]
|
||||
}
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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))
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user