fixed/metrics/prometheus: correctly handle prefix (#178)

This patches fixes a bug where the sanitization function of the prometheus metric manager was not correctly handling the given path if it was containing a '_prefix'. This patch fixes this issue and adds tests. The parameter 'url' of the interface MetricManager.MeasureRequest has also been renamed to 'path' for better usage clarity.
PaloAltoNetworks · Mar 30, 2023 · a0e0fb6 · a0e0fb6
1 parent ba5e229
commit a0e0fb6
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 14 deletions.
diff --git a/gateway/gateway_test.go b/gateway/gateway_test.go
@@ -55,7 +55,7 @@ type fakeMetricManager struct {
 	unregisterTCPConnectionCalled int64
 }
 
-func (m *fakeMetricManager) MeasureRequest(method string, url string) bahamut.FinishMeasurementFunc {
+func (m *fakeMetricManager) MeasureRequest(method string, path string) bahamut.FinishMeasurementFunc {
 	return func(code int, span opentracing.Span) time.Duration { return 0 }
 }
 

diff --git a/health_server_test.go b/health_server_test.go
@@ -37,7 +37,7 @@ func freePort() (port int) {
 // A MetricsManager handles Prometheus Metrics Management
 type testMetricsManager struct{}
 
-func (m *testMetricsManager) MeasureRequest(method string, url string) FinishMeasurementFunc {
+func (m *testMetricsManager) MeasureRequest(method string, path string) FinishMeasurementFunc {
 	return nil
 }
 func (m *testMetricsManager) RegisterWSConnection()    {}

diff --git a/metrics.go b/metrics.go
@@ -23,7 +23,7 @@ type FinishMeasurementFunc func(code int, span opentracing.Span) time.Duration
 
 // A MetricsManager handles Prometheus Metrics Management
 type MetricsManager interface {
-	MeasureRequest(method string, url string) FinishMeasurementFunc
+	MeasureRequest(method string, path string) FinishMeasurementFunc
 	RegisterWSConnection()
 	UnregisterWSConnection()
 	RegisterTCPConnection()

diff --git a/metrics_prometheus.go b/metrics_prometheus.go
@@ -23,20 +23,31 @@ import (
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 )
 
-var vregexp = regexp.MustCompile(`^/v/\d+`)
+var (
+	pregexp = regexp.MustCompile(`^/_[a-zA-Z0-9-_+]+`)
+	vregexp = regexp.MustCompile(`^/v/\d+`)
+)
 
-func sanitizeURL(url string) string {
+func sanitizePath(url string) string {
 
+	prefix := "/"
+	matches := pregexp.FindAllString(url, 1)
+	if len(matches) == 1 {
+		prefix = matches[0] + "/"
+		url = strings.TrimPrefix(url, matches[0])
+	}
 	url = vregexp.ReplaceAllString(url, "")
+	url = strings.TrimPrefix(url, "/")
 
 	parts := strings.Split(url, "/")
-	if len(parts) <= 2 {
-		return url
+
+	if len(parts) <= 1 {
+		return prefix + url
 	}
 
-	parts[2] = ":id"
+	parts[1] = ":id"
 
-	return strings.Join(parts, "/")
+	return prefix + strings.Join(parts, "/")
 }
 
 type prometheusMetricsManager struct {
@@ -119,9 +130,9 @@ func newPrometheusMetricsManager(registerer prometheus.Registerer) MetricsManage
 	return mc
 }
 
-func (c *prometheusMetricsManager) MeasureRequest(method string, url string) FinishMeasurementFunc {
+func (c *prometheusMetricsManager) MeasureRequest(method string, path string) FinishMeasurementFunc {
 
-	surl := sanitizeURL(url)
+	surl := sanitizePath(path)
 
 	timer := prometheus.NewTimer(
 		prometheus.ObserverFunc(

diff --git a/metrics_prometheus_test.go b/metrics_prometheus_test.go
@@ -69,10 +69,53 @@ func Test_sanitizeURL(t *testing.T) {
 			},
 			"/toto/:id/titi",
 		},
+
+		{
+			"test /_prefix/toto",
+			args{
+				"/_prefix/toto",
+			},
+			"/_prefix/toto",
+		},
+		{
+			"test /_prefix/v/1/toto",
+			args{
+				"/_prefix/v/1/toto",
+			},
+			"/_prefix/toto",
+		},
+		{
+			"test /_prefix/toto/xxxxxxx",
+			args{
+				"/_prefix/toto/xxxxxxx",
+			},
+			"/_prefix/toto/:id",
+		},
+		{
+			"test /_prefix/v/1/toto/xxxxxxx",
+			args{
+				"/_prefix/v/1/toto/xxxxxxx",
+			},
+			"/_prefix/toto/:id",
+		},
+		{
+			"test /_prefix/toto/xxxxxxx/titi",
+			args{
+				"/_prefix/toto/xxxxxxx/titi",
+			},
+			"/_prefix/toto/:id/titi",
+		},
+		{
+			"test /_prefix/v/1/toto/xxxxxxx/titi",
+			args{
+				"/_prefix/v/1/toto/xxxxxxx/titi",
+			},
+			"/_prefix/toto/:id/titi",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := sanitizeURL(tt.args.url); got != tt.want {
+			if got := sanitizePath(tt.args.url); got != tt.want {
 				t.Errorf("sanitizeURL() = %v, want %v", got, tt.want)
 			}
 		})
@@ -104,7 +147,7 @@ func TestMeasureRequest(t *testing.T) {
 
 		Convey("When I call measure a 502 request", func() {
 
-			f := pmm.MeasureRequest("GET", "http://toto.com/id/toto")
+			f := pmm.MeasureRequest("GET", "/toto/id")
 			f(502, nil)
 
 			data, _ := r.Gather()
@@ -114,7 +157,7 @@ func TestMeasureRequest(t *testing.T) {
 				So(data[0].GetMetric()[0].Label[0].String(), ShouldEqual, `name:"code" value:"502" `)
 				So(data[0].GetMetric()[0].Label[1].String(), ShouldEqual, `name:"method" value:"GET" `)
 				So(data[0].GetMetric()[0].Label[2].String(), ShouldEqual, `name:"trace" value:"unknown" `)
-				So(data[0].GetMetric()[0].Label[3].String(), ShouldEqual, `name:"url" value:"http://:id/id/toto" `)
+				So(data[0].GetMetric()[0].Label[3].String(), ShouldEqual, `name:"url" value:"/toto/:id" `)
 			})
 		})
 	})