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

Overrides Configuration Is Not Working #3795

Open
randrewy opened this issue Jun 19, 2024 · 20 comments · Fixed by grafana/helm-charts#3236
Open

Overrides Configuration Is Not Working #3795

randrewy opened this issue Jun 19, 2024 · 20 comments · Fixed by grafana/helm-charts#3236

Comments

@randrewy
Copy link

Describe the bug
Docs states that starting from version 2.3 there is a new overrides block.

To Reproduce
Steps to reproduce the behavior:

  1. Start Tempo with the following overrides section in config:
overrides:
  defaults:
    metrics_generator:
      processors: [service-graphs, span-metrics]
  1. get the error: line 38: field defaults not found in type overrides.legacyConfig

Expected behavior
Server starts.

Environment:

  • Infrastructure: local docker
  • Deployment tool: docker run

Additional Context
image "grafana/tempo:2.5.0"

At the same time another line from docs shows old format: "A snippet of a config.yaml file showing how the overrides section is here.". And with that legacy configuration everything works fine.

@mapno
Copy link
Member

mapno commented Jun 21, 2024

Hi! Do you have per-tenant overrides? You need to use the same format in both overrides configs.

@randrewy
Copy link
Author

Hello!
I have multi-tenancy disabled in the same config file

@ftong2020
Copy link

indeed, I am encountering this issue, too. It is quite confusing. Documentation is no where to be found how tempo tells which configuration is "new" and which is "legacy".

@AlexDCraig
Copy link
Contributor

AlexDCraig commented Jun 26, 2024

I get this too using the tempo-distributed helm chart and:

global_overrides:
  defaults:
    metrics_generator:
      processors:
        - span-metrics
      processor:
        span_metrics:
          histogram_buckets: [null]
          dimensions:
            - kubernetes_cluster
            - kubernetes.cluster
            - http.status_code

@mapno
Copy link
Member

mapno commented Jun 27, 2024

Documentation is no where to be found how tempo tells which configuration is "new" and which is "legacy".

That's fair. We probably didn't add the best documentation of the change. As a rule of thumb: long, unindented params are legacy (eg. metrics_generator_forwarder_queue_size), indented params are new (eg. metrics_generator.forwarder.queue_size).

With the new config, a new key defaults was also added after the overrides key, to be more explicit of the contents of the config block.

Finally, there is a command in the tempo-cli that allows you to convert legacy config to new overrides config.

@knylander-grafana
Copy link
Contributor

Thank you for the feedback on the docs. I've created a doc issue to make sure that we capture these changes and address the issues you've had.

#3820

@markustoivonen
Copy link
Contributor

markustoivonen commented Jul 17, 2024

@mapno How is one supposed to use overrides currently with helm, when using tempo-distributed? the tempo-distributed helm chart expects the value be

overrides: |
  overrides: 
    foo: bar

And what is in the documentation is:

overrides:
  defaults:
    foo: bar

doesn't work, as it results in an error

template: tempo-distributed/templates/_helpers.tpl:191:14: executing "tempo.overridesConfig" at <.Values.overrides>: wrong type for value; expected string; got map[string]interface {}

When using the legacy method with the legacy names (e.g. ingestion_rate_strategy), in my deployment logs there is the following message:

msg="Overrides config type mismatch" err="per-tenant overrides config type does not match static overrides config type" config_type=legacy static_config_type=new

I am also using the latest helm chart version.

@markustoivonen
Copy link
Contributor

@randrewy @AlexDCraig were you able to solve this somehow?

@randrewy
Copy link
Author

randrewy commented Jul 22, 2024

@markustoivonen I had to use old format despite of what docs are saying. So my overrides section looks like this

overrides:
  metrics_generator_processors: [service-graphs, span-metrics]
  metrics_generator_processor_local_blocks_complete_block_timeout: "1h"

despite the fact I use latest stable version available atm (2.5.0).

I am not using helm, so this is easy for me to do

@mapno
Copy link
Member

mapno commented Jul 23, 2024

I've drafted a PR to fix the bug grafana/helm-charts#3236. I've tested it locally both with new and legacy overrides, but more testing is welcome. Sorry for the inconveniences.

@knylander-grafana
Copy link
Contributor

@mapno I know we need to update the docs to address new vs legacy. With your fix, will the example in the docs linked in. previous comments still work?

@mapno
Copy link
Member

mapno commented Jul 24, 2024

@mapno I know we need to update the docs to address new vs legacy. With your fix, will the example in the docs linked in. previous comments still work?

The examples are now correct, they didn't work before. Yes, legacy overrides still work too :)

@markustoivonen
Copy link
Contributor

markustoivonen commented Aug 26, 2024

@mapno

I am trying to generate charts again, here is my overrides section:

overrides:
  defaults:
    ingestion:
      rate_strategy: local
      burst_size_bytes: 20000000

and receiving error

coalesce.go:237: warning: skipped value for tempo-distributed.overrides: Not a table.
coalesce.go:237: warning: skipped value for tempo-distributed.overrides: Not a table.
Error: template: tempo-distributed/templates/configmap-runtime.yaml:11:7: executing "tempo-distributed/templates/configmap-runtime.yaml" at <include "tempo.overridesConfig" .>: error calling include: template: tempo-distributed/templates/_helpers.tpl:191:14: executing "tempo.overridesConfig" at <.Values.overrides>: wrong type for value; expected string; got map[string]interface {}

If I pass overrides: "foo" the chart generation works no problem. Whats the issue here?

edit: The issue seems to have been in my helm cache, I think it pulled the old version of the chart for some reason. Got it working after clearing cache. Sorry for the ping! The point below is still relevant though.

Also, the example config file the documentation links to seems to be outdated.

@markustoivonen
Copy link
Contributor

markustoivonen commented Sep 10, 2024

@mapno I am back with another override issue when using the tempo-distributed helm chart. In my case, when using helm, I create the manifests, from from which they are deployed (so not using helm directly to modify my K8s cluster)

In my values.yaml I have:

overrides:
  defaults:
    ingestion:
      rate_strategy: local
      burst_size_bytes: 50000000
      rate_limit_bytes: 20000000
      max_traces_per_user: 15000

After generating my manifests, I have a configmap-runtime.yaml with the following content:

---
# Source: tempo-distributed/templates/configmap-runtime.yaml
apiVersion: v1
kind: ConfigMap
<...>
  overrides.yaml: |
    
    overrides:
      defaults:
        ingestion:
          burst_size_bytes: 50000000
          max_traces_per_user: 15000
          rate_limit_bytes: 20000000
          rate_strategy: local

And when I ssh into a deployed pod to check the contents of ~/runtime-config/overrides.yaml, it has the desired values in the same format as in the configmap.

However, my tempo deployment constantly logs error messages like:

msg="pusher failed to consume trace data" err="rpc error: code = ResourceExhausted desc = RATE_LIMITED: ingestion rate limit (local: 15000000 bytes, global: 0 bytes) exceeded while adding 979109 bytes for user single-tenant"

Any ideas why the overrides are not getting applied although I am using the format shown in documentation?

@markustoivonen
Copy link
Contributor

markustoivonen commented Sep 18, 2024

This answer is what solved the issue for me. So maybe a fix is needed for the overrides field?

@eugenekurasov
Copy link

Hi, I got the same message field defaults not found in type overrides.legacyConfig, and after a couple of hours of investigation, I found that I had a mix between the old format and the new.
For example.

    global_overrides:
      defaults:
        metrics_generator:
          processors: [service-graphs, span-metrics]
      max_bytes_per_trace: 22428800

max_bytes_per_trace - it is from old format
In the new format max_bytes_per_trace moved to the defaults.global section.

    global_overrides:
      defaults:
        metrics_generator:
          processors: [service-graphs, span-metrics]
        global:
          max_bytes_per_trace: 22428800

Summary: The error message means you use the new and old format at the same time, so please check all your options, probably some of them have been moved.

In my opinion error message should change to avoid confuse.

@ubcharron
Copy link

If it helps anyone else, this is what my helm values.yaml looks like, and it no longer generates any warnings or errors:

overrides:
  defaults:
    ingestion:
      max_traces_per_user: 1000 # 1k
      rate_limit_bytes: 2000000 # 2M
      burst_size_bytes: 4000000 # 4M

  tenant1:
    ingestion:
      max_traces_per_user: 2000000
      rate_limit_bytes: 15000000
      burst_size_bytes: 20000000
    metrics_generator:
      processors: [service-graphs, span-metrics, local-blocks]
      max_active_series: 0
      collection_interval: 15s

Chart version 1.18.0, and I did not modify global_overrides.

@eugenekurasov
Copy link

My point is that the error message doesn't clarify what exactly need to do. That error message means you use an old and a new config format at the same time.

@kelt1k
Copy link

kelt1k commented Oct 25, 2024

Hi all,
I'm using some simple workaround for this case
Just by enabling these two options

useExternalConfig: true 
configStorageType: ConfigMap

As I'm using Kustomize in couple with helm before generating configmap I used tempo-cli for migration to new overrides
then I have just this

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
  - tempo-gcp-secret.yaml
  - httproute.yaml

helmCharts:
  - name: tempo-distributed
    repo: https://grafana.github.io/helm-charts
    version: 1.20.0
    releaseName: tempo-distributed
    namespace: tracing
    valuesFile: ./values.yaml

configMapGenerator:
  - name: tempo-distributed-config
    files:
      - config/tempo.yaml
      - config/overrides.yaml

generatorOptions:
  disableNameSuffixHash: true

@knylander-grafana
Copy link
Contributor

Hi there! I noticed that we had a bunch of issues open for the overrides settings for the tempo-distributed Helm. chart. We have two updates that may address the issue that you're having:

Please let me know if this helps.

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 a pull request may close this issue.

9 participants