Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testutil compareMetricFamilies: make less error-prone #1424

Merged
merged 4 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions prometheus/testutil/testutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,15 @@ func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...str
if metricNames != nil {
got = filterMetrics(got, metricNames)
expected = filterMetrics(expected, metricNames)
if len(metricNames) > len(got) {
Copy link
Contributor Author

@leonnicolas leonnicolas Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking to use something like slices, to find the metricName that is not present in got. Would the additional package (slices is not yet in the std lib in go 1.19) and extra complexity of this function be outweighed by the improved usability? You would be able to see the misspelled or missing metricName in the error text.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I would either wait for older versions than 1.19 to be dropped from support or quickly write a small function finding those. I don't think at the end using std lib slices extension is that needed. It might be just two loops. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit. Finding the difference in 2 slices seems so easy but always requires so much code.

I added the "resulting" slice to the error with %v. I wonder if we should somehow allow callers of compareMetricFamilies to extract the missing metric Names? I guess that would require an API change :(
It is just awkward to test if the "correct" metric names are in the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any feedback @bwplotka ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why exact metric names would be useful? Isn't it enough to return error?

compareMetricFamilies has a clear semantics - we are just ensuring the error is more readable and comparison true.

I think small code complexity is fine here - proposed below what can be done.

Copy link

@pckushan pckushan Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it better to check the actual metrics names are included in the given metricsNames? The reason is if we have a test table including several tests and say it has 3 metics but sometimes it may trigger only two for a specific test scenario. In that execution we check this comparison passing all three metrics names but with this change it breaks the flow from this point if len(metricNames) > len(got) {
cc: @bwplotka @leonnicolas

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if len(metricNames) < len(got) it is ok to give an error, but if other way around need to check whether got names are matched with given names.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks Mimir tests when upgrading to v1.20.4, although I'm not sure exactly why yet.

Copy link

@aknuds1 aknuds1 Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can see it breaks Mimir tests because they expect to be able to compare with a greater metrics set than what is present. After this change, that results in an error. Maybe it makes sense? Not quite sure yet.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks mongodb_exporter as well, we test there if metric doesn't appear

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)
Expand Down Expand Up @@ -318,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
}
136 changes: 90 additions & 46 deletions prometheus/testutil/testutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,27 +328,46 @@ func TestMetricNotFound(t *testing.T) {
}

func TestScrapeAndCompare(t *testing.T) {
const expected = `
scenarios := map[string]struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

want string
metricNames []string
// expectedErr if empty, means no fail is expected for the comparison.
expectedErr string
}{
"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

Expand All @@ -358,53 +377,78 @@ func TestScrapeAndCompareWithMultipleExpected(t *testing.T) {
# TYPE some_total2 counter

some_total2{ label2 = "value2" } 1
`

expectedReader := strings.NewReader(expected)
`,
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

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"},
expectedErr: "expected metric name(s) not found: [some_total1 some_total3]",
},
}
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.expectedErr == "" || err.Error() != scenario.expectedErr {
t.Errorf("unexpected error happened: %s", err)
}
} else if scenario.expectedErr != "" {
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) {
Expand Down
Loading