From f447422a9c2efd7c816400ec4d3a5f9790ebda35 Mon Sep 17 00:00:00 2001 From: leonnicolas Date: Thu, 4 Jan 2024 12:17:42 +0100 Subject: [PATCH 1/4] testutil compareMetricFamilies: make less error-prone The functions `GatherAndCompare`, `ScrapeAndCompare` and others that use `compareMetricFamilies` under the hood can return no error if `metricNames` includes none of the names found in the scraped/gathered results. To avoid false Positves (an error being the negative case), we can return an error if there is is at least one name in `metricNames` that is not in the filtered results. Fixes: https://github.com/prometheus/client_golang/issues/1351 Signed-off-by: leonnicolas --- prometheus/testutil/testutil.go | 3 + prometheus/testutil/testutil_test.go | 138 ++++++++++++++++++--------- 2 files changed, 95 insertions(+), 46 deletions(-) diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go index e818c2fc3..c148235f3 100644 --- a/prometheus/testutil/testutil.go +++ b/prometheus/testutil/testutil.go @@ -277,6 +277,9 @@ func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...str if metricNames != nil { got = filterMetrics(got, metricNames) expected = filterMetrics(expected, metricNames) + if len(metricNames) > len(got) { + return fmt.Errorf("expected metrics name not found") + } } return compare(got, expected) diff --git a/prometheus/testutil/testutil_test.go b/prometheus/testutil/testutil_test.go index 06e367744..71ba7771e 100644 --- a/prometheus/testutil/testutil_test.go +++ b/prometheus/testutil/testutil_test.go @@ -328,27 +328,46 @@ func TestMetricNotFound(t *testing.T) { } func TestScrapeAndCompare(t *testing.T) { - const expected = ` + scenarios := map[string]struct { + want string + metricNames []string + errPrefix string + fail bool + }{ + "empty metric Names": { + want: ` # 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 - expectedReader := strings.NewReader(expected) + 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 - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - fmt.Fprintln(w, expected) - })) - defer ts.Close() + some_total{ label1 = "value1" } 1 - if err := ScrapeAndCompare(ts.URL, expectedReader, "some_total"); err != nil { - t.Errorf("unexpected scraping result:\n%s", err) - } -} + # HELP some_total2 A value that represents a counter. + # TYPE some_total2 counter -func TestScrapeAndCompareWithMultipleExpected(t *testing.T) { - const expected = ` + some_total2{ label2 = "value2" } 1 + `, + metricNames: []string{"some_total2"}, + }, + "expected metric name is not scraped": { + want: ` # HELP some_total A value that represents a counter. # TYPE some_total counter @@ -358,53 +377,80 @@ func TestScrapeAndCompareWithMultipleExpected(t *testing.T) { # TYPE some_total2 counter some_total2{ label2 = "value2" } 1 - ` - - expectedReader := strings.NewReader(expected) + `, + metricNames: []string{"some_total3"}, + errPrefix: "expected metrics name not found", + fail: true, + }, + "one of multiple expected metric names is not scraped": { + want: ` + # HELP some_total A value that represents a counter. + # TYPE some_total counter - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - fmt.Fprintln(w, expected) - })) - defer ts.Close() + some_total{ label1 = "value1" } 1 - if err := ScrapeAndCompare(ts.URL, expectedReader, "some_total2"); err != nil { - t.Errorf("unexpected scraping result:\n%s", err) - } -} + # HELP some_total2 A value that represents a counter. + # TYPE some_total2 counter -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") + some_total2{ label2 = "value2" } 1 + `, + metricNames: []string{"some_total1", "some_total3"}, + errPrefix: "expected metrics name not found", + fail: true, + }, } - if !strings.HasPrefix(err.Error(), "scraping metrics failed") { - t.Errorf("unexpected error happened: %s", err) + 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.fail || !strings.HasPrefix(err.Error(), scenario.errPrefix) { + t.Errorf("unexpected error happened: %s", err) + } + } else if scenario.fail { + t.Errorf("expected an error but got nil") + } + }) } -} -func TestScrapeAndCompareBadStatusCode(t *testing.T) { - const expected = ` + 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) + } + }) + + t.Run("bad status code", func(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 f0133c0ee119bf3e8a46ee83f572152fa5488063 Mon Sep 17 00:00:00 2001 From: leonnicolas Date: Mon, 8 Jan 2024 17:59:44 +0100 Subject: [PATCH 2/4] Add missing metricNames to error In to see which metric names are missing, we can add them to the error message. Signed-off-by: leonnicolas --- prometheus/testutil/testutil.go | 15 ++++++++++++++- prometheus/testutil/testutil_test.go | 4 ++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go index c148235f3..c8368c84d 100644 --- a/prometheus/testutil/testutil.go +++ b/prometheus/testutil/testutil.go @@ -278,7 +278,20 @@ func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...str got = filterMetrics(got, metricNames) expected = filterMetrics(expected, metricNames) if len(metricNames) > len(got) { - return fmt.Errorf("expected metrics name not found") + h := make(map[string]struct{}) + for _, mf := range got { + if mf == nil { + continue + } + h[mf.GetName()] = struct{}{} + } + var missingMetricNames []string + for _, name := range metricNames { + if _, ok := h[name]; !ok { + missingMetricNames = append(missingMetricNames, name) + } + } + return fmt.Errorf("expected metric name(s) not found: %v", missingMetricNames) } } diff --git a/prometheus/testutil/testutil_test.go b/prometheus/testutil/testutil_test.go index 71ba7771e..5cb858883 100644 --- a/prometheus/testutil/testutil_test.go +++ b/prometheus/testutil/testutil_test.go @@ -379,7 +379,7 @@ func TestScrapeAndCompare(t *testing.T) { some_total2{ label2 = "value2" } 1 `, metricNames: []string{"some_total3"}, - errPrefix: "expected metrics name not found", + errPrefix: "expected metric name(s) not found", fail: true, }, "one of multiple expected metric names is not scraped": { @@ -395,7 +395,7 @@ func TestScrapeAndCompare(t *testing.T) { some_total2{ label2 = "value2" } 1 `, metricNames: []string{"some_total1", "some_total3"}, - errPrefix: "expected metrics name not found", + errPrefix: "expected metric name(s) not found", fail: true, }, } From 9490310bc2b6162b1f23a8b9000cb3098675425b Mon Sep 17 00:00:00 2001 From: leonnicolas <60091705+leonnicolas@users.noreply.github.com> Date: Wed, 7 Feb 2024 18:46:34 +0100 Subject: [PATCH 3/4] Apply suggestions from code review - remove if nil check - use two nested loops instead of map - use new function `hasMetricByName` for readability Co-authored-by: Bartlomiej Plotka Signed-off-by: leonnicolas <60091705+leonnicolas@users.noreply.github.com> --- prometheus/testutil/testutil.go | 18 ++++++++++-------- prometheus/testutil/testutil_test.go | 18 ++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go index c8368c84d..de7eaf1ad 100644 --- a/prometheus/testutil/testutil.go +++ b/prometheus/testutil/testutil.go @@ -278,16 +278,9 @@ func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...str got = filterMetrics(got, metricNames) expected = filterMetrics(expected, metricNames) if len(metricNames) > len(got) { - h := make(map[string]struct{}) - for _, mf := range got { - if mf == nil { - continue - } - h[mf.GetName()] = struct{}{} - } var missingMetricNames []string for _, name := range metricNames { - if _, ok := h[name]; !ok { + if ok := hasMetricByName(got, name); !ok { missingMetricNames = append(missingMetricNames, name) } } @@ -334,3 +327,12 @@ 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 5cb858883..9ff7daa41 100644 --- a/prometheus/testutil/testutil_test.go +++ b/prometheus/testutil/testutil_test.go @@ -331,8 +331,8 @@ func TestScrapeAndCompare(t *testing.T) { scenarios := map[string]struct { want string metricNames []string - errPrefix string - fail bool + // expectedErrPrefix if empty, means no fail is expected for the comparison. + expectedErrPrefix string }{ "empty metric Names": { want: ` @@ -378,9 +378,8 @@ func TestScrapeAndCompare(t *testing.T) { some_total2{ label2 = "value2" } 1 `, - metricNames: []string{"some_total3"}, - errPrefix: "expected metric name(s) not found", - fail: true, + metricNames: []string{"some_total3"}, + expectedErrPrefix: "expected metric name(s) not found", }, "one of multiple expected metric names is not scraped": { want: ` @@ -394,9 +393,8 @@ func TestScrapeAndCompare(t *testing.T) { some_total2{ label2 = "value2" } 1 `, - metricNames: []string{"some_total1", "some_total3"}, - errPrefix: "expected metric name(s) not found", - fail: true, + metricNames: []string{"some_total1", "some_total3"}, + expectedErrPrefix: "expected metric name(s) not found", }, } for name, scenario := range scenarios { @@ -408,10 +406,10 @@ func TestScrapeAndCompare(t *testing.T) { })) defer ts.Close() if err := ScrapeAndCompare(ts.URL, expectedReader, scenario.metricNames...); err != nil { - if !scenario.fail || !strings.HasPrefix(err.Error(), scenario.errPrefix) { + if scenario.expectedErrPrefix == "" || !strings.HasPrefix(err.Error(), scenario.expectedErrPrefix) { t.Errorf("unexpected error happened: %s", err) } - } else if scenario.fail { + } else if scenario.expectedErrPrefix != "" { t.Errorf("expected an error but got nil") } }) From 0e3f5c6084fd416c96c340e29973187ac82a44c9 Mon Sep 17 00:00:00 2001 From: leonnicolas Date: Wed, 15 May 2024 11:10:58 +0200 Subject: [PATCH 4/4] prometheus/testutil/testutil_test.go: compare complete error Before we would only compare the error prefix in `TestScrapeAndCompare`. Signed-off-by: leonnicolas --- prometheus/testutil/testutil_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/prometheus/testutil/testutil_test.go b/prometheus/testutil/testutil_test.go index 9ff7daa41..ec835a69e 100644 --- a/prometheus/testutil/testutil_test.go +++ b/prometheus/testutil/testutil_test.go @@ -331,8 +331,8 @@ func TestScrapeAndCompare(t *testing.T) { scenarios := map[string]struct { want string metricNames []string - // expectedErrPrefix if empty, means no fail is expected for the comparison. - expectedErrPrefix string + // expectedErr if empty, means no fail is expected for the comparison. + expectedErr string }{ "empty metric Names": { want: ` @@ -378,8 +378,8 @@ func TestScrapeAndCompare(t *testing.T) { some_total2{ label2 = "value2" } 1 `, - metricNames: []string{"some_total3"}, - expectedErrPrefix: "expected metric name(s) not found", + metricNames: []string{"some_total3"}, + expectedErr: "expected metric name(s) not found: [some_total3]", }, "one of multiple expected metric names is not scraped": { want: ` @@ -393,8 +393,8 @@ func TestScrapeAndCompare(t *testing.T) { some_total2{ label2 = "value2" } 1 `, - metricNames: []string{"some_total1", "some_total3"}, - expectedErrPrefix: "expected metric name(s) not found", + metricNames: []string{"some_total1", "some_total3"}, + expectedErr: "expected metric name(s) not found: [some_total1 some_total3]", }, } for name, scenario := range scenarios { @@ -406,10 +406,10 @@ func TestScrapeAndCompare(t *testing.T) { })) defer ts.Close() if err := ScrapeAndCompare(ts.URL, expectedReader, scenario.metricNames...); err != nil { - if scenario.expectedErrPrefix == "" || !strings.HasPrefix(err.Error(), scenario.expectedErrPrefix) { + if scenario.expectedErr == "" || err.Error() != scenario.expectedErr { t.Errorf("unexpected error happened: %s", err) } - } else if scenario.expectedErrPrefix != "" { + } else if scenario.expectedErr != "" { t.Errorf("expected an error but got nil") } })