Merge #4452

4452: [Metrics] Fix issue with collecting REST Metrics r=peterargue a=Guitarheroua

This fix is implemented in response to [a comment made in PR 4288 ](#4288 (comment)).

After enabling metrics for REST, the system now generates metrics for each HTTP request based on the request method and URL path. However, when a path includes an ID, such as '**/v1/collection/12345/**' or '**/v1/collection/67890/**', a unique value is created for each path, leading to an issue.

To address this problem, I have modified the reporting logic to generate metrics based on the service ID and request method. Currently, the service ID is empty, but there is potential for improvement in future iterations.

Co-authored-by: Andrii Slisarchuk <andriyslisarchuk@gmail.com>
Co-authored-by: Andrii Slisarchuk <Guitarheroua@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>

engine/access/rest/middleware/metrics.go

@@ -11,12 +11,19 @@ import (
 	"github.com/onflow/flow-go/module"
 )
 
-func MetricsMiddleware(restCollector module.RestMetrics) mux.MiddlewareFunc {
+func MetricsMiddleware(restCollector module.RestMetrics, urlToRoute func(string) (string, error)) 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, req.URL.Path)
+			restCollector.AddTotalRequests(req.Context(), req.Method, routeName)
 
 			// Modify the writer
 			respWriter := &responseWriter{w, http.StatusOK}

engine/access/rest/router.go

@@ -1,7 +1,10 @@
 package rest
 
 import (
+	"fmt"
 	"net/http"
+	"regexp"
+	"strings"
 
 	"github.com/gorilla/mux"
 	"github.com/rs/zerolog"
@@ -21,8 +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())
-	// collecting too much metrics
-	// v1SubRouter.Use(middleware.MetricsMiddleware(restCollector))
+	v1SubRouter.Use(middleware.MetricsMiddleware(restCollector, URLToRoute))
 
 	linkGenerator := models.NewLinkGeneratorImpl(v1SubRouter)
 
@@ -115,3 +117,59 @@ var Routes = []route{{
 	Name:    "getNodeVersionInfo",
 	Handler: GetNodeVersionInfo,
 }}
+
+var routeUrlMap = map[string]string{}
+var routeRE = regexp.MustCompile(`(?i)/v1/(\w+)(/(\w+)(/(\w+))?)?`)
+
+func init() {
+	for _, r := range Routes {
+		routeUrlMap[r.Pattern] = r.Name
+	}
+}
+
+func URLToRoute(url string) (string, error) {
+	normalized, err := normalizeURL(url)
+	if err != nil {
+		return "", err
+	}
+
+	name, ok := routeUrlMap[normalized]
+	if !ok {
+		return "", fmt.Errorf("invalid url")
+	}
+	return name, nil
+}
+
+func normalizeURL(url string) (string, error) {
+	matches := routeRE.FindAllStringSubmatch(url, -1)
+	if len(matches) != 1 || len(matches[0]) != 6 {
+		return "", fmt.Errorf("invalid url")
+	}
+
+	// given a URL like
+	//      /v1/blocks/1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef/payload
+	// groups  [  1  ] [                                3                             ] [  5  ]
+	// normalized form like /v1/blocks/{id}/payload
+
+	parts := []string{matches[0][1]}
+
+	switch len(matches[0][3]) {
+	case 0:
+		// top level resource. e.g. /v1/blocks
+	case 64:
+		// id based resource. e.g. /v1/blocks/1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef
+		parts = append(parts, "{id}")
+	case 16:
+		// address based resource. e.g. /v1/accounts/1234567890abcdef
+		parts = append(parts, "{address}")
+	default:
+		// named resource. e.g. /v1/network/parameters
+		parts = append(parts, matches[0][3])
+	}
+
+	if matches[0][5] != "" {
+		parts = append(parts, matches[0][5])
+	}
+
+	return "/" + strings.Join(parts, "/"), nil
+}

engine/access/rest/router_test.go

@@ -0,0 +1,185 @@
+package rest
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseURL(t *testing.T) {
+	tests := []struct {
+		name     string
+		url      string
+		expected string
+	}{
+		{
+			name:     "/v1/transactions",
+			url:      "/v1/transactions",
+			expected: "createTransaction",
+		},
+		{
+			name:     "/v1/transactions/{id}",
+			url:      "/v1/transactions/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getTransactionByID",
+		},
+		{
+			name:     "/v1/transaction_results/{id}",
+			url:      "/v1/transaction_results/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getTransactionResultByID",
+		},
+		{
+			name:     "/v1/blocks",
+			url:      "/v1/blocks",
+			expected: "getBlocksByHeight",
+		},
+		{
+			name:     "/v1/blocks/{id}",
+			url:      "/v1/blocks/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getBlocksByIDs",
+		},
+		{
+			name:     "/v1/blocks/{id}/payload",
+			url:      "/v1/blocks/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76/payload",
+			expected: "getBlockPayloadByID",
+		},
+		{
+			name:     "/v1/execution_results/{id}",
+			url:      "/v1/execution_results/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getExecutionResultByID",
+		},
+		{
+			name:     "/v1/execution_results",
+			url:      "/v1/execution_results",
+			expected: "getExecutionResultByBlockID",
+		},
+		{
+			name:     "/v1/collections/{id}",
+			url:      "/v1/collections/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getCollectionByID",
+		},
+		{
+			name:     "/v1/scripts",
+			url:      "/v1/scripts",
+			expected: "executeScript",
+		},
+		{
+			name:     "/v1/accounts/{address}",
+			url:      "/v1/accounts/6a587be304c1224c",
+			expected: "getAccount",
+		},
+		{
+			name:     "/v1/events",
+			url:      "/v1/events",
+			expected: "getEvents",
+		},
+		{
+			name:     "/v1/network/parameters",
+			url:      "/v1/network/parameters",
+			expected: "getNetworkParameters",
+		},
+		{
+			name:     "/v1/node_version_info",
+			url:      "/v1/node_version_info",
+			expected: "getNodeVersionInfo",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := URLToRoute(tt.url)
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
+}
+
+func TestBenchmarkParseURL(t *testing.T) {
+	tests := []struct {
+		name     string
+		url      string
+		expected string
+	}{
+		{
+			name:     "/v1/transactions",
+			url:      "/v1/transactions",
+			expected: "createTransaction",
+		},
+		{
+			name:     "/v1/transactions/{id}",
+			url:      "/v1/transactions/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getTransactionByID",
+		},
+		{
+			name:     "/v1/transaction_results/{id}",
+			url:      "/v1/transaction_results/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getTransactionResultByID",
+		},
+		{
+			name:     "/v1/blocks",
+			url:      "/v1/blocks",
+			expected: "getBlocksByHeight",
+		},
+		{
+			name:     "/v1/blocks/{id}",
+			url:      "/v1/blocks/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getBlocksByIDs",
+		},
+		{
+			name:     "/v1/blocks/{id}/payload",
+			url:      "/v1/blocks/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76/payload",
+			expected: "getBlockPayloadByID",
+		},
+		{
+			name:     "/v1/execution_results/{id}",
+			url:      "/v1/execution_results/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getExecutionResultByID",
+		},
+		{
+			name:     "/v1/execution_results",
+			url:      "/v1/execution_results",
+			expected: "getExecutionResultByBlockID",
+		},
+		{
+			name:     "/v1/collections/{id}",
+			url:      "/v1/collections/53730d3f3d2d2f46cb910b16db817d3a62adaaa72fdb3a92ee373c37c5b55a76",
+			expected: "getCollectionByID",
+		},
+		{
+			name:     "/v1/scripts",
+			url:      "/v1/scripts",
+			expected: "executeScript",
+		},
+		{
+			name:     "/v1/accounts/{address}",
+			url:      "/v1/accounts/6a587be304c1224c",
+			expected: "getAccount",
+		},
+		{
+			name:     "/v1/events",
+			url:      "/v1/events",
+			expected: "getEvents",
+		},
+		{
+			name:     "/v1/network/parameters",
+			url:      "/v1/network/parameters",
+			expected: "getNetworkParameters",
+		},
+		{
+			name:     "/v1/node_version_info",
+			url:      "/v1/node_version_info",
+			expected: "getNodeVersionInfo",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			start := time.Now()
+			for i := 0; i < 100_000; i++ {
+				_, _ = URLToRoute(tt.url)
+			}
+			t.Logf("%s: %v", tt.name, time.Since(start)/100_000)
+		})
+	}
+}

module/metrics.go

@@ -605,7 +605,7 @@ type RestMetrics interface {
 	// Example recorder taken from:
 	// https://github.com/slok/go-http-metrics/blob/master/metrics/prometheus/prometheus.go
 	httpmetrics.Recorder
-	AddTotalRequests(ctx context.Context, service string, id string)
+	AddTotalRequests(ctx context.Context, method string, routeName string)
 }
 
 type GRPCConnectionPoolMetrics interface {

module/metrics/rest_api.go

@@ -81,7 +81,7 @@ func NewRestCollector(cfg metricsProm.Config) module.RestMetrics {
 			Subsystem: "http",
 			Name:      "requests_total",
 			Help:      "The number of requests handled over time.",
-		}, []string{cfg.ServiceLabel, cfg.HandlerIDLabel}),
+		}, []string{cfg.MethodLabel, cfg.HandlerIDLabel}),
 	}
 
 	cfg.Registry.MustRegister(
@@ -108,6 +108,6 @@ func (r *RestCollector) AddInflightRequests(_ context.Context, p httpmetrics.HTT
 }
 
 // New custom method to track all requests made for every REST API request
-func (r *RestCollector) AddTotalRequests(_ context.Context, method string, id string) {
-	r.httpRequestsTotal.WithLabelValues(method, id).Inc()
+func (r *RestCollector) AddTotalRequests(_ context.Context, method string, routeName string) {
+	r.httpRequestsTotal.WithLabelValues(method, routeName).Inc()
 }