-
Notifications
You must be signed in to change notification settings - Fork 590
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
Make eventing components observable #1722
Make eventing components observable #1722
Conversation
- Enable scrapping of eventing-controller - Add eventing reconciler dasshboard
Hi @sayanh. Thanks for your PR. I'm waiting for a knative 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. |
@@ -17,6 +17,608 @@ | |||
# --patch "$(cat 100-grafana-dash-eventing.yaml)" -n knative-monitoring | |||
--- | |||
data: | |||
knative-eventing-reconciler-dashboard.json: |+ |
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.
The eventing controllers will show up in the Knative Serving Controller
dashboards from my testing.
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.
True that. I removed this dashboard completely. We'll use Knative Serving - Reconciler
dashboard but I've changed the name to Knative - Reconciler
PR. And of course, we'll take all of the monitoring components out of serving as in here. By the way, where can I find Knative Serving Controller
dashboard? I just see the following dashboards.
rm tmp.prometheus-scrape-kn-eventing.yaml | ||
rm tmp.prometheus-scrape-config.yaml | ||
``` | ||
|
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 you are going to add a new scrap job eventing-controller
to Prometheus scrap configuration. But the instructions look very complicated. Do you want to try "kubectl patch configmap prometheus-scrape-config ..." ?
An example is that how we add the eventing dashboard to the configmap grafana-dashboard-definition-knative
:
kubectl patch configmap grafana-dashboard-definition-knative --patch "$(cat config/monitoring/metrics/grafana/100-grafana-dash-eventing.yaml)" -n knative-monitoring
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.
@daisy-ycguo The problem here is the scrapping config needs to be a part of the same file(key) in the configmap and we can't patch like the example you posted. Hence this complicated way of doing things.
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.
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 this is just a workaround until we get the prometheus operator in. So I'm fine with it.
@sayanh: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/ok-to-test |
pkg/reconciler/broker/broker_test.go
Outdated
@@ -283,7 +283,7 @@ func TestReconcileCRD(t *testing.T) { | |||
WantCreates: []runtime.Object{ | |||
NewService(filterServiceName, testNS, | |||
WithServiceOwnerReferences(ownerReferences()), | |||
WithServiceAnnotations(resources.FilterAnnotations()), | |||
WithServiceAnnotations(map[string]string{}), |
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 this call to WithServiceAnnotations can be removed
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.
fixed
pkg/reconciler/broker/broker_test.go
Outdated
@@ -459,7 +459,7 @@ func TestReconcileCRD(t *testing.T) { | |||
WantCreates: []runtime.Object{ | |||
NewService(ingressServiceName, testNS, | |||
WithServiceOwnerReferences(ownerReferences()), | |||
WithServiceAnnotations(resources.IngressAnnotations()), | |||
WithServiceAnnotations(map[string]string{}), |
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.
same here
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.
fixed
@capri-xiyue this will fix the infra issue you were having. |
/lgtm |
@n3wscott for approval |
/approve Thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: n3wscott, sayanh 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 |
* Make eventing controller observable - Enable scrapping of eventing-controller - Add eventing reconciler dasshboard * Retain vendor license * Removed eventing reconciler dashboard * Make broker filter and ingress scrapeable * Removed hard coded annotation for prometheus - Refer prometheus-operator/kube-prometheus#16 * Made sources-controller observable * Remove sources metrics service * Fixed as per PR comments
…t-proxy-some-headers Do not proxy some headers from reply
Fix the following components and make them scrapable:
For the following sources, I'll create a separate PR:
See #1693
Related PR in Serving.
Proposed Changes
Release Note