chore: remove partition ring livestore config - #6981
Conversation
![chatgpt-codex-connector[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ff529dc2e
ℹ️ 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".
| StreamOverHTTPEnabled bool `yaml:"stream_over_http_enabled,omitempty"` | ||
| HTTPAPIPrefix string `yaml:"http_api_prefix"` | ||
| EnableGoRuntimeMetrics bool `yaml:"enable_go_runtime_metrics,omitempty"` |
There was a problem hiding this comment.
Removing partition_ring_live_store from app.Config makes upgrades fail fast for any existing config that still includes this key, because runtime config loading is strict (yaml.UnmarshalStrict in cmd/tempo/main.go:185). This key existed in all shipped example configs before this change, so users who only bump the binary (without rewriting config) will now get an unknown-field startup error. Please keep an ignored compatibility field (similar to other deprecated config shims) for at least one release or add a migration shim so old configs still boot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Removes the partition_ring_live_store configuration knob from Tempo’s root config and updates bundled example/integration configs accordingly.
Changes:
- Drops
partition_ring_live_storefromcmd/tempo/approotConfig. - Removes
partition_ring_live_storefrom integration base config and multiple example deployment configs. - Cleans up the microservices Jsonnet example by removing related toggles.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| integration/util/config-base.yaml | Removes partition_ring_live_store from the e2e base configuration. |
| example/tk/tempo-microservices/main.jsonnet | Removes partition_ring_live_store (and related example toggles) from the Jsonnet-generated Tempo config. |
| example/nomad/tempo-distributed/config.yml | Removes partition_ring_live_store from the Nomad example config. |
| example/docker-compose/single-binary/tempo.yaml | Removes partition_ring_live_store from the single-binary docker-compose example. |
| example/docker-compose/multitenant/tempo.yaml | Removes partition_ring_live_store from the multitenant docker-compose example. |
| example/docker-compose/distributed/tempo.yaml | Removes partition_ring_live_store from the distributed docker-compose example. |
| example/docker-compose/debug/tempo.yaml | Removes partition_ring_live_store from the debug docker-compose example. |
| cmd/tempo/app/config.go | Removes the PartitionRingLiveStore field and its default assignment from the root config struct. |
| StreamOverHTTPEnabled bool `yaml:"stream_over_http_enabled,omitempty"` | ||
| HTTPAPIPrefix string `yaml:"http_api_prefix"` | ||
| EnableGoRuntimeMetrics bool `yaml:"enable_go_runtime_metrics,omitempty"` | ||
| PartitionRingLiveStore bool `yaml:"partition_ring_live_store,omitempty"` // todo: remove after rhythm migration | ||
|
|
||
| Memory MemoryConfig `yaml:"memory,omitempty"` |
There was a problem hiding this comment.
MEDIUM: The PR description still contains placeholders (eg Fixes #<issue number> and unchecked checklist items). Could this be updated so reviewers/users can understand the motivation and whether this is intended to be a breaking change?
| StreamOverHTTPEnabled bool `yaml:"stream_over_http_enabled,omitempty"` | ||
| HTTPAPIPrefix string `yaml:"http_api_prefix"` | ||
| EnableGoRuntimeMetrics bool `yaml:"enable_go_runtime_metrics,omitempty"` | ||
| PartitionRingLiveStore bool `yaml:"partition_ring_live_store,omitempty"` // todo: remove after rhythm migration | ||
|
|
||
| Memory MemoryConfig `yaml:"memory,omitempty"` |
There was a problem hiding this comment.
CRITICAL: Removing the partition_ring_live_store field from the root config will break upgrades for any users still setting it, because Tempo uses yaml.UnmarshalStrict for the config file (unknown fields cause startup failure). Could we keep this field as deprecated/ignored for at least one release (similar to MetricsGeneratorClient), or otherwise provide a compatibility path so existing configs continue to load?
| StreamOverHTTPEnabled bool `yaml:"stream_over_http_enabled,omitempty"` | ||
| HTTPAPIPrefix string `yaml:"http_api_prefix"` | ||
| EnableGoRuntimeMetrics bool `yaml:"enable_go_runtime_metrics,omitempty"` | ||
| PartitionRingLiveStore bool `yaml:"partition_ring_live_store,omitempty"` // todo: remove after rhythm migration | ||
|
|
||
| Memory MemoryConfig `yaml:"memory,omitempty"` |
There was a problem hiding this comment.
MEDIUM: This is a user-visible config change (removal of partition_ring_live_store). It looks like there’s no corresponding entry in CHANGELOG.md under main / unreleased, which makes it easy for operators to miss during upgrades.
zalegrala
left a comment
zalegrala
left a comment
There was a problem hiding this comment.
Is this a breaking change?
Added to the changelog |
* chore: remove partition ring livestore config * fmt * changelog
What this PR does:
partition_ring_live_storeis no longer referenced in the code. There is now a single partition ring so this knob is no longer neededWhich issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]