grafana / helm-charts

Apache License 2.0
1.56k stars 2.22k forks source link

[tempo-distributed] Missing mount for /conf/tempo-query.yaml #2023

Open eric-engberg opened 1 year ago

eric-engberg commented 1 year ago

Chart version 0.27.9 for tempo-distributed does not have an entry to mount the tempo-query.yaml file

2022-11-29T20:28:34.583Z [ERROR] tempo-query: failed to parse configuration file: @module=jaeger-tempo error="open /conf/tempo-query.yaml: no such file or directory" timestamp=2022-11-29T20:28:34.583Z

The template file only has the following for mounting of the config volume

The volume to mount for tempo configuration */}} {{- define "tempo.configVolume" -}} {{- if eq .Values.configStorageType "Secret" -}} secret:   secretName: {{ tpl .Values.externalConfigSecretName . }} {{- else if eq .Values.configStorageType "ConfigMap" -}} configMap:   name: {{ tpl .Values.externalConfigSecretName . }}   items:      - key: "tempo.yaml"         path: "tempo.yaml"      - key: "overrides.yaml"         path: "overrides.yaml" {{- end -}} {{- end -}}

There is no entry for the tempo-query.yaml file

Currently I am working around this with a kustomize patch

patchesJson6902:    - target:       group: apps       version: v1       kind: Deployment       name: grafana-tempo-distributed-query-frontend       patch: |-         - op: add            path: /spec/template/spec/volumes/0/configMap/items/-            value:              key: tempo-query.yaml              path: tempo-query.yaml

zanhsieh commented 1 year ago

@eric-engberg Could you try and review above PR #2059 ?

vaish1707 commented 1 year ago

@zanhsieh @eric-engberg I was also getting error while using tempo with jaeger and s3 as storage that its unable to read the "/conf/tempo-query.yaml" and the tempo-query container was crashing.. for this to work I turned on query.enabled to true and few more required configuration for traces storage in s3.. Does this PR https://github.com/grafana/helm-charts/pull/2059 resolve that issue?

sharkymcdongles commented 1 year ago

The problem is they use keys to mount the yaml bodies from the configmap for each file and fail to include tempo-query.yaml.

The linked PR will actually break if the query frontend is disabled. The easiest workaround for now is to simply change from configmap to secret for .Values.configStorageType because when using secret type it mounts the entire secret so the tempo-query.yaml is included.

Broken templating here:

{{/*
The volume to mount for tempo configuration
*/}}
{{- define "tempo.configVolume" -}}
{{- if eq .Values.configStorageType "Secret" -}}
secret:
  secretName: {{ tpl .Values.externalConfigSecretName . }}
{{- else if eq .Values.configStorageType "ConfigMap" -}}
configMap:
  name: {{ tpl .Values.externalConfigSecretName . }}
  items:
    - key: "tempo.yaml"
      path: "tempo.yaml"
    - key: "overrides.yaml"
      path: "overrides.yaml"
{{- end -}}
{{- end -}}

I would suggest amending the PR to just use the same mounting as secret without the keys:

{{/*
The volume to mount for tempo configuration
*/}}
{{- define "tempo.configVolume" -}}
{{- if eq .Values.configStorageType "Secret" -}}
secret:
  secretName: {{ tpl .Values.externalConfigSecretName . }}
{{- else if eq .Values.configStorageType "ConfigMap" -}}
configMap:
  name: {{ tpl .Values.externalConfigSecretName . }}
{{- end -}}
{{- end -}}

If the /conf/ folder cannot be overridden though and keys are needed then a boolean conditional for adding the tempo-query.yaml file needs to be added.

sharkymcdongles commented 1 year ago

On further review and testing, it seems the query container isn't even needed anymore since Grafana 7.5.

https://github.com/grafana/helm-charts/blob/main/charts/tempo-distributed/values.yaml#L498

manojm321 commented 10 months ago

This is needed if you want a direct link to trace. As grafana doesn't support something like https://<host>/trace/trace-id. Few of our tools that generate traces use this short link.