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

[kube-prometheus-stack] Fix jobLabel on prometheus-node-exporter #4851

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

Conversation

monchaio
Copy link

What this PR does / why we need it

When crate prometheus with existing chart I found that jobLabel from metrics of node exporters are not "node-exporter".
After investigation I found that we need to use commonLabels instead of podLabels.
https://github.com/prometheus-community/helm-charts/blob/main/charts/prometheus-node-exporter/templates/_helpers.tpl#L46.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes #

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

Signed-off-by: Monchai Onlamai <[email protected]>
Signed-off-by: Monchai Onlamai <[email protected]>
@jkroepke
Copy link
Member

hm. I looked in that PR and I don't understand WHY is solves your issues. I'm unable to find the root cause here.

Normalls, pod and service monitor taking the label from the pod and service I can see that service and pod labels are defined.

@monchaio
Copy link
Author

monchaio commented Sep 13, 2024

hm. I looked in that PR and I don't understand WHY is solves your issues. I'm unable to find the root cause here.

Normalls, pod and service monitor taking the label from the pod and service I can see that service and pod labels are defined.

Because prometheus-node-exporter change theirs _helper.tpl to use commonLabels instead of podLabels https://github.com/prometheus-community/helm-charts/blob/main/charts/prometheus-node-exporter/templates/_helpers.tpl#L46

In older version of prometheus-node-exporter they use podLabels.

{{ include "prometheus-node-exporter.selectorLabels" . }}
{{- with .Chart.AppVersion }}
app.kubernetes.io/version: {{ . | quote }}
{{- end }}
{{- with .Values.podLabels }}
{{ toYaml . }}
{{- end }}

Without jobLabel on servicemonitoring. Any metrics from node-exporter have job label in format "<release>-prometheus-node-exporter" and break many rule that still use job label "node-exporter".

node_cpu_seconds_total{mode="idle",job="<release>-prometheus-node-exporter"}

@jkroepke
Copy link
Member

If the Service Monitor requires that label, using prometheus.monitor.additionalLabels would enough?

@zeritti
Copy link
Member

zeritti commented Sep 13, 2024

Without jobLabel on servicemonitoring. Any metrics from node-exporter have job label in format "-prometheus-node-exporter" and break many rule that still use job label "node-exporter".

The label expected by the service monitor, jobLabel, at the Node exporter's service has been set in prometheus-node-exporter.service.labels by default to node-exporter since 62.3.0 (values.yaml). Prometheus operator sets the Node exporter's job name correctly to the label's value.

@jkroepke
Copy link
Member

Thats what I observe. The current config should just work. Or I'm wrong?

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