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

[prometheus] Add support for external_labels #3892

Closed

Conversation

adamchabin
Copy link

What this PR does / why we need it

Support for external_labels option under global section.

Which issue this PR fixes

When you use multiple Prometheus as agents you would like to add tags like tenant:

external_labels:
tenant: 'tenant1'

(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: Adam Chabin <[email protected]>
@zeritti zeritti changed the title prometheus-external_labels [prometheus] Add support for external_labels Oct 13, 2023
charts/prometheus/Chart.yaml Outdated Show resolved Hide resolved
charts/prometheus/templates/cm.yaml Outdated Show resolved Hide resolved
Comment on lines +274 to +276
"external_labels": {
"type": "object"
},
Copy link
Member

Choose a reason for hiding this comment

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

As shown above, you are setting the field nested in server, not in global. The schema has to include server.externalLabels instead. Please, remember to insert the new field in values.yaml as well with a short comment/reference as seen e.g. at server.remoteRead.

@adamchabin adamchabin requested a review from zeritti October 16, 2023 10:59
Signed-off-by: Adam Chabin <[email protected]>
@adamchabin adamchabin force-pushed the external_labels branch 2 times, most recently from 47d0f77 to ae3c207 Compare October 17, 2023 05:41
Comment on lines +237 to +242
{{/*
Define prometheus.server.global.externalLabels producing a list of externalLabels
*/}}
{{- define "prometheus.server.global.externalLabels" -}}
{{ toYaml .Values.server.global.externalLabels }}
{{- end -}}
Copy link
Member

Choose a reason for hiding this comment

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

The helper template is unnecessary.

Suggested change
{{/*
Define prometheus.server.global.externalLabels producing a list of externalLabels
*/}}
{{- define "prometheus.server.global.externalLabels" -}}
{{ toYaml .Values.server.global.externalLabels }}
{{- end -}}

Comment on lines +23 to +26
{{- if $root.Values.server.global.externalLabels }}
external_labels:
{{- include "prometheus.server.global.externalLabels" $root | nindent 8 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

The managed field externalLabels cannot be present in server.global as the latter gets inserted in Prometheus' config file as is and can therefore only include valid configuration parameters (the change won't pass CI). This is why external_labels and any other Prometheus' global configuration parameter can currently be set directly through this field.

Suggested change
{{- if $root.Values.server.global.externalLabels }}
external_labels:
{{- include "prometheus.server.global.externalLabels" $root | nindent 8 }}
{{- end }}
{{- with $root.Values.server.externalLabels }}
external_labels: {{ toYaml . | nindent 8 }}
{{- end }}

Comment on lines +232 to +234
## https://prometheus.io/docs/prometheus/latest/configuration/configuration/
##
externalLabels: {}
Copy link
Member

Choose a reason for hiding this comment

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

As in the comment above, needs to be moved out of server.global in server.

Suggested change
## https://prometheus.io/docs/prometheus/latest/configuration/configuration/
##
externalLabels: {}
## https://prometheus.io/docs/prometheus/latest/configuration/configuration/
##
externalLabels: {}

Copy link

stale bot commented Dec 15, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@zeritti zeritti closed this Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants