[tempo-distributed] Adding commonLabels for tempo-distributed HC. With this we'll be able to add labels to all K8S resources the chart is creating.

[tincho-burgos]

Enabling the tempo-distributed HelmChart to have commonLabels that will be added to all K8S created.

Signed-off-by: Martin Burgos [email protected]

[CLAassistant]

All committers have signed the CLA.

[KyriosGN0]

@tincho-burgos why not merge the logic to the tempo.labels?

[tincho-burgos]

It thought about that, but if I want to have some labels for expense tracking purposes, it wouldn't make sense to have those as part of the labels selectors.
Basically, I'm trying to add labels in an easy way (that I'll use later to filter resources) without affecting the current functionality.

[tincho-burgos]

Hi @Sheikh-Abubaker @BitProcessor @faustodavid,
Are you ok with this PR?

[tincho-burgos]

Hi @Sheikh-Abubaker @BitProcessor @faustodavid @swartz-k,
Could you please take a minute to review this PR? It helps to tag all K8S resources.

[tincho-burgos]

Hi @Sheikh-Abubaker @BitProcessor @faustodavid @swartz-k, Could you please take a minute to review this PR? It helps to tag all K8S resources.

Can you please take a look at it? @Sheikh-Abubaker @BitProcessor @faustodavid @swartz-k

[tincho-burgos]

Can you please take a look at it? @Sheikh-Abubaker @BitProcessor @faustodavid @swartz-k
It's been over a month for this.

[tincho-burgos]

Can you please take a look at it? @Sheikh-Abubaker @BitProcessor @faustodavid @swartz-k
Fixed linting, ran helm-docs.

[tincho-burgos]

Can you please take a look at it? @Sheikh-Abubaker @BitProcessor @faustodavid @swartz-k
Rebasing main, bump chart and ran helm-docs

[QuentinBisson]

@tincho-burgos this is a good idea but I probably would have only updated the helpers file by adding the common Label in the tempo.labels and tempo.podLabels that are already in the helpers file

[Sheikh-Abubaker]

@tincho-burgos Thanks for your contribution! it looks great, before we could proceed to merging it, I would like to elaborate on @QuentinBisson's above #3909 (comment)

• It would be much more clear and less confusing to folks, if the convention could be customLabels instead of commonLabels in the values.yaml file.

• The tempo.commonLabels helper function is perfect to wrap around the commonLabels logic from values.yaml file. However if we could wrap that exact logic within tempo.labels and tempo.podLabels helper function because every other resource include these functions thus achieving the desired goal of your new helper function i.e tempo.commonLabels in this case, the code looks more clean, less complex with minimal changes.

• For example tempo.labels helper function :

{{/*
Simple resource labels
*/}}
{{- define "tempo.labels" -}}
helm.sh/chart: {{ include "tempo.chart" .ctx }}
app.kubernetes.io/name: {{ include "tempo.name" .ctx }}
app.kubernetes.io/instance: {{ .ctx.Release.Name }}
{{- if .component }}
app.kubernetes.io/component: {{ .component }}
{{- end }}
{{- if .memberlist }}
app.kubernetes.io/part-of: memberlist
{{- end }}
{{- if .ctx.Chart.AppVersion }}
app.kubernetes.io/version: {{ .ctx.Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .ctx.Release.Service }}
{{- end -}}
{{/*
Custom labels for resources
*/}}
{{- with .Values.customLabels }}
{{ toYaml . }}
{{- end }}
{{- end -}}

• Please try a similar approach as above for tempo.podLabels function as well.

[QuentinBisson]

@Sheikh-Abubaker in that case, would it not be better to define global.labels and global.podLabels instead of customLabels? This is a breaking change though

[Sheikh-Abubaker]

@Sheikh-Abubaker in that case, would it not be better to define global.labels and global.podLabels instead of customLabels? This is a breaking change though

I guess so, could you elaborate more ?

[QuentinBisson]

First of, this would make this chart use the same patterns for label as the Loki and Mimir helm charts which provides the global.podLabels and global.podAnnotations fields.

Those are useful, specifically in the context of subcharts because those are also passed around.

For instance, I would find it useful to add tempo culture as a subchart. Following this global pattern would ensure that subcharts could use those labels as well

[Sheikh-Abubaker]

Following this global pattern would ensure that subcharts could use those labels as well

That's even better as it aligns with loki and mimir charts maintaining consistency, more simpler for umbrella chart scenarios and aligns well with this PR's scope as well! sounds good to me!

[tincho-burgos]

Hey @QuentinBisson @Sheikh-Abubaker,

Just to confirm I made the changes and bumped the chart version.

[Sheikh-Abubaker]

Hey @QuentinBisson @Sheikh-Abubaker,

Just to confirm I made the changes and bumped the chart version.

@tincho-burgos Thank you for the changes, look good to me! I just merged a PR so you'd need to bump version again.

[tincho-burgos]

@Sheikh-Abubaker Done!

[Sheikh-Abubaker]

@tincho-burgos Here's the trailing space, just get rid of it to fix failing CI
https://github.com/grafana/helm-charts/blob/6434e0321a6ba7477e312f0594571b42d6d8f3ff/charts/tempo-distributed/values.yaml#L13C39-L14C3

[tincho-burgos]

@Sheikh-Abubaker Done! Apologies for that, I missed the excluding pods part

[tincho-burgos]

@Sheikh-Abubaker , I have added an if to bypass the error if it's empty.

==> Linting charts/tempo-distributed Error: templates/: template: tempo-distributed/templates/query-frontend/service-query-frontend.yaml:7:8: executing "tempo-distributed/templates/query-frontend/service-query-frontend.yaml" at <include "tempo.labels" (dict "ctx" . "component" "query-frontend")>: error calling include: template: tempo-distributed/templates/_helpers.tpl:93:16: executing "tempo.labels" at <.Values.global.commonLabels>: nil pointer evaluating interface {}.global

[tincho-burgos]

@Sheikh-Abubaker it should work now.

[Sheikh-Abubaker]

I have added an if to bypass the error if it's empty.

That wouldn't help because it wasn't a with or if problem, what we actually need is .ctx.Values.global.commonLables I suppose due to the temporary dict being created here so we would need to call it via ctx

helm-charts/charts/tempo-distributed/templates/query-frontend/service-query-frontend.yaml

Line 7 in 8e94633

{{- include "tempo.labels" (dict "ctx" . "component" "query-frontend") | nindent 4 }}

[Sheikh-Abubaker]

LGTM!

[tincho-burgos]

@Sheikh-Abubaker Thanks for the help!