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.

Sign up for GitHub

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

Jump to bottom

testutil.GatherAndCompare is error-prone #1351

Open
machine424 opened this issue Sep 26, 2023 · 3 comments · May be fixed by #1424
Open

testutil.GatherAndCompare is error-prone #1351

machine424 opened this issue Sep 26, 2023 · 3 comments · May be fixed by #1424
Labels
bug help wanted

Comments

@machine424
Copy link

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
Copy link
Author

cc @dgrisonnet @wojtek-t

@machine424 machine424 mentioned this issue Sep 26, 2023
Merged
@dgrisonnet
Copy link

cc @kakkoyun @bwplotka

Could you please provide some insights here?

@bwplotka
Copy link
Member

bwplotka commented Oct 10, 2023 •
edited

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!

@leonnicolas leonnicolas linked a pull request Jan 4, 2024 that will close this issue
Open
leonnicolas added a commit to leonnicolas/client_golang that referenced this issue Jan 4, 2024
@leonnicolas
testutil compareMetricFamilies: make less error-prone
0381a25 
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 added a commit to leonnicolas/client_golang that referenced this issue Jan 4, 2024
@leonnicolas
testutil compareMetricFamilies: make less error-prone
7163ac9 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

testutil compareMetricFamilies: make less error-prone leonnicolas/client_golang
3 participants
@bwplotka @dgrisonnet @machine424