mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-20 14:34:25 +00:00
83854e80e0
# 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]>
93 lines
3.2 KiB
Go
93 lines
3.2 KiB
Go
package server
|
|
|
|
import (
|
|
"context"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"testing"
|
|
|
|
"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) {
|
|
req := httptest.NewRequest(http.MethodGet, "/auth/v1/users/me", nil)
|
|
resp := httptest.NewRecorder()
|
|
|
|
errorHandler(context.Background(), runtime.NewServeMux(), jsonMarshaler, resp, req, status.Error(codes.Unauthenticated, "auth header missing"))
|
|
|
|
assert.Equal(t, http.StatusUnauthorized, resp.Code)
|
|
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))
|
|
})
|
|
}
|
|
}
|