-
Notifications
You must be signed in to change notification settings - Fork 47
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
fix(endpoints): scrape endpoints and not services #148
Conversation
c948c40
to
9a8bc48
Compare
Currently after merging this we will be sending more data than before. We should decide to filter out some targets, es: func (k *KubernetesTargetRetriever) GetTargets() ([]Target, error) {
length := 0
k.targets.Range(func(_, _ interface{}) bool {
length++
return true
})
targets := make([]Target, 0, length)
k.targets.Range(func(_, y interface{}) bool {
for _, t := range y.([]Target) {
if t.Object.Kind == "service" && excludeServiceTargets {
continue
}
if t.Object.Kind == "endpoints" && excludeEndpointsTargets {
continue
}
targets = append(targets, t)
}
return true
})
return targets, nil
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to have these behind a feature flag keeping the old behavior by default. Or have a mayor release. I would also propose to have this feature in beta for some time.
Could you try the load test to se how is the result with the new feature?
@gsanchezgavier I added two feature flags and given what we talked about offline in the past days, for now the option to scrape endpoints will be disabled by default. I'll update the documentation and the HelmChart to further clarify this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
After merging and releasing we need to update documentation and expose the new config through the helm charts |
3ede2b9
to
b3b9156
Compare
Fix #27
The idea is that when a service is detected the underlying endpoints are fetched. For backward compatibility the metric directly coming from the service is kept
Two new config options have been added
ScrapeServices (default true)
andScrapeEndpoints(default false)
After merging we should update documentation and the helm chart to ask for listing/watching permission on endpoints