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

Do not target services with port that does not exist #458

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Do not target services with port that does not exist #458

merged 2 commits into from
Nov 27, 2023

Conversation

kirecek
Copy link
Contributor

@kirecek kirecek commented Nov 13, 2023

When prometheus.io/port annotation is set, no matter what, it just creates the target according the annotation value.

This potentially could make unnecessary calls to non-exposed port that end up with errors on every scrape tick.

... context deadline exceeded (Client.Timeout exceeded while awaiting headers) ...

This can be easily validated so this PR adds check for desired port and throws a single error if port does not exist.

Note: It's user error, so correct solution is to get rid of annotation, however, annotation might be static in third party packages therefore unnecessary overrides are needed till it's get fixed. In each case, this cloud help to have cleaner logs when user error occurs.

@kirecek kirecek requested a review from a team November 13, 2023 13:57
@CLAassistant
Copy link

CLAassistant commented Nov 13, 2023

CLA assistant check
All committers have signed the CLA.

@sigilioso
Copy link
Contributor

Hi @kirecek, Thanks for your contribution!

One of the existing tests is failing, could you take a look at it?

You can execute tests locally using:

$ make test

Thanks!

@kirecek
Copy link
Contributor Author

kirecek commented Nov 17, 2023

oh, sure thing. Sorry, didn't notice it because I executed only newly added test 🤦🏻

Should be fixed now.

make test
=== prometheus === [ test ]: running unit tests...
?   	github.com/newrelic/nri-prometheus/cmd/k8s-target-retriever	[no test files]
ok  	github.com/newrelic/nri-prometheus/cmd/nri-prometheus	(cached)
ok  	github.com/newrelic/nri-prometheus/internal/cmd/scraper	(cached)
ok  	github.com/newrelic/nri-prometheus/internal/integration	(cached)
?   	github.com/newrelic/nri-prometheus/internal/retry	[no test files]
?   	github.com/newrelic/nri-prometheus/load-test/mockexporter	[no test files]
ok  	github.com/newrelic/nri-prometheus/internal/pkg/endpoints	(cached)
ok  	github.com/newrelic/nri-prometheus/internal/pkg/labels	(cached)
ok  	github.com/newrelic/nri-prometheus/internal/pkg/prometheus	(cached)

Thank you :)

@sigilioso
Copy link
Contributor

Sorry the delay. There was an issue with the repository pipelines that is already fixed, could you rebase with main to bring the latest changes? Thanks!

@kirecek
Copy link
Contributor Author

kirecek commented Nov 22, 2023

It's totally fine.

Branch is rebased. Thank you!

@sigilioso sigilioso merged commit b43276e into newrelic:main Nov 27, 2023
8 checks passed
@kirecek kirecek deleted the skip-services-with-non-existing-port branch November 27, 2023 15:08
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.

3 participants