From f70259eaf8e46f69d7057b2311c36b32debef97f Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Fri, 10 Jan 2025 13:56:18 +0100 Subject: [PATCH] frontend: lint Prometheus metrics This change renames the `frontend_count` metric to `frontend_requests_total` to comply with the Prometheus guidelines. It also bounds the cardinality of the `route` label by using the matched pattern instead of the actual path. Signed-off-by: Simon Pasquier --- frontend/pkg/frontend/frontend_test.go | 29 ++++++++++++++++++++++--- frontend/pkg/frontend/metrics.go | 18 ++++++++++----- frontend/pkg/frontend/node_pool_test.go | 5 ++++- 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/frontend/pkg/frontend/frontend_test.go b/frontend/pkg/frontend/frontend_test.go index 143d4389c..23096a033 100644 --- a/frontend/pkg/frontend/frontend_test.go +++ b/frontend/pkg/frontend/frontend_test.go @@ -14,6 +14,7 @@ import ( "time" "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil" "go.uber.org/mock/gomock" "github.com/Azure/ARO-HCP/internal/api" @@ -62,10 +63,11 @@ func TestReadiness(t *testing.T) { t.Run(test.name, func(t *testing.T) { ctrl := gomock.NewController(t) mockDBClient := mocks.NewMockDBClient(ctrl) + reg := prometheus.NewRegistry() f := &Frontend{ dbClient: mockDBClient, - metrics: NewPrometheusEmitter(prometheus.NewRegistry()), + metrics: NewPrometheusEmitter(reg), } f.ready.Store(test.ready) ts := httptest.NewServer(f.routes()) @@ -84,6 +86,8 @@ func TestReadiness(t *testing.T) { if rs.StatusCode != test.expectedStatusCode { t.Errorf("expected status code %d, got %d", test.expectedStatusCode, rs.StatusCode) } + + lintMetrics(t, reg) }) } } @@ -119,10 +123,11 @@ func TestSubscriptionsGET(t *testing.T) { t.Run(test.name, func(t *testing.T) { ctrl := gomock.NewController(t) mockDBClient := mocks.NewMockDBClient(ctrl) + reg := prometheus.NewRegistry() f := &Frontend{ dbClient: mockDBClient, - metrics: NewPrometheusEmitter(prometheus.NewRegistry()), + metrics: NewPrometheusEmitter(reg), } // ArmSubscriptionGet and MetricsMiddleware @@ -147,6 +152,8 @@ func TestSubscriptionsGET(t *testing.T) { if rs.StatusCode != test.expectedStatusCode { t.Errorf("expected status code %d, got %d", test.expectedStatusCode, rs.StatusCode) } + + lintMetrics(t, reg) }) } } @@ -238,10 +245,11 @@ func TestSubscriptionsPUT(t *testing.T) { t.Run(test.name, func(t *testing.T) { ctrl := gomock.NewController(t) mockDBClient := mocks.NewMockDBClient(ctrl) + reg := prometheus.NewRegistry() f := &Frontend{ dbClient: mockDBClient, - metrics: NewPrometheusEmitter(prometheus.NewRegistry()), + metrics: NewPrometheusEmitter(reg), } body, err := json.Marshal(&test.subscription) @@ -297,6 +305,21 @@ func TestSubscriptionsPUT(t *testing.T) { if rs.StatusCode != test.expectedStatusCode { t.Errorf("expected status code %d, got %d", test.expectedStatusCode, rs.StatusCode) } + + lintMetrics(t, reg) }) } } + +func lintMetrics(t *testing.T, r prometheus.Gatherer) { + t.Helper() + + problems, err := testutil.GatherAndLint(r) + if err != nil { + t.Fatal(err) + } + + for _, p := range problems { + t.Errorf("metric %q: %s", p.Metric, p.Text) + } +} diff --git a/frontend/pkg/frontend/metrics.go b/frontend/pkg/frontend/metrics.go index c0eaa9d81..4b54587f0 100644 --- a/frontend/pkg/frontend/metrics.go +++ b/frontend/pkg/frontend/metrics.go @@ -5,6 +5,7 @@ package frontend import ( "net/http" + "regexp" "strconv" "sync" "time" @@ -17,7 +18,7 @@ import ( // Emitter emits different types of metrics type Emitter interface { - EmitCounter(metricName string, value float64, labels map[string]string) + AddCounter(metricName string, value float64, labels map[string]string) EmitGauge(metricName string, value float64, labels map[string]string) } @@ -49,7 +50,7 @@ func (pe *PrometheusEmitter) EmitGauge(name string, value float64, labels map[st vec.With(labels).Set(value) } -func (pe *PrometheusEmitter) EmitCounter(name string, value float64, labels map[string]string) { +func (pe *PrometheusEmitter) AddCounter(name string, value float64, labels map[string]string) { pe.mutex.Lock() defer pe.mutex.Unlock() vec, exists := pe.counters[name] @@ -62,6 +63,9 @@ func (pe *PrometheusEmitter) EmitCounter(name string, value float64, labels map[ vec.With(labels).Add(value) } +// patternRe is used to strip the METHOD string from the [ServerMux] pattern string. +var patternRe = regexp.MustCompile(`^[^\s]*\s+`) + type MetricsMiddleware struct { Emitter dbClient database.DBClient @@ -90,9 +94,11 @@ func (mm MetricsMiddleware) Metrics() MiddlewareFunc { next(lrw, r) // Process the request + duration := time.Since(startTime).Seconds() + // Get the route pattern that matched - routePattern := r.URL.Path - duration := time.Since(startTime).Milliseconds() + routePattern := r.Pattern + routePattern = patternRe.ReplaceAllString(routePattern, "") subscriptionState := "Unknown" subscriptionId := r.PathValue(PathSegmentSubscriptionID) @@ -100,13 +106,13 @@ func (mm MetricsMiddleware) Metrics() MiddlewareFunc { sub, err := mm.dbClient.GetSubscriptionDoc(r.Context(), subscriptionId) if err != nil { // If we can't determine the subscription state, we can still expose a metric for subscriptionState "Unknown" - logger.Info("unable to retrieve subscription document for the `frontend_count` metric", "subscriptionId", subscriptionId, "error", err) + logger.Info("unable to retrieve subscription document for the `frontend_requests_total` metric", "subscriptionId", subscriptionId, "error", err) } else { subscriptionState = string(sub.Subscription.State) } } - mm.Emitter.EmitCounter("frontend_count", 1.0, map[string]string{ + mm.Emitter.AddCounter("frontend_requests_total", 1.0, map[string]string{ "verb": r.Method, "api_version": r.URL.Query().Get(APIVersionKey), "code": strconv.Itoa(lrw.statusCode), diff --git a/frontend/pkg/frontend/node_pool_test.go b/frontend/pkg/frontend/node_pool_test.go index 43c3b2b42..f36ab8917 100644 --- a/frontend/pkg/frontend/node_pool_test.go +++ b/frontend/pkg/frontend/node_pool_test.go @@ -90,10 +90,11 @@ func TestCreateNodePool(t *testing.T) { t.Run(test.name, func(t *testing.T) { ctrl := gomock.NewController(t) mockDBClient := mocks.NewMockDBClient(ctrl) + reg := prometheus.NewRegistry() f := &Frontend{ dbClient: mockDBClient, - metrics: NewPrometheusEmitter(prometheus.NewRegistry()), + metrics: NewPrometheusEmitter(reg), clusterServiceClient: &mockCSClient, } hcpCluster := api.NewDefaultHCPOpenShiftCluster() @@ -166,6 +167,8 @@ func TestCreateNodePool(t *testing.T) { if rs.StatusCode != test.expectedStatusCode { t.Errorf("expected status code %d, got %d", test.expectedStatusCode, rs.StatusCode) } + + lintMetrics(t, reg) }) } }