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

[DOC] Update overrides content in Helm chart to clarify configuration for new, legacy #3820

Closed
Tracked by #4326
knylander-grafana opened this issue Jun 27, 2024 · 7 comments · Fixed by #4415
Closed
Tracked by #4326
Labels
keepalive Label to exempt Issues / PRs from stale workflow type/docs Improvements or additions to documentation

Comments

@knylander-grafana
Copy link
Contributor

knylander-grafana commented Jun 27, 2024

Update the overrides configuration documentation to address changes to the Overrides module. The configuration examples need to be checked to make sure they are for the updated overrdies and which ones are not.

Update these pages:

The configuratio file in this doc works, but it's a YAML file:
"A snippet of a config.yaml file showing how the overrides section is here.

Reference: #3795, grafana/helm-charts#2802 (comment)

  • You need per-tenant overrides. Same format in both overrides configs
  • How does Tempo tell which configuration is new and which one is legacy?

Notes

Same issue happens in the tempo-distributed Helm chart:

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

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.

Copy link
Contributor

This issue has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity.
Please apply keepalive label to exempt this Issue.

@github-actions github-actions bot added the stale Used for stale issues / PRs label Aug 27, 2024
@knylander-grafana knylander-grafana changed the title [DOC] Update overrides content to clarify configuration for new, legacy [DOC] Update overrides content in Helm chart to clarify configuration for new, legacy Aug 29, 2024
@github-actions github-actions bot removed the stale Used for stale issues / PRs label Aug 30, 2024
Copy link
Contributor

This issue has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity.
Please apply keepalive label to exempt this Issue.

@github-actions github-actions bot added the stale Used for stale issues / PRs label Oct 29, 2024
@knylander-grafana knylander-grafana added keepalive Label to exempt Issues / PRs from stale workflow and removed stale Used for stale issues / PRs labels Nov 4, 2024
@knylander-grafana knylander-grafana moved this from Todo to In Progress in Tempo squad Dec 4, 2024
@knylander-grafana
Copy link
Contributor Author

Mario found an issue in the tempo-distributed Helm chart that prevented the overrides examples from working. The issue was corrected in this PR: grafana/helm-charts#3236

@knylander-grafana
Copy link
Contributor Author

knylander-grafana commented Dec 4, 2024

There is an issue in the Helm chart that needs to be fixed. Outlined here: grafana/helm-charts#2802 (comment)

The tempo-distributed Helm chart doesn't have the same implementation of the overrides as the standard Tempo configuration. The per tenant overrides is being implemented with the global overrides. The docs are correct, but how the overrides are implemented in the Helm chart is slightly different. We should update the tempo-distributed Helm chart documentation with a work around until this

@knylander-grafana
Copy link
Contributor Author

The README needs to have these updates:

  • Explain what the overrides values are in the table
  • Update the S3 example with the correct overrides

@knylander-grafana
Copy link
Contributor Author

Related Helm chart PRs that we might want to check:

@github-project-automation github-project-automation bot moved this from In Progress to Done in Tempo squad Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive Label to exempt Issues / PRs from stale workflow type/docs Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant