-
-
Notifications
You must be signed in to change notification settings - Fork 44
feat: metrics support for llmaz #316
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
Conversation
|
/kind feature |
|
/hold |
chart/values.yaml
Outdated
| port: 8443 | ||
| protocol: TCP | ||
| targetPort: https | ||
| targetPort: 8080 |
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 can not change the values.yaml directly, it's auto generated which means it will be overwritten once we run make helm-package.
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.
revert.
ab25576 to
8d45fd8
Compare
1ebb10b to
3a01d1c
Compare
Makefile
Outdated
| $(ENVTEST): $(LOCALBIN) | ||
| test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest | ||
|
|
||
| .PHONY: prometheus |
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 wonder if it would be possible to support make prometheus here
| @@ -1,4 +1,7 @@ | |||
| {{- if .Values.prometheus.enable -}} | |||
| {{- if .Values.prometheus.enable }} | |||
| {{- if not (.Capabilities.APIVersions.Has "monitoring.coreos.com/v1/ServiceMonitor") }} | |||
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.
some docs: https://helm.sh/docs/chart_template_guide/builtin_objects/
to check ServiceMonitor resourecs
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.
So if we haven't install the prometheus-operator, it will report error here, right? This is useful.
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.
yes, test in local(without install the prometheus-operator)
root@VM-0-5-ubuntu:/home/ubuntu/llmaz# vi chart/values.global.yaml
root@VM-0-5-ubuntu:/home/ubuntu/llmaz# helm upgrade --install llmaz ./chart -f ./chart/values.global.yaml --namespace=llmaz-system
Release "llmaz" does not exist. Installing it now.
Error: execution error at (llmaz/templates/prometheus/serviceaccount.yaml:3:4): The cluster does not support the required API resource `monitoring.coreos.com/v1/ServiceMonitor`.
root@VM-0-5-ubuntu:/home/ubuntu/llmaz#|
Hi @googs1025 I updated some configs, ptal. Documentation is still missing. |
b4ed03e to
8cd8d55
Compare
Thanks for your help! I'll test it locally to double check. Also, what kind of docs do we need? |
|
Like how to install the prometheus operator, step to step to view the metrics. |
e4fb804 to
cf74d08
Compare
chart/templates/deployment.yaml
Outdated
| periodSeconds: 20 | ||
| name: manager | ||
| ports: | ||
| - containerPort: 8080 |
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.
use make helm-package
Generate the missing port part 🤔
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.
it's ok then. we just avoid to update the charts manually. This is because we added the metric patch in kustomize.
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.
so do I need to revert ? or just add this is ok?
We can directly see the docs which is more direct https://github.com/googs1025/llmaz/tree/fix/metrics_bind/docs/prometheus-operator |
7b6c78e to
aa7d5ac
Compare
|
Sorry for the late reply, I will test tonight. |
|
I tried this yesterday, but my network is broken, I'll try this later. Once I have a result, I'll comment here. |
|
please rebase then I'll merge this pr. |
|
See the last commit for fixes, generally the label selector not match. |
|
encounter bug with |
|
Install manually, it works great. Could you rebase then let's merge this PR, the helm chart problem I'll solve it in #343 |
a4646a3 to
be54420
Compare
|
/unhold |
Signed-off-by: googs1025 <[email protected]>
Signed-off-by: kerthcet <[email protected]>
Signed-off-by: kerthcet <[email protected]>
Signed-off-by: kerthcet <[email protected]>
be54420 to
1358a66
Compare
|
/unhold |
|
/lgtm |


What this PR does / why we need it
Which issue(s) this PR fixes
Fixes part of #307
Special notes for your reviewer
Does this PR introduce a user-facing change?