From 584a7ce3d935e4fdca7b893f5f741d59f3289140 Mon Sep 17 00:00:00 2001 From: bwplotka Date: Mon, 14 Oct 2024 21:52:04 +0100 Subject: [PATCH 1/2] Revert "testutil compareMetricFamilies: make less error-prone (#1424)" This reverts commit c7c7509669eaf604d0fa117579c6fc8b09b17210. --- prometheus/testutil/testutil.go | 18 ---- prometheus/testutil/testutil_test.go | 136 +++++++++------------------ 2 files changed, 46 insertions(+), 108 deletions(-) diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go index e0ac34666..fb1d51f98 100644 --- a/prometheus/testutil/testutil.go +++ b/prometheus/testutil/testutil.go @@ -277,15 +277,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 +318,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 -} diff --git a/prometheus/testutil/testutil_test.go b/prometheus/testutil/testutil_test.go index ec835a69e..06e367744 100644 --- a/prometheus/testutil/testutil_test.go +++ b/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) { From 504ad9bf5c6419449d2cacf8cf8855bfdcfcfc18 Mon Sep 17 00:00:00 2001 From: bwplotka Date: Mon, 14 Oct 2024 21:29:01 +0100 Subject: [PATCH 2/2] Cut 1.20.5; update comments. Signed-off-by: bwplotka # Conflicts: # CHANGELOG.md --- CHANGELOG.md | 8 +++++++- VERSION | 2 +- prometheus/testutil/testutil.go | 12 ++++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f390ed809..654a8d039 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/VERSION b/VERSION index 769e37e15..7bf9455f0 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.20.2 +1.20.5 diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go index fb1d51f98..6f1200180 100644 --- a/prometheus/testutil/testutil.go +++ b/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()