Overrides module refactor

[mapno]

What this PR does:

Follow up to #2638

The Limits struct has a mixed responsibility:

• it contains all the limits (we have more than 30 now!)
• it contains configuration for the overrides module itself

When it is used in the top-level App struct it contains default values for the limits and configuration for per-tenant overrides. But when Limits is used by the runtime config manager it contains per tenant values of the limits.

The dual purpose of Limits is awkward and confusing.

Furthermore, the overrides module has outgrown its original scope of providing limits to Tempo components, to hosting any kind of dynamic configuration that can be modified at runtime. In the same config block we have ingestion limits like ingestion_rate_limit_bytes, metrics-generator config such as metrics_generator_ring_size, and (soon™️) storage config with the introduction of #2551.

All of this makes for a complex config structure, with overly long param names (eg. metrics_generator_processor_service_graphs_enable_client_server_prefix), and unclear scope.

This PR addresses this by creating a Config struct that is explicitly intended to configure the overrides module, ensuring the Limits struct can focus on holding limits only. It also adds indentation to the parameters, making them easier to read and write.

These has effects on the tempo config file and the per tenant overrides file.

Old config:

overrides:
  ingestion_rate_strategy: local
  ingestion_rate_limit_bytes: 12345
  ingestion_burst_size_bytes: 67890
  max_search_duration: 17s
  forwarders: ['foo']
  metrics_generator_processors: [service-graphs, span-metrics]

New config:

overrides:
  defaults:
    ingestion:
      rate_strategy: local
      rate_limit_bytes: 12345
      burst_size_bytes: 67890
    read:
      max_search_duration: 17s
    forwarders: ['foo']
    metrics_generator:
      processors: [service-graphs, span-metrics]

Which issue(s) this PR fixes:
Fixes #2641

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[yvrhdn]

Nice work! This must have been a ton of work to track down all the compilation errors 😅

[knylander-grafana]

Docs look good. Thank you for adding them.

[yvrhdn]

Looks good to me ✅

Can't approve since I also have changes in here though + I think having another review to make sure the new overrides struct is reasonable would be nice.

[zalegrala]

This looks pretty good to me. A little tough to review the toLegacy bits due to the high number of config options. Have we been able to test this out somewhere? A little nervous to merge on a Friday.

[zalegrala]

I like that this lines up a little more consistently.

[mapno]

Have we been able to test this out somewhere?

I've tested it locally, but I can deploy it somewhere if necessary.

[yvrhdn]

Code changes look good to me. I think we will need to do some more docs changes to remove uses of the old structure, or did you want to do these in a separate PR?

The following pages need updates and there might be more examples with overrides:

• https://grafana.com/docs/tempo/latest/configuration/#overrides
• https://grafana.com/docs/tempo/latest/configuration/manifest/

[zalegrala]

This looks reasonable to me. Do we have thoughts about how to transition our jsonnet to the new approach?

[stoewer]

The code changes look good to me
But I agree with @kvrhdn that the docs for the overrides should be updated. I think it would be OK to do this in an extra PR though

[mapno]

Hi folks, I've updated all the docs. I'll open a new issue to handle the migration of jsonnet.

[YayBurritos]

@mapno : How/where do I put this in the Tempo Distributed Helm chart? Currently working with the 1.7.1 chart (multitenancy not enabled). Thanks!

overrides:
  defaults:
    ingestion:
      rate_strategy: local
      rate_limit_bytes: 12345
      burst_size_bytes: 67890
    read:
      max_search_duration: 17s
    forwarders: ['foo']
    metrics_generator:
      processors: [service-graphs, span-metrics]

I get errors on startup similar to this:

line 106: field defaults not found in type overrides.legacyConfig

[mapno]

Hey. Unfortunately, this PR is required to use the new format grafana/helm-charts#2825. Let's see if we can get a final approval.

[YayBurritos]

Hey. Unfortunately, this PR is required to use the new format grafana/helm-charts#2825. Let's see if we can get a final approval.

Thanks for the update! Didn't realize the Tempo Distributed Helm chart wasn't equipped to handle the new overrides format (aside from me trying to force it to happen). I may stick with the 'legacy' format for now in order to keep things moving on our side.

[ToonTijtgat2]

Dear

Can someone please give an example on how to use this new overrides config?

I placed it in the conf/tempo.yaml
Placed it on runtime-config/overrides.yaml
placed it under conf/overrides.yaml

It always keeps saying defaults is not found on the legacyoverrides.

The config I try to accomplice looks like this.

    defaults:
      global:
        max_bytes_per_trace: 1500000
      metrics_generator:
        processors: [service-graphs, span-metrics]
        remote_write_headers:
          tenant-id: prometheus-apps-app-dev```

I have a feeling I oversee something, because it can not be that hard to get those things done I guess?