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.

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

Refactor tests for slm collector #928

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

sysadmind
Copy link
Contributor

  • Remove up, totalScrapes, and jsonParseFailures metrics. They are not useful.
  • Move fixtures to individual files
  • Base tests on the metric output for better testing the expected output instead of the internals.

- Remove up, totalScrapes, and jsonParseFailures metrics. They are not useful.
- Move fixtures to individual files
- Base tests on the metric output for better testing the expected output instead of the internals.

Signed-off-by: Joe Adams <[email protected]>
@sysadmind sysadmind added this to the Test Cleanup milestone Sep 7, 2024
@SuperQ
Copy link
Contributor

SuperQ commented Sep 7, 2024

I think the reason we have the up metric is that the Collect() doesn't have any error return. So we can't pass back a failed scrape signal to the output. Without this, the exporter will silently fail, which is not good for the end user.

I think we need a bit more refactor here, so that error is returned and we fail the scrape with a 5xx error instead of returning partial data.

@sysadmind
Copy link
Contributor Author

I agree that knowing the collector as a whole or an individual collector was successful is useful. This is provided by the collector/Collector going forward: https://github.com/prometheus-community/elasticsearch_exporter/blob/master/collector/collector.go#L57

This particular change is just to update the testing before refactoring the slm collector to work with the Collector interface which will reintroduce the ability to determine if the collector scrape was successful. There have been a handful of similar PRs in this light. See #813

The follow up PR will will refactor the code to use the Collector interface. i.e. #789

I have been breaking up the refactor from the testing changes to make PRs easier to review and to make sure the refactors don't meaningfully change the metrics. If you would prefer a different strategy that is easier to review or more to your preference, let me know.

@sysadmind sysadmind merged commit d98d2f6 into prometheus-community:master Sep 13, 2024
4 checks passed
@sysadmind sysadmind self-assigned this Sep 13, 2024
@sysadmind sysadmind deleted the refactor-tests-slm branch September 13, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants