testutil compareMetricFamilies: make less error-prone #1424

leonnicolas · 2024-01-04T11:28:50Z

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: #1351

Signed-off-by: leonnicolas leonloechner@gmx.de

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: prometheus#1351 Signed-off-by: leonnicolas <leonloechner@gmx.de>

leonnicolas · 2024-01-04T11:41:07Z

prometheus/testutil/testutil.go

@@ -254,6 +254,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) {


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.

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?

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.

any feedback @bwplotka ?

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.

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

right, error should be enough

leonnicolas · 2024-01-04T11:46:26Z

According to Contributing.md I have to address one of the maintainers: @bwplotka

I also tried using table tests as you suggested in #1351, but I found it a bit messy for the test cases where the scraped server is not running or returns a bad request, so I left those tests as there where.

In to see which metric names are missing, we can add them to the error message. Signed-off-by: leonnicolas <leonloechner@gmx.de>

bwplotka

Nice, added suggestions, otherwise LGTM!

bwplotka · 2024-02-05T12:05:45Z

prometheus/testutil/testutil.go

+		if len(metricNames) > len(got) {
+			h := make(map[string]struct{})
+			for _, mf := range got {
+				if mf == nil {


got element can be really nil here?

prometheus/testutil/testutil.go

bwplotka · 2024-02-05T12:13:07Z

prometheus/testutil/testutil.go

@@ -254,6 +254,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) {


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.

bwplotka · 2024-02-05T12:13:38Z

prometheus/testutil/testutil_test.go

@@ -332,27 +332,46 @@ Diff:
 }

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


prometheus/testutil/testutil_test.go

- remove if nil check - use two nested loops instead of map - use new function `hasMetricByName` for readability Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: leonnicolas <60091705+leonnicolas@users.noreply.github.com>

leonnicolas · 2024-02-07T18:03:39Z

@bwplotka thanks for your review. I hope I addressed all you comments

leonnicolas · 2024-03-04T08:50:33Z

@bwplotka anything missing from my side to move on with this PR?

bwplotka

Thanks! Looks nice, just one comment, otherwise LGTM

bwplotka · 2024-05-09T11:45:54Z

prometheus/testutil/testutil_test.go

+		// expectedErrPrefix if empty, means no fail is expected for the comparison.
+		expectedErrPrefix string


Why prefix and not exact error? Would be nice to test that missing metric semantics.

leonnicolas force-pushed the testutil-compare-metrics-family branch from 0381a25 to 7163ac9 Compare January 4, 2024 11:31

leonnicolas commented Jan 4, 2024

View reviewed changes

Add missing metricNames to error

d4090e9

In to see which metric names are missing, we can add them to the error message. Signed-off-by: leonnicolas <leonloechner@gmx.de>

leonnicolas force-pushed the testutil-compare-metrics-family branch from 276aea0 to d4090e9 Compare January 8, 2024 17:03

leonnicolas requested a review from bwplotka January 11, 2024 10:11

bwplotka reviewed Feb 5, 2024

View reviewed changes

leonnicolas force-pushed the testutil-compare-metrics-family branch from 6085d5d to 847a7bb Compare February 7, 2024 17:55

Apply suggestions from code review

437f988

- remove if nil check - use two nested loops instead of map - use new function `hasMetricByName` for readability Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: leonnicolas <60091705+leonnicolas@users.noreply.github.com>

leonnicolas force-pushed the testutil-compare-metrics-family branch from 847a7bb to 437f988 Compare February 7, 2024 18:01

leonnicolas requested a review from bwplotka March 18, 2024 15:21

bwplotka approved these changes May 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testutil compareMetricFamilies: make less error-prone #1424

testutil compareMetricFamilies: make less error-prone #1424

leonnicolas commented Jan 4, 2024

leonnicolas Jan 4, 2024 •

edited

bwplotka Jan 8, 2024

leonnicolas Jan 8, 2024

leonnicolas Feb 5, 2024

bwplotka Feb 5, 2024

leonnicolas Feb 7, 2024

leonnicolas commented Jan 4, 2024

bwplotka left a comment

bwplotka Feb 5, 2024

bwplotka Feb 5, 2024

bwplotka Feb 5, 2024

leonnicolas commented Feb 7, 2024

leonnicolas commented Mar 4, 2024

bwplotka left a comment

bwplotka May 9, 2024

		// expectedErrPrefix if empty, means no fail is expected for the comparison.
		expectedErrPrefix string

testutil compareMetricFamilies: make less error-prone #1424

Are you sure you want to change the base?

testutil compareMetricFamilies: make less error-prone #1424

Conversation

leonnicolas commented Jan 4, 2024

leonnicolas Jan 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonnicolas commented Jan 4, 2024

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonnicolas commented Feb 7, 2024

leonnicolas commented Mar 4, 2024

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonnicolas Jan 4, 2024 •

edited