chore: remove span-metrics leftovers and lazy-init generator clients#6618
chore: remove span-metrics leftovers and lazy-init generator clients#6618javiermolinar wants to merge 10 commits intografana:mainfrom
Conversation
cmd/tempo/app/modules.go
Outdated
| t.cfg.Distributor.KafkaWritePathEnabled = t.cfg.Ingest.Enabled // TODO: Don't mix config params | ||
| t.cfg.Distributor.KafkaWritePathEnabled = false | ||
| t.cfg.Distributor.PushSpansToKafka = true | ||
| t.cfg.Distributor.PushSpansToGenerator = singleBinary |
There was a problem hiding this comment.
This is necessary until we implement the LiveStore grpc path
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
|
@codex review |
![chatgpt-codex-connector[bot]](https://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: 552bd48166
ℹ️ 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".
| t.cfg.Distributor.KafkaConfig = t.cfg.Ingest.Kafka | ||
| t.cfg.Distributor.IngesterWritePathEnabled = false | ||
| t.cfg.Distributor.KafkaWritePathEnabled = t.cfg.Ingest.Enabled // TODO: Don't mix config params | ||
| t.cfg.Distributor.PushSpansToKafka = true |
There was a problem hiding this comment.
initDistributor now forces PushSpansToKafka = true for every target, which makes distributor.New() always validate Kafka settings and fail when ingest.kafka.topic is unset. With the current defaults (ingest.enabled: false, empty topic), this turns the default all target into a startup error path (ErrMissingKafkaTopic) even for configs that previously booted without Kafka; the test change to BackendScheduler masks this regression rather than fixing it. Please only enable Kafka routing when the deployment/config is actually Kafka-backed (or provide a non-empty default topic).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Kafka is the only deployment model now unless we are in singlebinary mode. So this is totally right
modules/distributor/distributor.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| // In single-binary mode we also push directly to metrics-generator over gRPC, |
There was a problem hiding this comment.
If this functionality is only for monolithic, let's avoid gRPC and push directly via a func
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f34eb0b7f2
ℹ️ 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".
| t.cfg.Distributor.KafkaConfig = t.cfg.Ingest.Kafka | ||
| t.cfg.Distributor.IngesterWritePathEnabled = false | ||
| t.cfg.Distributor.KafkaWritePathEnabled = t.cfg.Ingest.Enabled // TODO: Don't mix config params | ||
| t.cfg.Distributor.PushSpansToKafka = true | ||
|
|
There was a problem hiding this comment.
initDistributor now enables Kafka writes but no longer clears distributor.ingester_write_path_enabled, so legacy configs that still set this deprecated flag to true will re-enable sendToIngestersViaBytes in Distributor.PushTraces while Kafka remains active. In current app wiring this means each push can be sent down two write paths (or fail if the live-store endpoint doesn’t serve PushBytesV2), causing duplicate ingestion or request failures for existing deployments carrying old config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There are no ingesters anymore
2 participants
What this PR does:
Now that we have decoupled the livestores from the MetricsGenerator service we can now complete the cleanup. This pr does:
How this has been tested:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]