From 025063deecbbb204158a3595c7aa447f0759f8d8 Mon Sep 17 00:00:00 2001 From: Pascal Bleser Date: Fri, 11 Sep 2026 19:21:05 +0200 Subject: [PATCH] feat(proxy): add the service to the proxy metrics Improve the metrics for the proxy service. * extend the instrumenter middleware to add a label with the name of the service the request is being dispatched to * add a native Prometheus histogram that tracks the durations and also includes a label for the name of the service the request is being dispatched to, as well as a 'result' label ('success', 'client-error', 'server-error') based on the HTTP status code of the response * add per-service gauge functions to count the number of in-flight requests * add a counter for routing failures, for when an inbound request cannot be mapped to a route * extend the route.RoutingInfo struct with a service attribute, and a Service() getter * remove the 'routing_failure_count' metric, as it is redundant --- pkg/metrics/metrics.go | 44 ++-- services/graph/pkg/metrics/metrics.go | 4 +- services/proxy/README.md | 28 ++- services/proxy/pkg/command/server.go | 23 ++- services/proxy/pkg/metrics/metrics.go | 191 +++++++++++++++--- services/proxy/pkg/middleware/metrics.go | 10 +- .../proxy/pkg/proxy/proxy_integration_test.go | 2 +- services/proxy/pkg/router/router.go | 10 +- 8 files changed, 247 insertions(+), 65 deletions(-) diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index e47d8280aa..951416c128 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -1,6 +1,7 @@ package metrics import ( + "errors" "fmt" "reflect" "strings" @@ -48,6 +49,21 @@ func describe(metric prometheus.Collector, initialize func() error) (string, err return fams[0].GetName(), nil } +func postProcess(c prometheus.Collector) error { + // special post-treatment for the BuildInfo metric, as we have that one pretty much + // everywhere: set its value with the current version so we don't need to do that every time + switch buildInfo := c.(type) { + case BuildInfoMetric: + if name, err := describe(buildInfo, func() error { buildInfo.WithLabelValues("0").Set(0.0); return nil }); err != nil { + return err + } else if strings.HasSuffix(name, "_build_info") { + buildInfo.Reset() + buildInfo.WithLabelValues(version.GetString()).Set(1) + } + } + return nil +} + // Take a struct that contains metrics as attributes and register all of them // with the specified Registerer. func RegisterAll(registerer prometheus.Registerer, m any, logger *log.Logger) error { @@ -85,17 +101,8 @@ func RegisterAll(registerer prometheus.Registerer, m any, logger *log.Logger) er } } else { succeeded = append(succeeded, n) - - // special post-treatment for the BuildInfo metric, as we have that one pretty much - // everywhere: set its value with the current version so we don't need to do that every time - switch buildInfo := c.(type) { - case BuildInfoMetric: - if name, err := describe(buildInfo, func() error { buildInfo.WithLabelValues("0").Set(0.0); return nil }); err != nil { - failed[n] = err - } else if strings.HasSuffix(name, "_build_info") { - buildInfo.Reset() - buildInfo.WithLabelValues(version.GetString()).Set(1) - } + if err := postProcess(c); err != nil { + failed[n] = err } } case *prometheus.Desc, @@ -135,9 +142,18 @@ func Register[M any](reg prometheus.Registerer, m M, logger *log.Logger) (M, err return m, err } -// Register a single metric. -func RegisterMetric[M prometheus.Collector](reg prometheus.Registerer, m M, logger *log.Logger) error { - return NewLoggingPrometheusRegisterer(reg, logger).Register(m) +// Register individual metrics. +func RegisterMetrics(reg prometheus.Registerer, logger *log.Logger, metrics ...prometheus.Collector) error { + lreg := NewLoggingPrometheusRegisterer(reg, logger) + errs := []error{} + for _, c := range metrics { + if err := lreg.Register(c); err != nil { + errs = append(errs, err) + } else { + errs = append(errs, postProcess(c)) + } + } + return errors.Join(errs...) } // Prometheus Registerer wrapper that logs every error that occurs when registering diff --git a/services/graph/pkg/metrics/metrics.go b/services/graph/pkg/metrics/metrics.go index 18358e1937..ac113f176f 100644 --- a/services/graph/pkg/metrics/metrics.go +++ b/services/graph/pkg/metrics/metrics.go @@ -118,7 +118,9 @@ func New(registerer prometheus.Registerer, logger *log.Logger, httpPathSplitter m, err := ocmetrics.Register(registerer, m, logger) // must additionally register unexported metrics: - err = errors.Join(err, ocmetrics.RegisterMetric(registerer, m.httpRequestDuration, logger)) + err = errors.Join(err, ocmetrics.RegisterMetrics(registerer, logger, + m.httpRequestDuration, + )) return m, err } diff --git a/services/proxy/README.md b/services/proxy/README.md index 16030f7530..d87b290212 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -300,12 +300,14 @@ In this mode, the proxy service only exposes its own metrics. The metrics of the ### Available Metrics The following metrics are exposed by the proxy service: -| Metric Name | Description | Labels | -|----------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------| -| `opencloud_proxy_requests_total` | [Counter](https://prometheus.io/docs/tutorials/understanding_metric_types/#counter) metric which reports the total number of HTTP requests. | `method`: HTTP method of the request | -| `opencloud_proxy_errors_total` | [Counter](https://prometheus.io/docs/tutorials/understanding_metric_types/#counter) metric which reports the total number of HTTP requests which have failed. That counts all response codes >= 500 | `method`: HTTP method of the request | -| `opencloud_proxy_duration_seconds` | [Histogram](https://prometheus.io/docs/tutorials/understanding_metric_types/#histogram) of the time (in seconds) each request took. A histogram metric uses buckets to count the number of events that fall into each bucket. | `method`: HTTP method of the request | -| `opencloud_proxy_build_info{version}` | A metric with a constant `1` value labeled by version, exposing the version of the OpenCloud proxy service. | `version`: Build version of the proxy | +| Name | Labels | Description | +| ---- | ------ | ----------- | +| `opencloud_proxy_concurrent_service_requests{service}` | `service`: identifier of the service the request is proxied to | Counts the number of in-flight requests that are being processed at a given time | +| `opencloud_proxy_routing_failure_count` | | Counts the number of inbound requests that cannot be proxied due to a failure of determining how to route it | +| `opencloud_proxy_duration_seconds{service}` | `service`: identifier of the service the request is proxied to | Classic histogram that measures the duration of proxied HTTP requests, per service | +| `opencloud_proxy_request_total{method,result,service}` | `method`: the HTTP method
`result`: one of `success`, `client-error`, `server-error`, depending on the status code in the response of the proxied HTTP request
`services`: identifier of the service the request is proxied to | Counts the number of proxied requests | +| `opencloud_proxy_request_duration_seconds_bucket{method,result,service=}` | `method`: the HTTP method
`result`: one of `success`, `client-error`, `server-error`, depending on the status code in the response of the proxied HTTP request
`services`: identifier of the service the request is proxied to | Native histogram that measures the duration of proxied HTTP requests, per service | +| `opencloud_proxy_build_info{version}` | `version`: build version of the proxy | A gauge with a constant value of `1` | ### Prometheus Configuration The following is an example prometheus configuration for the single process mode. It assumes that the proxy debug address is configured to bind on all interfaces `PROXY_DEBUG_ADDR=0.0.0.0:9205` and that the proxy is available via the `opencloud` service name (typically in docker-compose). The prometheus service detects the `/metrics` endpoint automatically and scrapes it every 15 seconds. @@ -318,3 +320,17 @@ scrape_configs: static_configs: - targets: ["opencloud:9205"] ``` + +In order to process native histograms, use this configuration instead: + +```yaml +global: + scrape_interval: 15s + scrape_native_histograms: true + scrape_protocols: ['PrometheusProto', 'OpenMetricsText1.0.0'] +scrape_configs: + - job_name: opencloud_proxy + static_configs: + - targets: ["opencloud:9205"] +``` + diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 2a9a5cc0ba..95f9f2625f 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -26,7 +26,6 @@ import ( "github.com/opencloud-eu/opencloud/pkg/runner" "github.com/opencloud-eu/opencloud/pkg/service/grpc" "github.com/opencloud-eu/opencloud/pkg/tracing" - "github.com/opencloud-eu/opencloud/pkg/version" policiessvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/policies/v0" settingssvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/settings/v0" "github.com/opencloud-eu/opencloud/services/proxy/pkg/config" @@ -122,8 +121,20 @@ func Server(cfg *config.Config) *cobra.Command { defer cancel() } - m := metrics.New() - m.BuildInfo.WithLabelValues(version.GetString()).Set(1) + m, err := metrics.New(func(yield func(config.Route) bool) { + // provide the metrics with an iterator that gives a list of the routes, + // to allow the metrics to initialize empty collectors accordingly + for _, pol := range cfg.Policies { + for _, r := range pol.Routes { + if !yield(r) { + return + } + } + } + }, &logger) + if err != nil { + return fmt.Errorf("failed to initialize metrics in reverse proxy: %w", err) + } rp, err := proxy.NewMultiHostReverseProxy( proxy.Logger(logger), @@ -201,7 +212,7 @@ func Server(cfg *config.Config) *cobra.Command { proxyHTTP.Logger(logger), proxyHTTP.Context(cfg.Context), proxyHTTP.Config(cfg), - proxyHTTP.Metrics(metrics.New()), + proxyHTTP.Metrics(m), proxyHTTP.Middlewares(middlewares), ) if err != nil { @@ -353,7 +364,6 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, ), middleware.Tracer(traceProvider), pkgmiddleware.TraceContext, - middleware.Instrumenter(metrics), middleware.AccessLog(logger), middleware.ContextLogger(logger), middleware.HTTPSRedirect, // redirect to https if enabled @@ -365,7 +375,8 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, middleware.Security(cspConfig), // 3. Routing & Authentication - router.Middleware(serviceSelector, cfg.PolicySelector, cfg.Policies, logger), + router.Middleware(serviceSelector, cfg.PolicySelector, cfg.Policies, metrics.RoutingFailed, logger), + middleware.Instrumenter(metrics), // must come after the router middleware as it needs to know the routeInfo for detailed metrics middleware.Authentication( authenticators, middleware.CredentialsByUserAgent(cfg.AuthMiddleware.CredentialsByUserAgent), diff --git a/services/proxy/pkg/metrics/metrics.go b/services/proxy/pkg/metrics/metrics.go index 57143cd8e6..8d711fd1f8 100644 --- a/services/proxy/pkg/metrics/metrics.go +++ b/services/proxy/pkg/metrics/metrics.go @@ -1,6 +1,16 @@ package metrics import ( + "errors" + "iter" + "net/http" + "sync/atomic" + "time" + + "github.com/opencloud-eu/opencloud/pkg/log" + ocmetrics "github.com/opencloud-eu/opencloud/pkg/metrics" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/config" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/router" "github.com/prometheus/client_golang/prometheus" ) @@ -14,48 +24,169 @@ var ( // Metrics defines the available metrics of this service. type Metrics struct { - Requests *prometheus.CounterVec - Errors *prometheus.CounterVec - Duration *prometheus.HistogramVec - BuildInfo *prometheus.GaugeVec + routingFailures prometheus.Counter + legacyCount *prometheus.CounterVec + duration *prometheus.HistogramVec + legacyDuration *prometheus.HistogramVec + inflightByService map[string]*atomic.Int64 +} + +const ( + LabelMethod = "method" + LabelService = "service" + LabelResult = "result" +) + +const ( + ResultSuccess = "success" + ResultClientError = "client-error" + ResultServerError = "server-error" +) + +func resultFromStatusCode(statusCode int) string { + if statusCode < 300 { + return ResultSuccess + } + if statusCode < 500 { + return ResultClientError + } + return ResultServerError } // New initializes the available metrics. -func New() *Metrics { +func New(routes iter.Seq[config.Route], logger *log.Logger) (*Metrics, error) { m := &Metrics{ - Requests: prometheus.NewCounterVec(prometheus.CounterOpts{ + routingFailures: prometheus.NewCounter(prometheus.CounterOpts{ Namespace: Namespace, Subsystem: Subsystem, - Name: "requests_total", - Help: "How many requests processed in total", - }, []string{"method"}), - Errors: prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "routing_failure_count", + Help: "number of inbound requests that could not be routed", + }), + + // Since we have a higher cardinality with this one, as we have three labels, + // we really should use Prometheus native histograms, as those are still recorded + // as singular samples, instead of a matrix of + // method ⨯ service ⨯ result ⨯ bucket + // where bucket is going to have about a dozen values. + duration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "request_duration_seconds", + Help: "request duration in seconds", + NativeHistogramBucketFactor: 1.1, // native exponential histograms with a 10% maximum bucket width + }, []string{LabelMethod, LabelService, LabelResult}), + + // In order to still make those measures available to Prometheus scrapers that are + // not configured to support native histograms (https://prometheus.io/docs/specs/native_histograms/), + // we also keep these two metrics. + // Once native histograms become the default in Prometheus scrapers, we could remove those + // two metrics below, as the one above provides all that data already (including the + // counter). + + // First, a counter which has the higher cardinality of + // method ⨯ service ⨯ result + // but without buckets, since it's just a counter. + legacyCount: prometheus.NewCounterVec(prometheus.CounterOpts{ Namespace: Namespace, Subsystem: Subsystem, - Name: "errors_total", - Help: "How many requests run into errors", - }, []string{"method"}), - Duration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ + Name: "request_total", + Help: "total number of requests", + }, []string{LabelMethod, LabelService, LabelResult}), + + // Secondly, a histogram that buckets the duration, but since this is not a native histogram, + // we want to keep the cardinality in check by only using the service as label. + legacyDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: Namespace, Subsystem: Subsystem, Name: "duration_seconds", - Help: "request duration in seconds", - }, []string{"method"}), - BuildInfo: prometheus.NewGaugeVec(prometheus.GaugeOpts{ - Namespace: Namespace, - Subsystem: Subsystem, - Name: "build_info", - Help: "Build Information", - }, []string{"version"}), + Help: "request duration in seconds (legacy)", + }, []string{LabelService}), + } + + // Initialize the metrics with 0 so that they immediately show up in the list of + // scraped metrics, instead of only showing up on-demand when they first collect + // a value later on, and possibly never + inflightByService := map[string]*atomic.Int64{} + inflightByServiceGaugeFuncs := map[string]prometheus.GaugeFunc{} + for route := range routes { + method := route.Method + if method == "" { + method = http.MethodGet + } + m.legacyDuration.WithLabelValues(route.Service) // initializes a Histogram as empty + + var counter atomic.Int64 + inflightByService[route.Service] = &counter + inflightByServiceGaugeFuncs[route.Service] = prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "concurrent_service_requests", + Help: "number of concurrent requests being processed for a given service", + ConstLabels: prometheus.Labels{LabelService: route.Service}, + }, func() float64 { + return float64(counter.Load()) + }) + + for _, result := range []string{ResultSuccess, ResultClientError, ResultServerError} { + m.duration.WithLabelValues(method, route.Service, result) // initializes a Histogram as empty + m.legacyCount.WithLabelValues(method, route.Service, result).Add(0) // initializes a Counter as empty + } + } + + m.inflightByService = inflightByService + + buildInfo := ocmetrics.BuildInfo(Namespace, Subsystem) + + errs := []error{} + // registers all the exported metrics: + errs = append(errs, ocmetrics.RegisterAll(prometheus.DefaultRegisterer, m, logger)) + // need an additional call for the unexported ones: + errs = append(errs, ocmetrics.RegisterMetrics(prometheus.DefaultRegisterer, logger, + // need to list unexported metrics here: + buildInfo, + m.routingFailures, + m.duration, + m.legacyCount, + m.legacyDuration, + )) + // need to iterate over these as the number of entries is dynamic: + { + collectors := make([]prometheus.Collector, 0, len(inflightByServiceGaugeFuncs)) + for _, c := range inflightByServiceGaugeFuncs { + collectors = append(collectors, c) + } + errs = append(errs, ocmetrics.RegisterMetrics(prometheus.DefaultRegisterer, logger, collectors...)) } + return m, errors.Join(errs...) +} - // Initialize the metrics with 0 - m.Requests.WithLabelValues("GET").Add(0) - m.Errors.WithLabelValues("GET").Add(0) +func (m *Metrics) Duration(r *http.Request, statusCode int, duration time.Duration) { + ri := router.ContextRoutingInfo(r.Context()) + service := ri.Service() + d := float64(duration.Seconds()) + result := resultFromStatusCode(statusCode) - _ = prometheus.Register(m.Requests) - _ = prometheus.Register(m.Errors) - _ = prometheus.Register(m.Duration) - _ = prometheus.Register(m.BuildInfo) - return m + m.duration.WithLabelValues(r.Method, service, result).Observe(d) + m.legacyDuration.WithLabelValues(service).Observe(d) + m.legacyCount.WithLabelValues(r.Method, service, result).Inc() +} + +func (m *Metrics) RoutingFailed(r *http.Request) { + m.routingFailures.Inc() +} + +func (m *Metrics) InFlightInc(r *http.Request) { + ri := router.ContextRoutingInfo(r.Context()) + service := ri.Service() + if counter, ok := m.inflightByService[service]; ok { + counter.Add(1) + } +} + +func (m *Metrics) InFlightDec(r *http.Request) { + ri := router.ContextRoutingInfo(r.Context()) + service := ri.Service() + if counter, ok := m.inflightByService[service]; ok { + counter.Add(-1) + } } diff --git a/services/proxy/pkg/middleware/metrics.go b/services/proxy/pkg/middleware/metrics.go index 8c2c640fb5..c18ef20444 100644 --- a/services/proxy/pkg/middleware/metrics.go +++ b/services/proxy/pkg/middleware/metrics.go @@ -6,7 +6,6 @@ import ( "github.com/go-chi/chi/v5/middleware" "github.com/opencloud-eu/opencloud/services/proxy/pkg/metrics" - "github.com/prometheus/client_golang/prometheus" ) // Instrumenter provides a middleware to create metrics @@ -14,15 +13,14 @@ func Instrumenter(m metrics.Metrics) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { start := time.Now() + m.InFlightInc(r) + defer m.InFlightDec(r) + ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor) - m.Requests.With(prometheus.Labels{"method": r.Method}).Inc() next.ServeHTTP(ww, r) - m.Duration.With(prometheus.Labels{"method": r.Method}).Observe(float64(time.Since(start).Seconds())) - if ww.Status() >= 500 { - m.Errors.With(prometheus.Labels{"method": r.Method}).Inc() - } + m.Duration(r, ww.Status(), time.Since(start)) }) } } diff --git a/services/proxy/pkg/proxy/proxy_integration_test.go b/services/proxy/pkg/proxy/proxy_integration_test.go index ace5543a59..a72c9a587c 100644 --- a/services/proxy/pkg/proxy/proxy_integration_test.go +++ b/services/proxy/pkg/proxy/proxy_integration_test.go @@ -121,7 +121,7 @@ func TestProxyIntegration(t *testing.T) { t.Parallel() tc := tests[k] - rt := router.Middleware(sel, nil, tc.conf, log.NewLogger()) + rt := router.Middleware(sel, nil, tc.conf, func(_ *http.Request) {}, log.NewLogger()) rp := newTestProxy(testConfig(tc.conf), func(req *http.Request) *http.Response { if got, want := req.URL.String(), tc.expect.String(); got != want { t.Errorf("Proxied url should be %v got %v", want, got) diff --git a/services/proxy/pkg/router/router.go b/services/proxy/pkg/router/router.go index 9406ef0af2..ac08c6bb53 100644 --- a/services/proxy/pkg/router/router.go +++ b/services/proxy/pkg/router/router.go @@ -20,12 +20,13 @@ type routingInfoCtxKey struct{} var noInfo = RoutingInfo{} // Middleware returns a HTTP middleware containing the router. -func Middleware(serviceSelector selector.Selector, policySelectorCfg *config.PolicySelector, policies []config.Policy, logger log.Logger) func(http.Handler) http.Handler { +func Middleware(serviceSelector selector.Selector, policySelectorCfg *config.PolicySelector, policies []config.Policy, onFailedRouting func(r *http.Request), logger log.Logger) func(http.Handler) http.Handler { router := New(serviceSelector, policySelectorCfg, policies, logger) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ri, ok := router.Route(r) if !ok { + onFailedRouting(r) w.WriteHeader(http.StatusInternalServerError) return } @@ -88,6 +89,7 @@ func New(serviceSelector selector.Selector, policySelectorCfg *config.PolicySele // RoutingInfo contains the proxy rewrite hook and some information about the route. type RoutingInfo struct { rewrite func(*httputil.ProxyRequest) + service string endpoint string unprotected bool remoteUserHeader string @@ -115,6 +117,11 @@ func (r RoutingInfo) SkipXAccessToken() bool { return r.skipXAccessToken } +// Returns the identifier of the service the request ought to be routed to +func (r RoutingInfo) Service() string { + return r.service +} + // Router handles the routing of HTTP requests according to the given policies. type Router struct { logger log.Logger @@ -141,6 +148,7 @@ func (rt Router) addHost(policy string, target *url.URL, route config.Route) { rt.rewriters[policy][routeType][route.Method] = append(rt.rewriters[policy][routeType][route.Method], RoutingInfo{ endpoint: route.Endpoint, + service: route.Service, unprotected: route.Unprotected, remoteUserHeader: route.RemoteUserHeader, skipXAccessToken: route.SkipXAccessToken,