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 [email protected]

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.

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

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.

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

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.

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

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.

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

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.

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 <[email protected]>

In to see which metric names are missing, we can add them to the error message. Signed-off-by: leonnicolas <[email protected]>

- remove if nil check - use two nested loops instead of map - use new function `hasMetricByName` for readability Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: leonnicolas <[email protected]>

Before we would only compare the error prefix in `TestScrapeAndCompare`. Signed-off-by: leonnicolas <[email protected]>

bwplotka

Thanks! ❤️

* 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: prometheus#1351 Signed-off-by: leonnicolas <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Signed-off-by: leonnicolas <[email protected]> * prometheus/testutil/testutil_test.go: compare complete error Before we would only compare the error prefix in `TestScrapeAndCompare`. Signed-off-by: leonnicolas <[email protected]> --------- Signed-off-by: leonnicolas <[email protected]> Signed-off-by: leonnicolas <[email protected]> Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: Steven Swartz <[email protected]>

…sure the metric with the provided metricName is found. We were depending on it not erroring. This PR removes that assumption and instead makes sure the metric does no longer existi using the CollectAndCount function. Signed-off-by: Tim Ramlot <[email protected]>

Follow-up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

Follow up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

Follow-up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

Follow up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

Have a function that can emulate the old GatherAndCompare Follow up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

Have a function that can emulate the old GatherAndCompare Follow up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

bwplotka · 2024-10-06T12:00:54Z

FYI as a learning: This broke dozens if not hundreds of downstream users.

Feedback welcome of how we could improve those kind of changes in the future. This was a potentially "buggy" behaviour of the testing helper (surprising users). However, many users might have not care perhaps and we should just have create another function with the desired logic? 🤔

logicalhan · 2024-10-10T16:44:51Z

I think we should revert this. Prometheus should not be breaking a bunch of downstream tests with changes like this.

logicalhan · 2024-10-10T16:46:02Z

FYI as a learning: This broke dozens if not hundreds of downstream users.

Feedback welcome of how we could improve those kind of changes in the future. This was a potentially "buggy" behaviour of the testing helper (surprising users). However, many users might have not care perhaps and we should just have create another function with the desired logic? 🤔

You can deprecate the existing function and introduce a new function which has the behavior you want. This is NOT the way to change an existing API that hundreds of people are likely using and depending on.

This reverts commit c7c7509.

bwplotka · 2024-10-14T21:03:53Z

Thanks for your feedback @logicalhan.

Digression; are all breaking changes bad?

You can deprecate the existing function and introduce a new function which has the behavior you want. This is NOT the way to change an existing API that hundreds of people are likely using and depending on.

Is it that simple? What if the existing users were not using the API as they should (or abuse it). e.g. why they are filtering by metric names which they don't expect to exist? What if this new behaviour might save you or your project from a critical production bug or major annoyance in the future? What if this breaking change is easily noticeable, only in testing code and might trigger a positive changes around Kubernetes tests.

Interesting enough this breaking change was proposed and noticed by our Kubernetes community. Only thanks to @machine424 due diligence we noticed this and avoid regression (or at least noop test).

Finally, your suggestion would work normally, but in this case it would mean literally all functions in the testutil would be duplicated only to make the test check... actually do its job better.

As a result the proposal to revert the fix and leave the main functions error prone.. might be not rationally better (:

Proposal

However, I spoke to a few people (e.g. @dashpole @dgrisonnet @kakkoyun @ArthurSens @vesari and your opinion), and it feels it might be easier to revert.

Created PR -- added rationales there.

I propose we revert this, and work on a long term solution. With the team we also discussed a way, where every client_golang release pipeline could propose an upgrade PR to a Kubernetes library that heavily depends on client_golang and check if the CI passes (compatibility check with a big user).

Thanks!

bwplotka · 2024-10-15T10:06:49Z

This PR was reverted: https://github.com/prometheus/client_golang/releases/tag/v1.20.5

…heus#1424)" This reverts commit c7c7509. Signed-off-by: Shivanth <[email protected]>

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

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 2 times, most recently 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

leonnicolas and others added 4 commits May 15, 2024 11:12

Add missing metricNames to error

f0133c0

In to see which metric names are missing, we can add them to the error message. Signed-off-by: leonnicolas <[email protected]>

Apply suggestions from code review

9490310

- remove if nil check - use two nested loops instead of map - use new function `hasMetricByName` for readability Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: leonnicolas <[email protected]>

prometheus/testutil/testutil_test.go: compare complete error

0e3f5c6

Before we would only compare the error prefix in `TestScrapeAndCompare`. Signed-off-by: leonnicolas <[email protected]>

leonnicolas force-pushed the testutil-compare-metrics-family branch from 437f988 to 0e3f5c6 Compare May 15, 2024 09:12

bwplotka approved these changes May 15, 2024

View reviewed changes

bwplotka merged commit c7c7509 into prometheus:main May 15, 2024
9 checks passed

mfaizanse mentioned this pull request Aug 19, 2024

chore: bump dependencies kyma-project/eventing-publisher-proxy#150

Merged

krajorama added a commit to grafana/mimir that referenced this pull request Aug 30, 2024

Fix test asserts

fa0d03b

Follow-up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

krajorama mentioned this pull request Aug 30, 2024

update mimir-prometheus at grafana/mimir-prometheus@53cf3fb grafana/mimir#9148

Merged

krajorama added a commit to grafana/mimir that referenced this pull request Aug 30, 2024

More test assert fixes

102125c

Follow up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

krajorama added a commit to grafana/mimir that referenced this pull request Aug 30, 2024

More test asserts fixed

b48666f

Follow-up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

krajorama added a commit to grafana/mimir that referenced this pull request Aug 30, 2024

Have a function that can emulate old GatherAndCompare

e9c7162

Follow up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

krajorama added a commit to grafana/mimir that referenced this pull request Aug 30, 2024

Fix test asserts

3186aa9

Have a function that can emulate the old GatherAndCompare Follow up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

harry671003 mentioned this pull request Sep 4, 2024

Chore: Update Thanos cortexproject/cortex#6189

Merged

3 tasks

krajorama added a commit to grafana/mimir that referenced this pull request Sep 6, 2024

Fix test asserts

44c5c37

Have a function that can emulate the old GatherAndCompare Follow up to prometheus/client_golang#1424 Signed-off-by: György Krajcsovits <[email protected]>

jkh52 mentioned this pull request Oct 4, 2024

Bump the gomod-dependencies group across 1 directory with 7 updates kubernetes-sigs/apiserver-network-proxy#660

Closed

bwplotka mentioned this pull request Oct 8, 2024

testutil: Better registry state assertion framework/API #1639

Open

dashpole mentioned this pull request Oct 8, 2024

Update prometheus client golang kubernetes/kubernetes#127941

Closed

bwplotka added a commit that referenced this pull request Oct 14, 2024

Revert "testutil compareMetricFamilies: make less error-prone (#1424)"

f3d6c5d

This reverts commit c7c7509.

bwplotka added a commit that referenced this pull request Oct 14, 2024

Revert "testutil compareMetricFamilies: make less error-prone (#1424)"

584a7ce

This reverts commit c7c7509.

bwplotka mentioned this pull request Oct 15, 2024

Merge release 1.20 to main #1647

Merged

mjsmithnh mentioned this pull request Oct 16, 2024

[v1.16.1] sync with upstream gravitational/cert-manager#36

Closed

shivanthzen pushed a commit to shivanthzen/client_golang that referenced this pull request Nov 3, 2024

Revert "testutil compareMetricFamilies: make less error-prone (promet…

4fce733

…heus#1424)" This reverts commit c7c7509. Signed-off-by: Shivanth <[email protected]>

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

Loading

bwplotka Jan 8, 2024

leonnicolas Jan 8, 2024

leonnicolas Feb 5, 2024

bwplotka Feb 5, 2024

pckushan Sep 5, 2024 •

edited

Loading

pckushan Sep 5, 2024

aknuds1 Oct 6, 2024

aknuds1 Oct 6, 2024 •

edited

Loading

BupycHuk Oct 13, 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

leonnicolas May 15, 2024

bwplotka left a comment

bwplotka commented Oct 6, 2024

logicalhan commented Oct 10, 2024

logicalhan commented Oct 10, 2024

bwplotka commented Oct 14, 2024

bwplotka commented Oct 15, 2024

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

testutil compareMetricFamilies: make less error-prone #1424

testutil compareMetricFamilies: make less error-prone #1424

Conversation

leonnicolas commented Jan 4, 2024

leonnicolas Jan 4, 2024 • edited Loading

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

pckushan Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Oct 6, 2024 • edited Loading

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

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Oct 6, 2024

logicalhan commented Oct 10, 2024

logicalhan commented Oct 10, 2024

bwplotka commented Oct 14, 2024

Digression; are all breaking changes bad?

Proposal

bwplotka commented Oct 15, 2024

leonnicolas Jan 4, 2024 •

edited

Loading

pckushan Sep 5, 2024 •

edited

Loading

aknuds1 Oct 6, 2024 •

edited

Loading