Merge pull request #1645 from prometheus/cut-1204-pr1424

undefined

Author: bwplotka

CHANGELOG.md

@@ -1,5 +1,11 @@
 ## Unreleased
 
+## 1.20.5 / 2024-10-15
+
+* [BUGFIX] testutil: Reverted #1424; functions using compareMetricFamilies are (again) only failing if filtered metricNames are in the expected input.
+
+## 1.20.4 / 2024-09-07
+
 * [BUGFIX] histograms: Fix possible data race when appending exemplars vs metrics gather. #1623
 
 ## 1.20.3 / 2024-09-05
@@ -28,7 +34,7 @@
 * [FEATURE] promlint: Add duplicated metric lint rule. #1472
 * [BUGFIX] promlint: Relax metric type in name linter rule. #1455
 * [BUGFIX] promhttp: Make sure server instrumentation wrapping supports new and future extra responseWriter methods. #1480
-* [BUGFIX] testutil: Functions using compareMetricFamilies are now failing if filtered metricNames are not in the input. #1424
+* [BUGFIX] **breaking** testutil: Functions using compareMetricFamilies are now failing if filtered metricNames are not in the input. #1424 (reverted in 1.20.5)
 
 ## 1.19.0 / 2024-02-27
 

VERSION

@@ -1 +1 @@
-1.20.2
+1.20.5

prometheus/testutil/testutil.go

@@ -158,6 +158,9 @@ func GatherAndCount(g prometheus.Gatherer, metricNames ...string) (int, error) {
 // ScrapeAndCompare calls a remote exporter's endpoint which is expected to return some metrics in
 // plain text format. Then it compares it with the results that the `expected` would return.
 // If the `metricNames` is not empty it would filter the comparison only to the given metric names.
+//
+// NOTE: Be mindful of accidental discrepancies between expected and metricNames; metricNames filter
+// both expected and scraped metrics. See https://github.com/prometheus/client_golang/issues/1351.
 func ScrapeAndCompare(url string, expected io.Reader, metricNames ...string) error {
 	resp, err := http.Get(url)
 	if err != nil {
@@ -185,6 +188,9 @@ func ScrapeAndCompare(url string, expected io.Reader, metricNames ...string) err
 
 // CollectAndCompare collects the metrics identified by `metricNames` and compares them in the Prometheus text
 // exposition format to the data read from expected.
+//
+// NOTE: Be mindful of accidental discrepancies between expected and metricNames; metricNames filter
+// both expected and collected metrics. See https://github.com/prometheus/client_golang/issues/1351.
 func CollectAndCompare(c prometheus.Collector, expected io.Reader, metricNames ...string) error {
 	reg := prometheus.NewPedanticRegistry()
 	if err := reg.Register(c); err != nil {
@@ -197,6 +203,9 @@ func CollectAndCompare(c prometheus.Collector, expected io.Reader, metricNames .
 // it to an expected output read from the provided Reader in the Prometheus text
 // exposition format. If any metricNames are provided, only metrics with those
 // names are compared.
+//
+// NOTE: Be mindful of accidental discrepancies between expected and metricNames; metricNames filter
+// both expected and gathered metrics. See https://github.com/prometheus/client_golang/issues/1351.
 func GatherAndCompare(g prometheus.Gatherer, expected io.Reader, metricNames ...string) error {
 	return TransactionalGatherAndCompare(prometheus.ToTransactionalGatherer(g), expected, metricNames...)
 }
@@ -205,6 +214,9 @@ func GatherAndCompare(g prometheus.Gatherer, expected io.Reader, metricNames ...
 // it to an expected output read from the provided Reader in the Prometheus text
 // exposition format. If any metricNames are provided, only metrics with those
 // names are compared.
+//
+// NOTE: Be mindful of accidental discrepancies between expected and metricNames; metricNames filter
+// both expected and gathered metrics. See https://github.com/prometheus/client_golang/issues/1351.
 func TransactionalGatherAndCompare(g prometheus.TransactionalGatherer, expected io.Reader, metricNames ...string) error {
 	got, done, err := g.Gather()
 	defer done()
@@ -277,15 +289,6 @@ func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...str
 	if metricNames != nil {
 		got = filterMetrics(got, metricNames)
 		expected = filterMetrics(expected, metricNames)
-		if len(metricNames) > len(got) {
-			var missingMetricNames []string
-			for _, name := range metricNames {
-				if ok := hasMetricByName(got, name); !ok {
-					missingMetricNames = append(missingMetricNames, name)
-				}
-			}
-			return fmt.Errorf("expected metric name(s) not found: %v", missingMetricNames)
-		}
 	}
 
 	return compare(got, expected)
@@ -327,12 +330,3 @@ func filterMetrics(metrics []*dto.MetricFamily, names []string) []*dto.MetricFam
 	}
 	return filtered
 }
-
-func hasMetricByName(metrics []*dto.MetricFamily, name string) bool {
-	for _, mf := range metrics {
-		if mf.GetName() == name {
-			return true
-		}
-	}
-	return false
-}

prometheus/testutil/testutil_test.go

@@ -328,46 +328,27 @@ func TestMetricNotFound(t *testing.T) {
 }
 
 func TestScrapeAndCompare(t *testing.T) {
-	scenarios := map[string]struct {
-		want        string
-		metricNames []string
-		// expectedErr if empty, means no fail is expected for the comparison.
-		expectedErr string
-	}{
-		"empty metric Names": {
-			want: `
+	const expected = `
 		# HELP some_total A value that represents a counter.
 		# TYPE some_total counter
 
 		some_total{ label1 = "value1" } 1
-	`,
-			metricNames: []string{},
-		},
-		"one metric": {
-			want: `
-		# HELP some_total A value that represents a counter.
-		# TYPE some_total counter
+	`
 
-		some_total{ label1 = "value1" } 1
-	`,
-			metricNames: []string{"some_total"},
-		},
-		"multiple expected": {
-			want: `
-		# HELP some_total A value that represents a counter.
-		# TYPE some_total counter
+	expectedReader := strings.NewReader(expected)
 
-		some_total{ label1 = "value1" } 1
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprintln(w, expected)
+	}))
+	defer ts.Close()
 
-		# HELP some_total2 A value that represents a counter.
-		# TYPE some_total2 counter
+	if err := ScrapeAndCompare(ts.URL, expectedReader, "some_total"); err != nil {
+		t.Errorf("unexpected scraping result:\n%s", err)
+	}
+}
 
-		some_total2{ label2 = "value2" } 1
-	`,
-			metricNames: []string{"some_total2"},
-		},
-		"expected metric name is not scraped": {
-			want: `
+func TestScrapeAndCompareWithMultipleExpected(t *testing.T) {
+	const expected = `
 		# HELP some_total A value that represents a counter.
 		# TYPE some_total counter
 
@@ -377,78 +358,53 @@ func TestScrapeAndCompare(t *testing.T) {
 		# TYPE some_total2 counter
 
 		some_total2{ label2 = "value2" } 1
-	`,
-			metricNames: []string{"some_total3"},
-			expectedErr: "expected metric name(s) not found: [some_total3]",
-		},
-		"one of multiple expected metric names is not scraped": {
-			want: `
-		# HELP some_total A value that represents a counter.
-		# TYPE some_total counter
+	`
 
-		some_total{ label1 = "value1" } 1
+	expectedReader := strings.NewReader(expected)
 
-		# HELP some_total2 A value that represents a counter.
-		# TYPE some_total2 counter
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprintln(w, expected)
+	}))
+	defer ts.Close()
 
-		some_total2{ label2 = "value2" } 1
-	`,
-			metricNames: []string{"some_total1", "some_total3"},
-			expectedErr: "expected metric name(s) not found: [some_total1 some_total3]",
-		},
-	}
-	for name, scenario := range scenarios {
-		t.Run(name, func(t *testing.T) {
-			expectedReader := strings.NewReader(scenario.want)
-
-			ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				fmt.Fprintln(w, scenario.want)
-			}))
-			defer ts.Close()
-			if err := ScrapeAndCompare(ts.URL, expectedReader, scenario.metricNames...); err != nil {
-				if scenario.expectedErr == "" || err.Error() != scenario.expectedErr {
-					t.Errorf("unexpected error happened: %s", err)
-				}
-			} else if scenario.expectedErr != "" {
-				t.Errorf("expected an error but got nil")
-			}
-		})
+	if err := ScrapeAndCompare(ts.URL, expectedReader, "some_total2"); err != nil {
+		t.Errorf("unexpected scraping result:\n%s", err)
 	}
+}
 
-	t.Run("fetching fail", func(t *testing.T) {
-		err := ScrapeAndCompare("some_url", strings.NewReader("some expectation"), "some_total")
-		if err == nil {
-			t.Errorf("expected an error but got nil")
-		}
-		if !strings.HasPrefix(err.Error(), "scraping metrics failed") {
-			t.Errorf("unexpected error happened: %s", err)
-		}
-	})
+func TestScrapeAndCompareFetchingFail(t *testing.T) {
+	err := ScrapeAndCompare("some_url", strings.NewReader("some expectation"), "some_total")
+	if err == nil {
+		t.Errorf("expected an error but got nil")
+	}
+	if !strings.HasPrefix(err.Error(), "scraping metrics failed") {
+		t.Errorf("unexpected error happened: %s", err)
+	}
+}
 
-	t.Run("bad status code", func(t *testing.T) {
-		const expected = `
+func TestScrapeAndCompareBadStatusCode(t *testing.T) {
+	const expected = `
 		# HELP some_total A value that represents a counter.
 		# TYPE some_total counter
 
 		some_total{ label1 = "value1" } 1
 	`
 
-		expectedReader := strings.NewReader(expected)
+	expectedReader := strings.NewReader(expected)
 
-		ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(http.StatusBadGateway)
-			fmt.Fprintln(w, expected)
-		}))
-		defer ts.Close()
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusBadGateway)
+		fmt.Fprintln(w, expected)
+	}))
+	defer ts.Close()
 
-		err := ScrapeAndCompare(ts.URL, expectedReader, "some_total")
-		if err == nil {
-			t.Errorf("expected an error but got nil")
-		}
-		if !strings.HasPrefix(err.Error(), "the scraping target returned a status code other than 200") {
-			t.Errorf("unexpected error happened: %s", err)
-		}
-	})
+	err := ScrapeAndCompare(ts.URL, expectedReader, "some_total")
+	if err == nil {
+		t.Errorf("expected an error but got nil")
+	}
+	if !strings.HasPrefix(err.Error(), "the scraping target returned a status code other than 200") {
+		t.Errorf("unexpected error happened: %s", err)
+	}
 }
 
 func TestCollectAndCount(t *testing.T) {