-
Notifications
You must be signed in to change notification settings - Fork 820
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
Expose containing port of serving metrics #4877
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4877 +/- ##
==========================================
+ Coverage 53.05% 53.07% +0.01%
==========================================
Files 250 251 +1
Lines 20396 20389 -7
==========================================
- Hits 10822 10821 -1
+ Misses 8855 8854 -1
+ Partials 719 714 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/assign @chaosi-zju |
Hi! @wangxf1987 Glad to see your contribution! The CI of this PR failed due to it wasn't signed off, usually please use Detail guideline can refer to: https://probot.github.io/apps/dco/ |
…erviceMonitor Signed-off-by: wangxiaofei67 <[email protected]>
2b9e3cc
to
a4d9be4
Compare
done |
/lgtm |
Can you write the release-notes? @wangxf1987 |
|
@wangxf1987 Here. In your PR description |
done |
Is this ServiceMonitor and PodMonitor? By the way, this PR focuses on exposing ports in the helm chart template, what about operators and cli? Should we keep them consistent? |
Let me talk about my understanding of this PR: First, our
we can try as: $ kubectl --context karmada-host exec -it karmada-controller-manager-cdbbf75dd-69fxb -n karmada-system -- sh
/ # curl http://127.0.0.1:8080/metrics
.....
work_sync_workload_duration_seconds_bucket{result="success",le="0.512"} 4
work_sync_workload_duration_seconds_bucket{result="success",le="1.024"} 4
work_sync_workload_duration_seconds_bucket{result="success",le="2.048"} 4
work_sync_workload_duration_seconds_bucket{result="success",le="+Inf"} 4
work_sync_workload_duration_seconds_sum{result="success"} 0.075089723
work_sync_workload_duration_seconds_count{result="success"} 4 even without this PR, we can still access $ kubectl --context karmada-host exec -it karmada-descheduler-7d654cbff6-5jmrt -n karmada-system -- sh
# here, `10.244.0.12` is the pod IP of `karmada-controller-manager`.
/ # curl http://10.244.0.12:8080/metrics
.....
work_sync_workload_duration_seconds_bucket{result="success",le="0.512"} 4
work_sync_workload_duration_seconds_bucket{result="success",le="1.024"} 4
work_sync_workload_duration_seconds_bucket{result="success",le="2.048"} 4
work_sync_workload_duration_seconds_bucket{result="success",le="+Inf"} 4
work_sync_workload_duration_seconds_sum{result="success"} 0.075089723
work_sync_workload_duration_seconds_count{result="success"} 4 So, the normal operation of the However, @wangxf1987 has needs to use take apiVersion: monitoring.coreos.com/v1
kind: PodMonitor
metadata:
name: example-app
labels:
team: frontend
spec:
selector:
matchLabels:
app: example-app
podMetricsEndpoints:
- port: web In practice, the Pay attention to the official API recommends to use So, here comes the problem, we didn't specify the this is the reason for why this PR proposed. Hi @wangxf1987, am I right? |
If we all agree on the necessity of this PR, then we should build an issue to make up for the corresponding modifications of other installation methods. |
Expose this port for PodMonitor only |
Yes, the por name needs to be specified in PM, so the metrices port name needs to be defined in the contaienr. |
I think we also need a new PR describing how to monitor the search component. Similarly, you need to modify the port name in the search service. ---
apiVersion: v1
kind: Service
metadata:
name: {{ $name }}-search
namespace: {{ include "karmada.namespace" . }}
labels:
{{- include "karmada.search.labels" . | nindent 4 }}
spec:
ports:
- name: https # add this
port: 443
protocol: TCP
targetPort: 443
selector:
{{- include "karmada.search.labels" . | nindent 4 }} |
when the port is defined by default, it it used by prothumes. like this. |
|
It looks really fantastic! 👍 Did you do this monitoring through the existing karmada document or create it through your personal way? By the way, I actually very much hope you can share the complete operation steps of your build monitoring in the form of documents ٩(๑❛ᴗ❛๑)۶. If you also willing to contribute a document to karmada, you can refer to this doc. |
@chaosi-zju Would you like to run a test as per this document, and share it with us at a community meeting? |
OK |
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.
/assign
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.
This PR focuses on exposing the metrics endpoint for karmada-agent
, karmada-controller-manager
and karmada-scheduler
.
What about the other components? I suppose all components' configuration should be synchronized.
@@ -60,6 +60,10 @@ spec: | |||
initialDelaySeconds: 15 | |||
periodSeconds: 15 | |||
timeoutSeconds: 5 | |||
ports: | |||
- containerPort: 10351 | |||
name: http |
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.
name: http | |
name: metrics |
Why not name it with metrics
?
/retitle Expose containing port of serving metrics |
…erviceMonitor
What type of PR is this?
This is improve for installation, the port of karmada-apiserver is exposed, but other componment is not.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: