chore: remove ingest.enabled config#6873
Conversation
javiermolinar
requested review from
carles-grafana,
electron0zero,
ie-pham,
knylander-grafana,
mapno,
mattdurham,
mdisibio,
oleg-kozlyuk-grafana,
ruslan-mikhailov,
stoewer,
yvrhdn and
zalegrala
as code owners
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy ingest.enabled YAML config from Tempo’s ingest configuration and updates code/docs/examples to rely on Kafka ingest configuration (notably ingest.kafka.topic) as the enablement signal.
Changes:
- Remove
ingest.enabledfrompkg/ingest.Configand update ingester code to use a helper (ingestStorageEnabled) instead of the removed field. - Add strict YAML unmarshal tests to ensure legacy
enabled(and other unknown fields) are rejected and that defaults are preserved. - Update docs/examples/manifests and add a changelog entry documenting the breaking config removal.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/ingest/config.go | Removes the deprecated Enabled field from ingest config. |
| pkg/ingest/config_test.go | Adds strict YAML unmarshal tests for legacy/unknown fields and default preservation. |
| modules/ingester/ingester.go | Switches gating from cfg.IngestStorageConfig.Enabled to ingestStorageEnabled(...). |
| modules/ingester/downscale.go | Updates downscale handler gating to use ingestStorageEnabled(...). |
| modules/ingester/config.go | Introduces ingestStorageEnabled helper (topic-based enablement). |
| modules/generator/AGENTS.md | Removes mention of deprecated ingest.enabled. |
| integration/util/config-base.yaml | Drops ingest.enabled from integration base config. |
| example/nomad/tempo-distributed/config.yml | Drops ingest.enabled from example config. |
| example/docker-compose/single-binary/tempo.yaml | Drops ingest.enabled from example config. |
| example/docker-compose/multitenant/tempo.yaml | Drops ingest.enabled from example config. |
| example/docker-compose/distributed/tempo.yaml | Drops ingest.enabled from example config. |
| example/docker-compose/debug/tempo.yaml | Drops ingest.enabled from example config. |
| docs/sources/tempo/configuration/manifest.md | Removes ingest.enabled from the published config manifest. |
| CHANGELOG.md | Adds a [CHANGE] entry for removing ingest.enabled. |
| func ingestStorageEnabled(cfg ingest.Config) bool { | ||
| return cfg.Kafka.Topic != "" |
There was a problem hiding this comment.
ingestStorageEnabled takes ingest.Config by value, which copies the full KafkaConfig (including secret fields and other nested config) each time it’s called. Could this take *ingest.Config (or just the topic string) instead to avoid unnecessary copying and make it clear the helper is read-only?
| func ingestStorageEnabled(cfg ingest.Config) bool { | |
| return cfg.Kafka.Topic != "" | |
| func ingestStorageEnabled(cfg *ingest.Config) bool { | |
| return cfg != nil && cfg.Kafka.Topic != "" |
There was a problem hiding this comment.
This is just to make this thing build. Ingesters are about to disappear
| func TestConfigUnmarshalYAMLRejectsLegacyEnabled(t *testing.T) { | ||
| var cfg Config | ||
|
|
||
| err := yaml.UnmarshalStrict([]byte(` | ||
| enabled: false | ||
| kafka: | ||
| address: localhost:9092 | ||
| topic: tempo | ||
| `), &cfg) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "field enabled not found") | ||
| } | ||
|
|
||
| func TestConfigUnmarshalYAMLStrictUnknownField(t *testing.T) { | ||
| var cfg Config | ||
|
|
||
| err := yaml.UnmarshalStrict([]byte(` | ||
| kafka: | ||
| address: localhost:9092 | ||
| topic: tempo | ||
| unknown: true | ||
| `), &cfg) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "field unknown not found") | ||
| } |
There was a problem hiding this comment.
These tests assert a specific YAML strict-unmarshal error string ("field … not found"). The exact wording can vary across yaml library versions, which can make the tests brittle. Would you consider asserting on the error type/details (eg *yaml.TypeError contents) or checking for a smaller invariant substring (like just the field name) instead?
| * [CHANGE] Deprecate metrics-generator no-local-blocks [#6707](https://github.com/grafana/tempo/pull/6707) (@javiermolinar) | ||
| * [CHANGE] Own local block and partition ring helpers [#6808](https://github.com/grafana/tempo/pull/6808) (@javiermolinar) | ||
| * [CHANGE] Track invalid trace and span id discards [#6799](https://github.com/grafana/tempo/pull/6799) (@javiermolinar) | ||
| * [CHANGE] Remove ingest.enabled config [#6873](https://github.com/grafana/tempo/pull/6873) (@javiermolinar) |
There was a problem hiding this comment.
Sure, I'll change it
| * [ENHANCEMENT] Expose MinIO retry settings via S3 config [#6561](https://github.com/grafana/tempo/pull/6561) (@rwhitty) | ||
| * [BUGFIX] Fix integer overflow in query parameters by using `strconv.ParseUint` instead of `strconv.Atoi`/`strconv.ParseInt` for unsigned integer fields. [#6612](https://github.com/grafana/tempo/pull/6612) (@bejaratommy) | ||
|
|
||
| * [CHANGE] **BREAKING CHANGE** Centralize block and WAL config: `block_builder` and `live_store` now always use `storage.trace.block` settings; per-module block config fields are removed. [#6647](https://github.com/grafana/tempo/pull/6647) (@stoewer) |
There was a problem hiding this comment.
The ## main / unreleased section now has an extra blank line between list items (line 4), and the first entry is an [ENHANCEMENT] that sits above the [CHANGE] block. Could we remove the blank line and keep entries grouped/sorted by type (CHANGE/FEATURE/ENHANCEMENT/BUGFIX) to match the checklist guidance and the rest of the file’s structure?
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]