Skip to content

Conversation

@bmm-alc
Copy link
Contributor

@bmm-alc bmm-alc commented Feb 25, 2025

with servicemonitor prometheus will be able to discover the metrics exposed on the pod.

with servicemonitor prometheus will be able to discover the metrics
exposed on the pod.
@bmm-alc
Copy link
Contributor Author

bmm-alc commented Mar 13, 2025

Hello,

I'd be happy to get some feedback, while I can still have my hand into this at work.
I have this working fine in my deployment.

Best

@lucasfcnunes
Copy link

@drakkan @sagikazarmark

PING

@lucasfcnunes
Copy link

@bmm-alc, can you solve the merge conflicts?

Comment on lines +16 to +18
selector:
matchLabels:
{{- include "sftpgo.selectorLabels" . | nindent 6 }}

Choose a reason for hiding this comment

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

There's no way to differentiate between the built-in .service and the extra .services.* on labels only.

I think we need to add labels to differentiate them and select only the internal service.

Comment on lines +76 to +84
{{- if .Values.monitoring.enabled }}
- name: telemetry
port: 10000
targetPort: telemetry
protocol: TCP
{{- if semverCompare ">=1.20-0" .Capabilities.KubeVersion.GitVersion }}
appProtocol: http
{{- end }}
{{- end }}

Choose a reason for hiding this comment

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

We should add the same if {{- if .Values.monitoring.enabled }} in the deployment telemetry env vars.

Comment on lines +6 to +11
labels:
{{- include "sftpgo.labels" . | nindent 4 }}
{{- with .Values.service.annotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}

Choose a reason for hiding this comment

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

Missing ServiceMonitor exclusive labels and annotations

@drakkan
Copy link
Member

drakkan commented Jun 13, 2025

@drakkan @sagikazarmark

PING

@lucasfcnunes I'm sorry but at the moment I don't have the time to properly review and test this change. Also, our CI is currently broken (see #33). Any help would be appreciated. Thank you

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.

3 participants