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

Clean up config in overrides module #2641

Closed
2 tasks
mapno opened this issue Jul 12, 2023 · 1 comment · Fixed by #2688
Closed
2 tasks

Clean up config in overrides module #2641

mapno opened this issue Jul 12, 2023 · 1 comment · Fixed by #2688

Comments

@mapno
Copy link
Member

mapno commented Jul 12, 2023

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. So, in the same config block we have ingestion limits like ingestion_rate_limit_bytes and max_traces_per_user, metrics-generator config such as metrics_generator_ring_size and metrics_generator_processor_service_graphs_histogram_buckets, and soon even storage config with the introduction of vParquet3.

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.

Proposals

  • Add indentation to overrides block to make reading and writing configs easier

    Old overrides:

    overrides:
        ingestion_rate_strategy: global
        ingestion_rate_limit_bytes: 1000000
        ingestion_burst_size_bytes: 1000000
        max_traces_per_user: 10000
        metrics_generator_ring_size: 3
        metrics_generator_processors: [span-metrics, service-graphs]
        metrics_generator_max_active_series: 100000
        metrics_generator_processor_service_graphs_dimensions: [http.status, http.method]
        metrics_generator_processor_span_metrics_enable_target_info: true

    New overrides:

    overrides:
        ingestion:
            rate_strategy: global
            rate_limit_bytes: 1000000
            burst_size_bytes: 1000000
            max_traces_per_user: 10000
        metrics_generator:
            ring_size: 3
            processors: [span-metrics, service-graphs]
            max_active_series: 100000
            processor:
                service_graphs:
                    dimensions: [http.status, http.method]
                span_metrics:
                    enable_target_info: true
  • Drop word limits from the overrides module. It's not longer accurate and confusing when developing. (Not referring to override names).

@yvrhdn
Copy link
Member

yvrhdn commented Jul 12, 2023

I think we can combine this with #2638, this moves the default values from the top-level to it's own nested key. If we are changing the yaml structure anyway, we can do both at once imo.

During transition we could follow this logic for parsing the config:

  • attempt to parse the config using the new nested structure
  • if this fails, attempt to parse into the flat structure
  • if this also fails we return an error

We can print a warning when people use the flat structure to encourage them to transition.

@mapno mapno mentioned this issue Jul 24, 2023
3 tasks
@github-project-automation github-project-automation bot moved this from Todo to Done in Tempo squad Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants