-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.GatherAndCompare is error-prone #1351
Comments
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 What's broken now is that the filtered metric names should be PRESENT and expected. Acceptance Criteria:
Help wanted! |
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]>
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]>
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]>
* 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: #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]>
* 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]>
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 |
After #1143, the function
testutil.GatherAndCompare
:client_golang/prometheus/testutil/testutil.go
Lines 197 to 203 in 1bae6c1
client_golang/prometheus/testutil/testutil.go
Lines 205 to 222 in 1bae6c1
client_golang/prometheus/testutil/testutil.go
Lines 236 to 245 in 1bae6c1
became error-prone, as we filter the
expected
metrics incompareMetricFamilies
as well now, if we make a mistake in one of the argumentsexpected
ormetricNames
, 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.
The text was updated successfully, but these errors were encountered: