testutil.GatherAndCompare is error-prone

[machine424]

After #1143, the function testutil.GatherAndCompare:

client_golang/prometheus/testutil/testutil.go

Lines 197 to 203 in 1bae6c1

	// GatherAndCompare gathers all metrics from the provided Gatherer and compares
	// 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.
	func GatherAndCompare(g prometheus.Gatherer, expected io.Reader, metricNames ...string) error {
	return TransactionalGatherAndCompare(prometheus.ToTransactionalGatherer(g), expected, metricNames...)
	}

client_golang/prometheus/testutil/testutil.go

Lines 205 to 222 in 1bae6c1

	// TransactionalGatherAndCompare gathers all metrics from the provided Gatherer and compares
	// 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.
	func TransactionalGatherAndCompare(g prometheus.TransactionalGatherer, expected io.Reader, metricNames ...string) error {
	got, done, err := g.Gather()
	defer done()
	if err != nil {
	return fmt.Errorf("gathering metrics failed: %w", err)
	}
	wanted, err := convertReaderToMetricFamily(expected)
	if err != nil {
	return err
	}
	return compareMetricFamilies(got, wanted, metricNames...)
	}

client_golang/prometheus/testutil/testutil.go

Lines 236 to 245 in 1bae6c1

	// compareMetricFamilies would compare 2 slices of metric families, and optionally filters both of
	// them to the `metricNames` provided.
	func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...string) error {
	if metricNames != nil {
	got = filterMetrics(got, metricNames)
	expected = filterMetrics(expected, metricNames)
	}
	return compare(got, expected)
	}

became error-prone, as we filter the expected metrics in compareMetricFamilies as well now, if we make a mistake in one of the arguments expected or metricNames, the function will no longer return an error.

If e.g. we make a mistake in the name of the metric (provide a metric that is not exported because of a typo or re-using a variable containing the wrong metric name), the function will return nil.

Or e.g. if we provide an expected with the wrong metrics to a faulty code, we could never be aware of that.

I think expected lost its safeguard/source of truth utility.

I expect the user/tester to have full control on expected, I don't see why one couldn't provide the exact series they expect.

Plus having "dynamic" expected makes the tests hard to grasp and debug. No one needs tests with too many logic/intelligence that needs to be tested as well :) Way beneficial when they are more explicit even if this result in a little bit more code.

Looking at the code that was justifying the change:

https://github.com/fluxninja/aperture/blob/416d9ceaff9c1b2c4c238210c7c067dcf0a65567/pkg/otelcollector/metricsprocessor/processor_test.go#L148-L168

it's still not making use of the change yet, but I think they can easily split their expected and pass it as a map or sth to their test function...

Discovered this while adding a test for kubernetes etcd metrics: kubernetes/kubernetes#120827. I copied/pasted a test function and the test was passing because I forgot to change the metric name.

[machine424]

cc @dgrisonnet @wojtek-t

[dgrisonnet]

cc @kakkoyun @bwplotka

Could you please provide some insights here?

[bwplotka]

Thanks for this finding. I think both you and #1143 was right. I think you would agree that before #1143 we had broken implementation as both signature and comments of GatherAndCompare, TransactionalGatherAndCompare and compareMetricFamilies suggest that we compare all metrics OR only filtered one by "metricNames" slice if provided.

What's broken now is that the filtered metric names should be PRESENT and expected.

Acceptance Criteria:

• If metricNames > 0 AND one of those metric is NOT present in "got" (or expected optionally) error should be returned.
• Clean up tests - all ScrapeAndCompare, GatherAndCompare etc are scattered and duplicated. Let's focus to test once each functionality and use table tests for different cases.

Help wanted!

[bwplotka]

Fix for this was reverted: https://github.com/prometheus/client_golang/releases/tag/v1.20.5, so reopening. We plan to deliver long term fix in #1639