Skip to content

Commit

Permalink
frontend: lint Prometheus metrics
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
simonpasquier committed Jan 20, 2025
1 parent 19ddb5a commit f70259e
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 10 deletions.
29 changes: 26 additions & 3 deletions frontend/pkg/frontend/frontend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
Expand All @@ -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)
})
}
}
Expand Down Expand Up @@ -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
Expand All @@ -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)
})
}
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
}
18 changes: 12 additions & 6 deletions frontend/pkg/frontend/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package frontend

import (
"net/http"
"regexp"
"strconv"
"sync"
"time"
Expand All @@ -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)
}

Expand Down Expand Up @@ -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]
Expand All @@ -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
Expand Down Expand Up @@ -90,23 +94,25 @@ 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)
if subscriptionId != "" {
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),
Expand Down
5 changes: 4 additions & 1 deletion frontend/pkg/frontend/node_pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
})
}
}
Expand Down

0 comments on commit f70259e

Please sign in to comment.