Merge #4571

4571: [Access] Update REST metrics to use route name for all types r=peterargue a=peterargue

The REST metrics handler passes the original route into the collector, causing high cardinality and performance issues on the prometheus servers. We attempted to fix this with #4452, but only updated 1 of 4 metrics that were reporting the URL.

This PR updates all metrics to use the mapped route name. It also removes a bunch of unnecessary config and aligns the collector with the other metrics collectors in flow-go

Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>

cmd/access/node_builder/access_node_builder.go

@@ -37,6 +37,7 @@ import (
 	"github.com/onflow/flow-go/crypto"
 	"github.com/onflow/flow-go/engine/access/ingestion"
 	pingeng "github.com/onflow/flow-go/engine/access/ping"
+	"github.com/onflow/flow-go/engine/access/rest"
 	"github.com/onflow/flow-go/engine/access/rpc"
 	"github.com/onflow/flow-go/engine/access/rpc/backend"
 	"github.com/onflow/flow-go/engine/access/state_stream"
@@ -216,6 +217,7 @@ type FlowAccessNodeBuilder struct {
 	CollectionsToMarkExecuted  *stdmap.Times
 	BlocksToMarkExecuted       *stdmap.Times
 	TransactionMetrics         *metrics.TransactionCollector
+	RestMetrics                *metrics.RestCollector
 	AccessMetrics              module.AccessMetrics
 	PingMetrics                module.PingMetrics
 	Committee                  hotstuff.DynamicCommittee
@@ -964,10 +966,19 @@ func (builder *FlowAccessNodeBuilder) Build() (cmd.Node, error) {
 			)
 			return nil
 		}).
+		Module("rest metrics", func(node *cmd.NodeConfig) error {
+			m, err := metrics.NewRestCollector(rest.URLToRoute, node.MetricsRegisterer)
+			if err != nil {
+				return err
+			}
+			builder.RestMetrics = m
+			return nil
+		}).
 		Module("access metrics", func(node *cmd.NodeConfig) error {
 			builder.AccessMetrics = metrics.NewAccessCollector(
 				metrics.WithTransactionMetrics(builder.TransactionMetrics),
 				metrics.WithBackendScriptsMetrics(builder.TransactionMetrics),
+				metrics.WithRestMetrics(builder.RestMetrics),
 			)
 			return nil
 		}).

engine/access/rest/middleware/metrics.go

@@ -11,19 +11,12 @@ import (
 	"github.com/onflow/flow-go/module"
 )
 
-func MetricsMiddleware(restCollector module.RestMetrics, urlToRoute func(string) (string, error)) mux.MiddlewareFunc {
+func MetricsMiddleware(restCollector module.RestMetrics) mux.MiddlewareFunc {
 	metricsMiddleware := middleware.New(middleware.Config{Recorder: restCollector})
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-			//urlToRoute transforms specific URL to generic url pattern
-			routeName, err := urlToRoute(req.URL.Path)
-			if err != nil {
-				// In case of an error, an empty route name filled with "unknown"
-				routeName = "unknown"
-			}
-
 			// This is a custom metric being called on every http request
-			restCollector.AddTotalRequests(req.Context(), req.Method, routeName)
+			restCollector.AddTotalRequests(req.Context(), req.Method, req.URL.Path)
 
 			// Modify the writer
 			respWriter := &responseWriter{w, http.StatusOK}

engine/access/rest/router.go

@@ -24,7 +24,7 @@ func newRouter(backend access.API, logger zerolog.Logger, chain flow.Chain, rest
 	v1SubRouter.Use(middleware.LoggingMiddleware(logger))
 	v1SubRouter.Use(middleware.QueryExpandable())
 	v1SubRouter.Use(middleware.QuerySelect())
-	v1SubRouter.Use(middleware.MetricsMiddleware(restCollector, URLToRoute))
+	v1SubRouter.Use(middleware.MetricsMiddleware(restCollector))
 
 	linkGenerator := models.NewLinkGeneratorImpl(v1SubRouter)
 

module/metrics/access.go

@@ -3,7 +3,6 @@ package metrics
 import (
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
-	metricsProm "github.com/slok/go-http-metrics/metrics/prometheus"
 
 	"github.com/onflow/flow-go/module"
 	"github.com/onflow/flow-go/module/counters"
@@ -23,6 +22,12 @@ func WithBackendScriptsMetrics(m module.BackendScriptsMetrics) AccessCollectorOp
 	}
 }
 
+func WithRestMetrics(m module.RestMetrics) AccessCollectorOpts {
+	return func(ac *AccessCollector) {
+		ac.RestMetrics = m
+	}
+}
+
 type AccessCollector struct {
 	module.RestMetrics
 	module.TransactionMetrics
@@ -101,8 +106,6 @@ func NewAccessCollector(opts ...AccessCollectorOpts) *AccessCollector {
 			Help:      "gauge to track the maximum block height of execution receipts received",
 		}),
 		maxReceiptHeightValue: counters.NewMonotonousCounter(0),
-
-		RestMetrics: NewRestCollector(metricsProm.Config{Prefix: "access_rest_api"}),
 	}
 
 	for _, opt := range opts {

module/metrics/labels.go

@@ -20,6 +20,10 @@ const (
 	LabelSuccess             = "success"
 	LabelCtrlMsgType         = "control_message"
 	LabelMisbehavior         = "misbehavior"
+	LabelHandler             = "handler"
+	LabelStatusCode          = "code"
+	LabelMethod              = "method"
+	LabelService             = "service"
 )
 
 const (

module/metrics/namespaces.go

@@ -15,6 +15,7 @@ const (
 	namespaceExecutionDataSync = "execution_data_sync"
 	namespaceChainsync         = "chainsync"
 	namespaceFollowerEngine    = "follower"
+	namespaceRestAPI           = "access_rest_api"
 )
 
 // Network subsystems represent the various layers of networking.
@@ -43,6 +44,7 @@ const (
 	subsystemTransactionTiming     = "transaction_timing"
 	subsystemTransactionSubmission = "transaction_submission"
 	subsystemConnectionPool        = "connection_pool"
+	subsystemHTTP                  = "http"
 )
 
 // Observer subsystem

module/metrics/rest_api.go

@@ -2,112 +2,112 @@ package metrics
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
+	httpmetrics "github.com/slok/go-http-metrics/metrics"
 
 	"github.com/onflow/flow-go/module"
-
-	httpmetrics "github.com/slok/go-http-metrics/metrics"
-	metricsProm "github.com/slok/go-http-metrics/metrics/prometheus"
 )
 
 type RestCollector struct {
 	httpRequestDurHistogram   *prometheus.HistogramVec
 	httpResponseSizeHistogram *prometheus.HistogramVec
 	httpRequestsInflight      *prometheus.GaugeVec
 	httpRequestsTotal         *prometheus.GaugeVec
+
+	// urlToRouteMapper is a callback that converts a URL to a route name
+	urlToRouteMapper func(string) (string, error)
 }
 
 var _ module.RestMetrics = (*RestCollector)(nil)
 
 // NewRestCollector returns a new metrics RestCollector that implements the RestCollector
 // using Prometheus as the backend.
-func NewRestCollector(cfg metricsProm.Config) module.RestMetrics {
-	if len(cfg.DurationBuckets) == 0 {
-		cfg.DurationBuckets = prometheus.DefBuckets
-	}
-
-	if len(cfg.SizeBuckets) == 0 {
-		cfg.SizeBuckets = prometheus.ExponentialBuckets(100, 10, 8)
-	}
-
-	if cfg.Registry == nil {
-		cfg.Registry = prometheus.DefaultRegisterer
-	}
-
-	if cfg.HandlerIDLabel == "" {
-		cfg.HandlerIDLabel = "handler"
-	}
-
-	if cfg.StatusCodeLabel == "" {
-		cfg.StatusCodeLabel = "code"
-	}
-
-	if cfg.MethodLabel == "" {
-		cfg.MethodLabel = "method"
-	}
-
-	if cfg.ServiceLabel == "" {
-		cfg.ServiceLabel = "service"
+func NewRestCollector(urlToRouteMapper func(string) (string, error), registerer prometheus.Registerer) (*RestCollector, error) {
+	if urlToRouteMapper == nil {
+		return nil, fmt.Errorf("urlToRouteMapper cannot be nil")
 	}
 
 	r := &RestCollector{
+		urlToRouteMapper: urlToRouteMapper,
 		httpRequestDurHistogram: prometheus.NewHistogramVec(prometheus.HistogramOpts{
-			Namespace: cfg.Prefix,
-			Subsystem: "http",
+			Namespace: namespaceRestAPI,
+			Subsystem: subsystemHTTP,
 			Name:      "request_duration_seconds",
 			Help:      "The latency of the HTTP requests.",
-			Buckets:   cfg.DurationBuckets,
-		}, []string{cfg.ServiceLabel, cfg.HandlerIDLabel, cfg.MethodLabel, cfg.StatusCodeLabel}),
+			Buckets:   prometheus.DefBuckets,
+		}, []string{LabelService, LabelHandler, LabelMethod, LabelStatusCode}),
 
 		httpResponseSizeHistogram: prometheus.NewHistogramVec(prometheus.HistogramOpts{
-			Namespace: cfg.Prefix,
-			Subsystem: "http",
+			Namespace: namespaceRestAPI,
+			Subsystem: subsystemHTTP,
 			Name:      "response_size_bytes",
 			Help:      "The size of the HTTP responses.",
-			Buckets:   cfg.SizeBuckets,
-		}, []string{cfg.ServiceLabel, cfg.HandlerIDLabel, cfg.MethodLabel, cfg.StatusCodeLabel}),
+			Buckets:   prometheus.ExponentialBuckets(100, 10, 8),
+		}, []string{LabelService, LabelHandler, LabelMethod, LabelStatusCode}),
 
 		httpRequestsInflight: prometheus.NewGaugeVec(prometheus.GaugeOpts{
-			Namespace: cfg.Prefix,
-			Subsystem: "http",
+			Namespace: namespaceRestAPI,
+			Subsystem: subsystemHTTP,
 			Name:      "requests_inflight",
 			Help:      "The number of inflight requests being handled at the same time.",
-		}, []string{cfg.ServiceLabel, cfg.HandlerIDLabel}),
+		}, []string{LabelService, LabelHandler}),
 
 		httpRequestsTotal: prometheus.NewGaugeVec(prometheus.GaugeOpts{
-			Namespace: cfg.Prefix,
-			Subsystem: "http",
+			Namespace: namespaceRestAPI,
+			Subsystem: subsystemHTTP,
 			Name:      "requests_total",
 			Help:      "The number of requests handled over time.",
-		}, []string{cfg.MethodLabel, cfg.HandlerIDLabel}),
+		}, []string{LabelMethod, LabelHandler}),
 	}
 
-	cfg.Registry.MustRegister(
+	registerer.MustRegister(
 		r.httpRequestDurHistogram,
 		r.httpResponseSizeHistogram,
 		r.httpRequestsInflight,
 		r.httpRequestsTotal,
 	)
 
-	return r
+	return r, nil
 }
 
-// These methods are called automatically by go-http-metrics/middleware
+// ObserveHTTPRequestDuration records the duration of the REST request.
+// This method is called automatically by go-http-metrics/middleware
 func (r *RestCollector) ObserveHTTPRequestDuration(_ context.Context, p httpmetrics.HTTPReqProperties, duration time.Duration) {
-	r.httpRequestDurHistogram.WithLabelValues(p.Service, p.ID, p.Method, p.Code).Observe(duration.Seconds())
+	handler := r.mapURLToRoute(p.ID)
+	r.httpRequestDurHistogram.WithLabelValues(p.Service, handler, p.Method, p.Code).Observe(duration.Seconds())
 }
 
+// ObserveHTTPResponseSize records the response size of the REST request.
+// This method is called automatically by go-http-metrics/middleware
 func (r *RestCollector) ObserveHTTPResponseSize(_ context.Context, p httpmetrics.HTTPReqProperties, sizeBytes int64) {
-	r.httpResponseSizeHistogram.WithLabelValues(p.Service, p.ID, p.Method, p.Code).Observe(float64(sizeBytes))
+	handler := r.mapURLToRoute(p.ID)
+	r.httpResponseSizeHistogram.WithLabelValues(p.Service, handler, p.Method, p.Code).Observe(float64(sizeBytes))
 }
 
+// AddInflightRequests increments and decrements the number of inflight request being processed.
+// This method is called automatically by go-http-metrics/middleware
 func (r *RestCollector) AddInflightRequests(_ context.Context, p httpmetrics.HTTPProperties, quantity int) {
-	r.httpRequestsInflight.WithLabelValues(p.Service, p.ID).Add(float64(quantity))
+	handler := r.mapURLToRoute(p.ID)
+	r.httpRequestsInflight.WithLabelValues(p.Service, handler).Add(float64(quantity))
+}
+
+// AddTotalRequests records all REST requests
+// This is a custom method called by the REST handler
+func (r *RestCollector) AddTotalRequests(_ context.Context, method, path string) {
+	handler := r.mapURLToRoute(path)
+	r.httpRequestsTotal.WithLabelValues(method, handler).Inc()
 }
 
-// New custom method to track all requests made for every REST API request
-func (r *RestCollector) AddTotalRequests(_ context.Context, method string, routeName string) {
-	r.httpRequestsTotal.WithLabelValues(method, routeName).Inc()
+// mapURLToRoute uses the urlToRouteMapper callback to convert a URL to a route name
+// This normalizes the URL, removing dynamic information converting it to a static string
+func (r *RestCollector) mapURLToRoute(url string) string {
+	route, err := r.urlToRouteMapper(url)
+	if err != nil {
+		return "unknown"
+	}
+
+	return route
 }