metrics: remove invalid routes deletion

When validation is enabled and a new invalid route is created, it gets rejected and a metric is emitted. But when the route is later fixed, the old metric is not removed. Once the route is fixed, there is no error, so the corrected route no longer matches the old metric key.

Author: vlktna

metrics/all_kind.go

@@ -136,11 +136,6 @@ func (a *All) SetInvalidRoute(routeId, reason string) {
 	a.codaHale.SetInvalidRoute(routeId, reason)
 }
 
-func (a *All) DeleteInvalidRoute(routeId string) {
-	a.prometheus.DeleteInvalidRoute(routeId)
-	a.codaHale.DeleteInvalidRoute(routeId)
-}
-
 func (a *All) Close() {
 	a.codaHale.Close()
 	a.prometheus.Close()

metrics/codahale.go

@@ -6,7 +6,6 @@ import (
 	"net/http"
 	"path"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/rcrowley/go-metrics"
@@ -45,15 +44,13 @@ const (
 
 // CodaHale is the CodaHale format backend, implements Metrics interface in DropWizard's CodaHale metrics format.
 type CodaHale struct {
-	reg             metrics.Registry
-	createTimer     func() metrics.Timer
-	createCounter   func() metrics.Counter
-	createGauge     func() metrics.GaugeFloat64
-	options         Options
-	handler         http.Handler
-	quit            chan struct{}
-	invalidRoutesMu sync.RWMutex
-	invalidRoutes   map[string]string // routeId -> metric key
+	reg           metrics.Registry
+	createTimer   func() metrics.Timer
+	createCounter func() metrics.Counter
+	createGauge   func() metrics.GaugeFloat64
+	options       Options
+	handler       http.Handler
+	quit          chan struct{}
 }
 
 // NewCodaHale returns a new CodaHale backend of metrics.
@@ -64,7 +61,6 @@ func NewCodaHale(o Options) *CodaHale {
 
 	c.quit = make(chan struct{})
 	c.reg = metrics.NewRegistry()
-	c.invalidRoutes = make(map[string]string)
 
 	var createSample func() metrics.Sample
 	if o.UseExpDecaySample {
@@ -270,24 +266,10 @@ func (c *CodaHale) IncErrorsStreaming(routeId string) {
 }
 
 func (c *CodaHale) SetInvalidRoute(routeId, reason string) {
-	c.invalidRoutesMu.Lock()
-	defer c.invalidRoutesMu.Unlock()
-
 	key := fmt.Sprintf(KeyInvalidRoutes, routeId, reason)
-	c.invalidRoutes[routeId] = key
 	c.UpdateGauge(key, 1)
 }
 
-func (c *CodaHale) DeleteInvalidRoute(routeId string) {
-	c.invalidRoutesMu.Lock()
-	defer c.invalidRoutesMu.Unlock()
-
-	if key, exists := c.invalidRoutes[routeId]; exists {
-		c.UpdateGauge(key, 0)
-		delete(c.invalidRoutes, routeId)
-	}
-}
-
 func (c *CodaHale) Close() {
 	close(c.quit)
 }

metrics/metrics.go

@@ -81,7 +81,6 @@ type Metrics interface {
 	RegisterHandler(path string, handler *http.ServeMux)
 	UpdateGauge(key string, value float64)
 	SetInvalidRoute(routeId, reason string)
-	DeleteInvalidRoute(routeId string)
 	Close()
 }
 

metrics/metricstest/metricsmock.go

@@ -5,7 +5,6 @@ package metricstest
 import (
 	"fmt"
 	"net/http"
-	"strings"
 	"sync"
 	"time"
 )
@@ -230,15 +229,4 @@ func (m *MockMetrics) SetInvalidRoute(routeID, reason string) {
 	m.UpdateGauge(key, 1)
 }
 
-func (m *MockMetrics) DeleteInvalidRoute(routeID string) {
-	m.WithGauges(func(gauges map[string]float64) {
-		prefix := fmt.Sprintf("route.invalid.%s.", routeID)
-		for key := range gauges {
-			if strings.HasPrefix(key, prefix) {
-				gauges[key] = 0
-			}
-		}
-	})
-}
-
 func (*MockMetrics) Close() {}

metrics/metricstest/metricsmock_test.go

@@ -165,7 +165,7 @@ func TestMockMetrics(t *testing.T) {
 		}
 	})
 
-	t.Run("test-measure-backend-request-header", func(t *testing.T) {
+	t.Run("test-set-invalid-route", func(t *testing.T) {
 		routeID := "my-route"
 		reason := "foo"
 		key := fmt.Sprintf("route.invalid.%s..%s", routeID, reason)
@@ -177,13 +177,6 @@ func TestMockMetrics(t *testing.T) {
 		} else if f != 1 {
 			t.Fatalf("Failed to get the right value after inc: %0.2f", f)
 		}
-
-		m.DeleteInvalidRoute(routeID)
-		if f, ok := m.gauges[key]; !ok {
-			t.Fatalf("Failed to find value %q for routeID %q", key, routeID)
-		} else if f != 0 {
-			t.Fatalf("Failed to get the right value after inc: %0.2f", f)
-		}
 	})
 
 	t.Run("test-gauge", func(t *testing.T) {

metrics/prometheus.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"net/http"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
@@ -74,10 +73,6 @@ type Prometheus struct {
 	customGaugeM               *prometheus.GaugeVec
 	invalidRouteM              *prometheus.GaugeVec
 
-	// Track invalid route metrics for proper cleanup
-	invalidRoutesMu    sync.RWMutex
-	invalidRouteLabels map[string][]prometheus.Labels // routeId -> []labels
-
 	opts     Options
 	registry *prometheus.Registry
 	handler  http.Handler
@@ -88,9 +83,8 @@ func NewPrometheus(opts Options) *Prometheus {
 	opts = applyCompatibilityDefaults(opts)
 
 	p := &Prometheus{
-		registry:           opts.PrometheusRegistry,
-		opts:               opts,
-		invalidRouteLabels: make(map[string][]prometheus.Labels),
+		registry: opts.PrometheusRegistry,
+		opts:     opts,
 	}
 
 	if p.registry == nil {
@@ -537,25 +531,6 @@ func (p *Prometheus) IncErrorsStreaming(routeID string) {
 // SetInvalidRoute satisfies Metrics interface.
 func (p *Prometheus) SetInvalidRoute(routeId, reason string) {
 	p.invalidRouteM.WithLabelValues(routeId, reason).Set(1)
-
-	p.invalidRoutesMu.Lock()
-	defer p.invalidRoutesMu.Unlock()
-
-	labels := prometheus.Labels{"route_id": routeId, "reason": reason}
-	p.invalidRouteLabels[routeId] = append(p.invalidRouteLabels[routeId], labels)
-}
-
-// DeleteInvalidRoute satisfies Metrics interface.
-func (p *Prometheus) DeleteInvalidRoute(routeId string) {
-	p.invalidRoutesMu.Lock()
-	defer p.invalidRoutesMu.Unlock()
-
-	if labels, exists := p.invalidRouteLabels[routeId]; exists {
-		for _, labelSet := range labels {
-			p.invalidRouteM.With(labelSet).Set(0)
-		}
-		delete(p.invalidRouteLabels, routeId)
-	}
 }
 
 func (p *Prometheus) Close() {}

routing/datasource.go

@@ -475,7 +475,7 @@ func processTreePredicates(r *Route, predicateList []*eskip.Predicate) error {
 // ValidateRoute processes a route definition for the routing table.
 // This function is exported to be used by validation webhooks.
 func ValidateRoute(o *Options, def *eskip.Route) (*Route, error) {
-	return processRouteDef(o, mapPredicates(o.Predicates), def)
+	return processRouteDef(o, mapPredicates(o.Predicates), def) //
 }
 
 // processes a route definition for the routing table
@@ -525,9 +525,6 @@ func processRouteDefs(o *Options, defs []*eskip.Route) (routes []*Route, invalid
 		route, err := processRouteDef(o, cpm, def)
 		if err == nil {
 			routes = append(routes, route)
-			if o.Metrics != nil {
-				o.Metrics.DeleteInvalidRoute(def.Id)
-			}
 		} else {
 			invalidDefs = append(invalidDefs, def)
 			o.Log.Errorf("failed to process route %s: %v", def.Id, err)

routing/datasource_test.go

@@ -594,21 +594,21 @@ func waitForIndividualRouteMetrics(t *testing.T, metrics *metricstest.MockMetric
 	}
 }
 
-func TestRouteMetricsSetToZeroWhenFixed(t *testing.T) {
+func TestRouteMetricsStaySetWhenRouteIsFixed(t *testing.T) {
 	t.Run("MockMetrics", func(t *testing.T) {
 		synctest.Test(t, func(t *testing.T) {
-			testRouteMetricsSetToZeroWhenFixedWithMock(t)
+			testRouteMetricsStaySetWhenRouteIsFixedWithMock(t)
 		})
 	})
 
 	t.Run("Prometheus", func(t *testing.T) {
 		synctest.Test(t, func(t *testing.T) {
-			testRouteMetricsSetToZeroWhenFixedWithPrometheus(t)
+			testRouteMetricsStaySetWhenRouteIsFixedWithPrometheus(t)
 		})
 	})
 }
 
-func testRouteMetricsSetToZeroWhenFixedWithMock(t *testing.T) {
+func testRouteMetricsStaySetWhenRouteIsFixedWithMock(t *testing.T) {
 	metrics := &metricstest.MockMetrics{}
 
 	// Start with an invalid route
@@ -649,16 +649,16 @@ func testRouteMetricsSetToZeroWhenFixedWithMock(t *testing.T) {
 	// Wait for the update to be processed
 	time.Sleep(50 * time.Millisecond)
 
-	// Verify the metric is now set to 0 (not deleted)
+	// Verify the metric remains set (not deleted or zeroed out)
 	metrics.WithGauges(func(gauges map[string]float64) {
 		expectedKey := "route.invalid.invalidBackend1..failed_backend_split"
-		if gauges[expectedKey] != 0 {
-			t.Errorf("Expected invalid route metric to be set to 0 when route becomes valid, got %f", gauges[expectedKey])
+		if gauges[expectedKey] != 1 {
+			t.Errorf("Expected invalid route metric to stay set when route becomes valid, got %f", gauges[expectedKey])
 		}
 	})
 }
 
-func testRouteMetricsSetToZeroWhenFixedWithPrometheus(t *testing.T) {
+func testRouteMetricsStaySetWhenRouteIsFixedWithPrometheus(t *testing.T) {
 	pm := metrics.NewPrometheus(metrics.Options{})
 	path := "/metrics"
 
@@ -708,16 +708,16 @@ func testRouteMetricsSetToZeroWhenFixedWithPrometheus(t *testing.T) {
 	// Wait for the update to be processed
 	time.Sleep(50 * time.Millisecond)
 
-	// Check that the metric is now set to 0
+	// Check that the metric stays set (not deleted or zeroed out)
 	req = httptest.NewRequest("GET", path, nil)
 	w = httptest.NewRecorder()
 	mux.ServeHTTP(w, req)
 	body, _ = io.ReadAll(w.Result().Body)
 	output = string(body)
 
-	expectedZeroLine := `skipper_route_invalid{reason="failed_backend_split",route_id="invalidBackend1"} 0`
-	if !strings.Contains(output, expectedZeroLine) {
-		t.Errorf("Expected to find invalid route metric set to 0 when route becomes valid")
+	expectedLine = `skipper_route_invalid{reason="failed_backend_split",route_id="invalidBackend1"} 1`
+	if !strings.Contains(output, expectedLine) {
+		t.Errorf("Expected invalid route metric to remain set when route becomes valid")
 		t.Logf("Output: %s", output)
 	}
 }

routing/errors.go

@@ -30,7 +30,6 @@ func WrapInvalidDefinitionReason(reason string, err error) error {
 
 func HandleValidationError(mtr metrics.Metrics, err error, routeId string) error {
 	if err == nil {
-		mtr.DeleteInvalidRoute(routeId)
 		return nil
 	}
 

routing/errors_test.go

@@ -2,6 +2,7 @@ package routing
 
 import (
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 
@@ -87,10 +88,9 @@ func TestValidationErrorsNoError(t *testing.T) {
 	}
 
 	mtr.WithGauges(func(g map[string]float64) {
-		for k, v := range g {
-			if strings.HasPrefix(k, "route.invalid") && v > 0 {
-				t.Fatalf("Failed to have no invalid routes %q -> %0.2f", k, v)
-			}
+		key := fmt.Sprintf("route.invalid.%s..%s", routeID, errInvalidPredicateParams)
+		if v := g[key]; v != 1 {
+			t.Fatalf("Invalid route metric should remain set, got %0.2f", v)
 		}
 	})
 }