change: disable legacy overrides by default, add flag to opt in#6741
change: disable legacy overrides by default, add flag to opt in#6741electron0zero merged 7 commits intografana:mainfrom
Conversation
Legacy (flat, unscoped) overrides are now disabled by default. Tempo will refuse to start if legacy overrides are detected unless the user sets enable_legacy_overrides: true (or -config.enable-legacy-overrides=true). A deprecation warning is logged when legacy overrides are enabled. The tempo-cli migrate-overrides-config command force-enables legacy overrides since its purpose is to convert them to the new format.
Migrate the super_user per-tenant overrides in the jsonnet microservices config from the flat legacy format to the new scoped format with ingestion and global sections. Regenerate compiled configs.
There was a problem hiding this comment.
Pull request overview
This PR introduces a breaking change to the overrides subsystem by disabling the legacy (flat/unscoped) overrides format by default, and adds an explicit escape-hatch config/CLI flag to temporarily re-enable it while users migrate to the scoped overrides format.
Changes:
- Add
enable_legacy_overrides(and-config.enable-legacy-overrides) to opt back into legacy overrides temporarily. - Reject legacy overrides on startup unless explicitly opted in, and emit deprecation warnings.
- Update Jsonnet examples/compiled manifests and documentation to use the new scoped overrides structure.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| operations/jsonnet/microservices/config.libsonnet | Updates example overrides content to scoped (ingestion/global) structure. |
| operations/jsonnet-compiled/microservices/gen/ConfigMap-tempo-overrides.yaml | Regenerates overrides ConfigMap to scoped format. |
| operations/jsonnet-compiled/microservices-with-extras/gen/ConfigMap-tempo-overrides.yaml | Regenerates overrides ConfigMap to scoped format (extras variant). |
| operations/jsonnet-compiled/jsonnetfile.lock.json | Updates pinned Jsonnet dependency revisions. |
| modules/overrides/runtime_config_overrides_test.go | Updates legacy-path tests to explicitly enable legacy overrides. |
| modules/overrides/overrides_test.go | Adds coverage asserting legacy overrides are disabled by default. |
| modules/overrides/overrides.go | Enforces legacy disable-by-default and logs deprecation warning. |
| modules/overrides/config.go | Adds EnableLegacyOverrides config field, YAML wiring, and CLI flag. |
| docs/sources/tempo/configuration/_index.md | Documents the new enable_legacy_overrides option. |
| cmd/tempo/app/config.go | Updates the startup config warning text for legacy overrides usage. |
| cmd/tempo-cli/cmd-migrate-overrides-config.go | Forces legacy overrides enabled in the migration command path. |
Log a deprecation warning on every per-tenant overrides reload when legacy format is detected, matching the behavior in NewOverrides. Also reject legacy per-tenant overrides when EnableLegacyOverrides is false..
|
@codex review please |
There was a problem hiding this comment.
Pull request overview
Disables legacy (flat/unscoped) overrides by default and introduces a config/CLI escape hatch to temporarily allow them, while updating generated manifests and docs accordingly.
Changes:
- Add
enable_legacy_overrides/-config.enable-legacy-overridesto opt into legacy overrides, otherwise error on startup if legacy format is detected. - Add/adjust tests to cover the new default behavior (legacy rejected unless explicitly enabled).
- Update example Jsonnet and generated ConfigMaps to the new scoped overrides format; update docs and changelog for the breaking change.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| operations/jsonnet/microservices/config.libsonnet | Update example overrides to new scoped format (ingestion/global). |
| operations/jsonnet-compiled/microservices/gen/ConfigMap-tempo-overrides.yaml | Regenerate overrides ConfigMap to scoped format. |
| operations/jsonnet-compiled/microservices-with-extras/gen/ConfigMap-tempo-overrides.yaml | Regenerate overrides ConfigMap to scoped format. |
| operations/jsonnet-compiled/jsonnetfile.lock.json | Bump Jsonnet dependency lock versions. |
| modules/overrides/runtime_config_overrides_test.go | Update tests for new loader signature + explicitly enable legacy in legacy test case. |
| modules/overrides/runtime_config_overrides.go | Gate legacy per-tenant override loading behind new enableLegacy parameter and error if disabled. |
| modules/overrides/overrides_test.go | Add tests asserting legacy overrides are rejected by default (static + per-tenant). |
| modules/overrides/overrides.go | Error on startup when legacy overrides are detected unless explicitly enabled; log deprecation warning. |
| modules/overrides/config.go | Add EnableLegacyOverrides config field, YAML handling, and CLI flag registration. |
| docs/sources/tempo/configuration/manifest.md | Document enable_legacy_overrides in the rendered manifest. |
| docs/sources/tempo/configuration/_index.md | Document enable_legacy_overrides config option. |
| cmd/tempo/app/config.go | Update legacy overrides warning message and add a migration “Explain”. |
| cmd/tempo-cli/cmd-migrate-overrides-config.go | Force-enable legacy overrides when running the migration command. |
| CHANGELOG.md | Add breaking-change entry describing the new default + opt-in flag. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63453badd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
stoewer
left a comment
There was a problem hiding this comment.
LGTM. However, some of the bot review comments are actually quite useful. Should we apply them?
…ation log, fix warning message - Reset EnableLegacyOverrides before marshaling migrated config output - Throttle per-tenant legacy deprecation warning to once per 10 minutes - Match config warning Explain text with other error messages
|
@codex review please |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
What this PR does:
Disable legacy (flat, unscoped) overrides by default. Tempo will refuse to start if legacy overrides are detected in either the main config or per-tenant overrides file.
Added
enable_legacy_overridesconfig option (and-config.enable-legacy-overridesCLI flag) to opt back in temporarily.If legacy overrides are enabled, a deprecation warning is logged on startup and each time per tenant overrides are loaded.
Breaking Change
Tempo will now error on startup if legacy overrides config is detected. Users must either:
Before (legacy):
After (new):
Similar changes can be done in per tenant overrides file, and see https://grafana.com/docs/tempo/latest/configuration/#overrides for more details.
enable_legacy_overrides: truein the overrides config block or-config.enable-legacy-overrides=trueon the CLI. This is a temporary escape hatch - legacy overrides will be removed in a future release.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]