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

feat: Add ServiceMonitor for data-prepper #619

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

darnaut
Copy link

@darnaut darnaut commented Nov 9, 2024

Description

Adds ServiceMonitor resource for monitoring with Prometheus.

Issues Resolved

N/A

Check List

  • Commits are signed per the DCO using --signoff

For any changes to files within Helm chart directories:

  • Helm chart version bumped
  • Helm chart CHANGELOG.md updated to reflect change

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@peterzhuamazon
Copy link
Member

Hi @TheAlgo @prudhvigodithi could you take a look on this?

Thanks.

@sgorshkov85
Copy link

Hello.
Currently(with what to have: https://github.com/opensearch-project/helm-charts/blob/data-prepper-0.1.0/charts/data-prepper/templates/deployment.yaml#L49-L57), it makes sense to use PodMonitor instead.
OR - adjust https://github.com/opensearch-project/helm-charts/blob/data-prepper-0.1.0/charts/data-prepper/templates/service.yaml#L9-L15 to add server(port 4900) to the list, so the approach with service monitor will also work.

@darnaut
Copy link
Author

darnaut commented Nov 19, 2024

@sgorshkov85 For context, the choices made here were intentional:

  1. ServiceMonitor is typically the recommend approach by Prometheus and also works better here given that there's a service available and it will only match the endpoints available in the service (e.g. it won't scrape pods removed from the service)
  2. The port is not enabled by default because the ServiceMonitor itself is not enabled by default. I'm avoiding exposing by default a port which wasn't previously exposed. It's left up to the Helm chart user to enable the required port when enabling the ServiceMonitor.

@sgorshkov85
Copy link

sgorshkov85 commented Nov 20, 2024

@darnaut I got your point about ServiceMonitor, it was my first option as well.
I may lose something, but I don't see how it should work with these changes.

  1. To use ServiceMonitor monitor we need to configure the service resource. It could be done by defining the port variable:
    ports:
 - name: server
   port: 4900
  1. It will cause an error during the creation deployment resource, because of setting the same port number two times:
        ports:
        {{- range .Values.ports }}
          - name: {{ .name }}
            containerPort: {{ .port }}
            protocol: TCP
        {{- end }}
          - name: server
            containerPort: {{ (.Values.config).serverPort | default 4900 }}
            protocol: TCP

In the deployment this port is hardcoded, because it is used for the readiness and liveness probes.

@darnaut
Copy link
Author

darnaut commented Nov 21, 2024

@sgorshkov85 if there's some issue for you, please submit a PR fixing the duplicate port issue. this would be a problem regardless of this change (e.g. can't expose the server port in the service).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants