-
Notifications
You must be signed in to change notification settings - Fork 82
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: Add metric port to pods and service #1199
fix: Add metric port to pods and service #1199
Conversation
This allows the usage of Prometheus PodMonitors and/or ServiceMonitors to configure scraping of metrics. This is often preferred over the annotations on the pods. This is also more inline with the rest of Knative. - Add metric port definition to both gateway pods and controller pods. - Add metric port to controller service NOTE: Did not add metric port to gateway services as those often are exposed and exposing the metrics via those services is probably not desired in most cases.
Welcome @arsenetar! It looks like this is your first PR to knative-extensions/net-kourier 🎉 |
Hi @arsenetar. Thanks for your PR. I'm waiting for a knative-extensions member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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
/approve
/hold for @skonto
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arsenetar, ReToCode The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks, I think this makes sense.
I think that's a good idea. Any input @skonto? |
/ok-to-test |
Hi all sorry for the late reply, out of curiosity I would like to take a look at what is exposed. One long standing concern with all metric ports is what info they leak see for example: knative/serving#8959. Downstream we fix that with kube-rbac-proxy at the moment. |
@skonto I don't disagree with the potential for exposed data. If we don't add the port to the controller service then this has no impact vs existing. For the pods they already have to old annotations for Prometheus, the change to those only allows the pods to be used with the PodMonitors as well which is the more common approach. It doesn't change the potential exposure at the pod level. |
/unhold |
Changes
This allows the usage of Prometheus PodMonitors and/or ServiceMonitors to configure scraping of metrics. This is often preferred over the annotations on the pods. This is also more inline with the rest of Knative.
NOTE: Did not add metric port to gateway services as those often are exposed and exposing the metrics via those services is probably not desired in most cases.
/kind enhancement
Release Note
Docs
NONE
Additionally ServiceMonitors and PodMonitors should probably be provided somewhere, maybe over in https://github.com/knative-extensions/monitoring with the rest for base Knative?