Skip to content
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

Add servicemonitors #159

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

seeker815
Copy link

@seeker815 seeker815 commented Jan 9, 2024

Description

Enable ServiceMonitors resources for intents operator and network mapper which Prometheus can use to scrape metrics endpoints.

Checklist

  • I have updated the values file to enable or disable servicemonitor for the operator
  • I have tested the service monitor on local kubernetes setup

Copy link

github-actions bot commented Jan 9, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@seeker815
Copy link
Author

I have read and understood the CLA and hereby agree to its terms by making this Pull Request Comment.

@seeker815
Copy link
Author

recheck

@seeker815 seeker815 changed the title Seeker815/add servicemonitor Add servicemonitors Jan 9, 2024
@seeker815
Copy link
Author

Please review @orishoshan and we can add on/improve with servicemonitor for credentials operator

@orishoshan orishoshan self-requested a review January 17, 2024 12:40
@orishoshan
Copy link
Contributor

Hey @seeker815, having a look now :)

app.kubernetes.io/version: {{ .Chart.Version }}
spec:
endpoints:
- port: https
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? I think the metrics are served on port 2112.
Can you create a helper template (https://github.com/otterize/helm-charts/blob/main/intents-operator/templates/_helpers.tpl) for the port, and then use the template in both places?

Copy link
Author

@seeker815 seeker815 Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, as we are referring to the same metrics port 2112. The service definition for intents-operator-controller-manager-metrics-service has it is labelled as https, see service definition below

  - name: https
    port: 2112
    protocol: TCP
    targetPort: 2112
  selector:
    app: intents-operator

Maybe we could change the port name for service definition at some point for cleaner name. Do we need helper template or not needed after the above clarification @orishoshan

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seeker815 Please fix the port name in the Service then :) The port in the Deployment is correctly labeled "metrics"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change that as well, thanks

app.kubernetes.io/version: {{ .Chart.Version }}
spec:
endpoints:
- port: http
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here re the metrics port

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also related to service port labelling which we are referring to in the service monitor, the otterize-network-mapper has

ports:
  - name: http
    port: 9090
    protocol: TCP
    targetPort: 9090

@orishoshan
Copy link
Contributor

orishoshan commented Jan 17, 2024

Please update the README.md for each component (the Markdown table documenting the Helm values), and also add a configuration in .Values.global.serviceMonitor that enables it for all components together. The behavior you should have: .Values.networkMapper.serviceMonitor and .Values.intentsOperator.serviceMonitor should be preferred if they are set. If not set, use the value from .Values.global.serviceMonitor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants