From 8c2dda5db683bf32062d9916a3523d0dcad4bf56 Mon Sep 17 00:00:00 2001 From: Arpit Jain Date: Tue, 29 Sep 2026 01:19:42 -0400 Subject: [PATCH] fix: bound the http.route metric label on the evaluation service Signed-off-by: Arpit Jain --- .../flag-evaluation/connect_service.go | 9 ++-- .../middleware/metrics/http_metrics.go | 23 +++++++++- .../middleware/metrics/http_metrics_test.go | 46 +++++++++++++++++++ 3 files changed, 74 insertions(+), 4 deletions(-) diff --git a/flagd/pkg/service/flag-evaluation/connect_service.go b/flagd/pkg/service/flag-evaluation/connect_service.go index 6e49ae570..c12c5256c 100644 --- a/flagd/pkg/service/flag-evaluation/connect_service.go +++ b/flagd/pkg/service/flag-evaluation/connect_service.go @@ -148,7 +148,7 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene protojson.UnmarshalOptions{DiscardUnknown: true}, ) - _, oldHandler := schemaConnectV1.NewServiceHandler(fes, append(svcConf.Options, marshalOpts)...) + oldPath, oldHandler := schemaConnectV1.NewServiceHandler(fes, append(svcConf.Options, marshalOpts)...) // register handler for new flag evaluation schema (v1) @@ -161,7 +161,7 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene svcConf.StreamDeadline, ) - _, v1Handler := evaluationV1.NewServiceHandler(v1Fes, append(svcConf.Options, marshalOpts)...) + v1Path, v1Handler := evaluationV1.NewServiceHandler(v1Fes, append(svcConf.Options, marshalOpts)...) // register handler for evaluation v2 schema (with optional value and variant) @@ -174,7 +174,7 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene svcConf.StreamDeadline, ) - _, v2Handler := evaluationV2.NewServiceHandler(v2Fes, append(svcConf.Options, marshalOpts)...) + v2Path, v2Handler := evaluationV2.NewServiceHandler(v2Fes, append(svcConf.Options, marshalOpts)...) bs := bufSwitchHandler{ old: oldHandler, @@ -203,6 +203,9 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene MetricRecorder: s.metrics, Logger: s.logger, HandlerID: "", + // Without this the http.route label is whatever path the caller sent, which is + // unbounded. flag-sync and ofrep pin their handler IDs for the same reason. + RoutePrefixes: []string{oldPath, v1Path, v2Path, "/healthz", "/readyz", "/metrics"}, }) s.AddMiddleware(metricsMiddleware) diff --git a/flagd/pkg/service/middleware/metrics/http_metrics.go b/flagd/pkg/service/middleware/metrics/http_metrics.go index cfc05b39e..2107a1e0a 100644 --- a/flagd/pkg/service/middleware/metrics/http_metrics.go +++ b/flagd/pkg/service/middleware/metrics/http_metrics.go @@ -9,6 +9,7 @@ import ( "net" "net/http" "strconv" + "strings" "time" "github.com/open-feature/flagd/core/pkg/logger" @@ -22,8 +23,15 @@ type Config struct { GroupedStatus bool DisableMeasureSize bool HandlerID string + // RoutePrefixes are the paths this server actually serves. When HandlerID is empty, a + // request whose path is not one of these is recorded as "other" rather than as itself, + // so a caller cannot mint a new metric series per request path. + RoutePrefixes []string } +// otherRoute is the handler ID used for requests that match none of the served routes. +const otherRoute = "other" + type Middleware struct { cfg Config } @@ -45,6 +53,19 @@ func (cfg *Config) defaults() { } } +// handlerID returns the configured ID, or the request path when the server serves it. +func (m Middleware) handlerID(path string) string { + if m.cfg.HandlerID != "" { + return m.cfg.HandlerID + } + for _, prefix := range m.cfg.RoutePrefixes { + if strings.HasPrefix(path, prefix) { + return path + } + } + return otherRoute +} + func (m Middleware) Measure(ctx context.Context, handlerID string, reporter Reporter, next func()) { // If there isn't predefined handler ID we // set that ID as the URL path. @@ -102,7 +123,7 @@ func (m Middleware) Handler(h http.Handler) http.Handler { w: wi, r: r, } - m.Measure(r.Context(), m.cfg.HandlerID, reporter, func() { + m.Measure(r.Context(), m.handlerID(r.URL.Path), reporter, func() { h.ServeHTTP(wi, r) }) }) diff --git a/flagd/pkg/service/middleware/metrics/http_metrics_test.go b/flagd/pkg/service/middleware/metrics/http_metrics_test.go index 35d5c3dec..3fc73d529 100644 --- a/flagd/pkg/service/middleware/metrics/http_metrics_test.go +++ b/flagd/pkg/service/middleware/metrics/http_metrics_test.go @@ -231,3 +231,49 @@ func TestNewHttpMetric(t *testing.T) { t.Errorf("Expected DisableMeasureSize to be configured with %v, got %v", disableMeasureSize, mdw.cfg.DisableMeasureSize) } } + +func TestHandlerID(t *testing.T) { + tests := []struct { + name string + cfg Config + path string + expected string + }{ + { + name: "configured handler ID wins", + cfg: Config{HandlerID: "/ofrep/v1/evaluate/flags/{key}", RoutePrefixes: []string{"/flagd.evaluation.v1.Service/"}}, + path: "/anything", + expected: "/ofrep/v1/evaluate/flags/{key}", + }, + { + name: "served route keeps its path", + cfg: Config{RoutePrefixes: []string{"/flagd.evaluation.v1.Service/"}}, + path: "/flagd.evaluation.v1.Service/ResolveBoolean", + expected: "/flagd.evaluation.v1.Service/ResolveBoolean", + }, + { + name: "unserved route is folded", + cfg: Config{RoutePrefixes: []string{"/flagd.evaluation.v1.Service/"}}, + path: "/not-a-route-12345", + expected: otherRoute, + }, + { + name: "no prefixes folds everything", + cfg: Config{}, + path: "/flagd.evaluation.v1.Service/ResolveBoolean", + expected: otherRoute, + }, + } + + log := logger.NewLogger(nil, false) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.cfg.Logger = log + mdw := NewHTTPMetric(tt.cfg) + if got := mdw.handlerID(tt.path); got != tt.expected { + t.Errorf("Expected %q, got %q", tt.expected, got) + } + }) + } +}