-
Notifications
You must be signed in to change notification settings - Fork 58
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
📖 (doc): Add a doc as a guidance to help users know how to consume the metrics and integrate it with other solutions #1524
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
bac6ff6
to
3b00bd8
Compare
3b00bd8
to
85f94da
Compare
/hold Just to ensure that we all convey |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1524 +/- ##
=======================================
Coverage 68.24% 68.24%
=======================================
Files 58 58
Lines 4988 4988
=======================================
Hits 3404 3404
Misses 1342 1342
Partials 242 242
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
We should probably be proposing to fix this, if needed, not remove it, since via Prometheus is the primary way people would likely want to monitor metrics for op-con doing its work. |
Hi @bentito I think the best approach might be:
WDYT? |
This comment was marked as resolved.
This comment was marked as resolved.
85f94da
to
23cc069
Compare
Could we move a fixed |
Not opposed, but we need to be careful about how we do this because our service names are not part of our public API and may change at any time. If we were the ones managing the ServiceMonitor, we could make sure that it was updated appropriately if/when we change our service names. By documenting it, we may inadvertently put the service name and namespace in our public API, which IMO requires a MUCH bigger discussion. |
23cc069
to
cbdc3a7
Compare
/hold cancel It seems reasonable to get merged now. |
I adding Because we need to merge
First due the doc added |
/hold cancel |
Hi @azych, Thank you for raising this! Your comment brings up an important point. However, I believe this behavior depends on how Prometheus is configured. For example, it is possible to configure Prometheus to monitor all namespaces, as outlined in the official documentation: Given this flexibility in configuration, I don’t think we should add details about serviceMonitorSelector in this guide. The ServiceMonitor here is just an example, and its primary purpose is to clarify how to pass the serviceName and certificates, rather than enforce a specific Prometheus setup. However, a note to alert users about it was added. PS.: See: https://github.com/prometheus-operator/kube-prometheus/blob/23b33729e2d31660539ca43f5c553907c3f0b823/manifests/prometheus-prometheus.yaml#L48-L49 by default it seems to be configured to check all namespaces. Let me know what you think! |
2a713a8
to
5b50966
Compare
Hi @LalatenduMohanty @tmshort @bentito @michaelryanpeter It is good to review now. Thank you for the help. |
@camilamacedo86 Do you want us to test the manifests and commands are in this PR? |
In the past, I did mainly the same as what is here and added the outputs in the description of the PRs see:
From there, the only change was the port (fixed now)
I can test again now since it is open for a while, and I addressed some comments. |
fd28a8c
to
dc78cdd
Compare
I went through all the steps again since it had been a while, and I needed to make two tweaks. I really appreciate your time and help! Thanks again! |
|
||
```shell | ||
curl -v -k -H "Authorization: Bearer <TOKEN>" \ | ||
https://operator-controller-service.olmv1-system.svc.cluster.local:8443/metrics |
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.
@tylerslaton THANK You for check that it has something wrong
The name of the service was wrong when I copied and pasted the command.
It is fine now as you can check in the description
|
||
```shell | ||
OLM_SECRET=$(kubectl get secret -n olmv1-system -o jsonpath="{.items[*].metadata.name}" | tr ' ' '\n' | grep '^catalogd-service-cert') | ||
echo $OLM_SECRET |
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.
@tylerslaton @LalatenduMohanty
Here as well was not working now it is fine
$ OLM_SECRET=$(kubectl get secret -n olmv1-system -o jsonpath="{.items[*].metadata.name}" | tr ' ' '\n' | grep '^catalogd-service-cert')
camilam@Camilas-MacBook-Pro ~/go/src/github/operator-framework/operator-controller (remove-monitor-prometheues) $ echo $OLM_SECRET
catalogd-service-cert-v1.2.0-rc4-19-g099a6cf
eb14a7c
to
a6a73ec
Compare
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.
If you have questions, please feel free to reach out.
I don't think anything I mentioned is a blocker. Only suggestions to improve clarity and scanability.
0ba1078
to
fe87ffa
Compare
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.
Just a couple of final nits. Otherwise, LGTM.
…metrics and integrate it with other solutions
31fb82f
to
1d0c654
Compare
See the commands in this guidance executed with success
Operator-Controller Metrics
CatalogD Metrics
Prometheus
$ kubectl apply --server-side -f https://github.com/prometheus-operator/prometheus-operator/releases/download/v0.77.1/bundle.yaml
customresourcedefinition.apiextensions.k8s.io/alertmanagerconfigs.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/alertmanagers.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/podmonitors.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/probes.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/prometheusagents.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/prometheuses.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/prometheusrules.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/scrapeconfigs.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/servicemonitors.monitoring.coreos.com serverside-applied
customresourcedefinition.apiextensions.k8s.io/thanosrulers.monitoring.coreos.com serverside-applied
clusterrolebinding.rbac.authorization.k8s.io/prometheus-operator serverside-applied
clusterrole.rbac.authorization.k8s.io/prometheus-operator serverside-applied
deployment.apps/prometheus-operator serverside-applied
serviceaccount/prometheus-operator serverside-applied
service/prometheus-operator serverside-applied
Operator-Controller
Catalogd