grafana / helm-charts

Apache License 2.0
1.55k stars 2.21k forks source link

[Tempo] - Overrides section changes #2802

Open bmgante opened 7 months ago

bmgante commented 7 months ago

Hi, Wondering to upgrade to tempo 2.3.0 and noticed the following section has changed: image

The helm chart which is updated seems not to reflect this change, or am i wrong?

  overrides:
    {{- toYaml .Values.global_overrides | nindent 2 }}
    {{- if .Values.metricsGenerator.enabled }}
    metrics_generator_processors:
    {{- range .Values.global_overrides.metrics_generator_processors }}
    - {{ . }}
    {{- end }}
    {{- end }}

My values.yaml needs to be changed? It is currently as below

# Global overrides
global_overrides:
  per_tenant_override_config: /runtime-config/overrides.yaml
  metrics_generator_processors:
    - service-graphs
    - span-metrics

Thanks

edgarkz commented 7 months ago

+1. At this moment the new style is not working using helm value. As a temporary solution - add manual section: values: |- overrides: | overrides: "*": ingestion_burst_size_bytes: 50000000 ingestion_rate_limit_bytes: 35000000

bmgante commented 7 months ago

What if changing the values yaml this way, would it work?

From:

  overrides:
    {{- toYaml .Values.global_overrides | nindent 2 }}
    {{- if .Values.metricsGenerator.enabled }}
    metrics_generator_processors:
    {{- range .Values.global_overrides.metrics_generator_processors }}
    - {{ . }}
    {{- end }}
    {{- end }}

...
global_overrides:
  per_tenant_override_config: /runtime-config/overrides.yaml
  #metrics_generator_processors: []
  metrics_generator_processors:
    - service-graphs
    - span-metrics

To:

  overrides:
  defaults:
    {{- toYaml .Values.global_overrides | nindent 2 }}
    {{- if .Values.metricsGenerator.enabled }}
    metrics_generator:
    {{- range .Values.global_overrides.metrics_generator.processors }}
    - {{ . }}
    {{- end }}
    {{- end }}

...
global_overrides:
  per_tenant_override_config: /runtime-config/overrides.yaml
  #metrics_generator_processors: []
  metrics_generator:
    processors:
      - service-graphs
      - span-metrics
      - local-blocks
bmgante commented 7 months ago

@edgarkz could you please elaborate a bit more on how to apply your manual workaround to have the helm chart working fine with 2.3.x tempo version and the new override syntax?

AlexDCraig commented 6 months ago

Yeah we definitely need this, looks like https://github.com/grafana/helm-charts/pull/2825 is necessary. If you're like me and have some global overrides it may make sense to wait until this patch is released to upgrade to the chart versions that use 2.3. Losing some of those configs suddenly would have really hurt if I hadn't checked the Grafana update notes. Significant stuff like this should also be in the chart release notes, as this directly relates to the chart

edgarkz commented 6 months ago

@edgarkz could you please elaborate a bit more on how to apply your manual workaround to have the helm chart working fine with 2.3.x tempo version and the new override syntax?

It doesn't work with new syntax.. I have added old syntax overrides to make it work in tempo 2.3 values: |- overrides: | overrides: "*":

pantuza commented 3 weeks ago

TL;DR

On the Helm Chart tempo-distributed instead of adding a parameter overrides, use global_overrides as a workaround:

global_overrides:
  defaults:
    ingestion:
      rate_limit_bytes: 32000000 # 32MB
      burst_size_bytes: 48000000 # 48MB
      max_traces_per_user: 50000

This is a workaround. The global_overrides isn't the right place but since the template outputs its content to the right final overrides block, you can use it by now.

Why the issue is happening?

The Grafana Tempo overrides documentation is correct! If your final yaml file ends up with a overrides block, it will work. Although, distributed-tempo Helm Chart is where the issue happens. Since version 1.8.0 of this Helm Chart it is using a new block called overrides that accepts a string. Even if you provide the proper string content it will fail simply because the Helm template isn't adding this block on the final yaml file.

Basically, the helm read the content of that block and add to a variable tempo.OverridesConfig (reference). Then it creates a ConfigMap that uses the content of this variable as a new file overrides.yaml (reference). And here comes the issue: when it generates the final yaml (tempo.yaml) it does not add the overrides block (reference). It lets its content on that external file called overrides.yaml.

Possible solutions

I think in this block the Helm Chart should not only add global_overrides but also overrides block as its content. This would output the overrides content into the final yaml file. Thus, solving the issue because Tempo binary will read the expected overrides block properly.