chore: remove span-metrics leftovers and lazy-init generator clients by javiermolinar · Pull Request #6618 · grafana/tempo

javiermolinar · 2026-03-05T18:17:56Z

What this PR does:

Now that we have decoupled the livestores from the MetricsGenerator service we can now complete the cleanup. This pr does:

Metrics generator gprc path is removed
Simplifies the initialization, SingleBinary mode use a simple callback for pushing traces to the Metrics generator
Deprecate unnecesary configs
Remove all query-path from generator and proto code
Lazy-init generator clients in the distributor. Only for single binary mode.

How this has been tested:

Compiled locally and ran single-binary and distributed docker compose modes. As expected the single binary do no longer connect to Kafka for the distributor

Which issue(s) this PR fixes:
Fixes #

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

javiermolinar · 2026-03-05T18:24:17Z

-	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


This is necessary until we implement the LiveStore grpc path

javiermolinar · 2026-03-06T09:36:54Z

@codex review

chatgpt-codex-connector

💡 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".

chatgpt-codex-connector · 2026-03-06T09:45:56Z

 	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


Gate Kafka write-path enablement on actual ingest configuration

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 👍 / 👎.

Kafka is the only deployment model now unless we are in singlebinary mode. So this is totally right

javiermolinar · 2026-03-10T15:00:38Z

@codex review

chatgpt-codex-connector

💡 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".

mapno

Love the cleanup, but I think it can be further simplified.

mapno · 2026-03-11T09:14:27Z

 	}()

 	config := NewDefaultConfig()
+	config.Target = BackendScheduler


What's this change?

Now setting up Kafka is mandatory. If we dont set it and we dont set target we get a validation error:

level=info msg="server listening on addresses" http=[::]:60047 grpc=[::]:60048 --- FAIL: TestApp_RunStop (30.00s) app_test.go:44: Checking Tempo is up... app_test.go:38: Error Trace: /Users/javimolina/grafana/tempo.fork/cmd/tempo/app/app_test.go:38 /opt/homebrew/Cellar/go/1.26.0/libexec/src/runtime/asm_arm64.s:1447 Error: Received unexpected error: failed to init module services: error initialising module: distributor: failed to create distributor: the Kafka topic has not been configured Test: TestApp_RunStop app_test.go:43: Error Trace: /Users/javimolina/grafana/tempo.fork/cmd/tempo/app/app_test.go:43 Error: Condition never satisfied Test: TestApp_RunStop FAIL exit status 1 FAIL github.com/grafana/tempo/cmd/tempo/app 30.994s

Until we make the whole target --all kafkaless we need to use a non kafka module. It can be the backendscheduler or any other one

mapno · 2026-03-11T09:39:18Z

+	KafkaConfig           ingest.KafkaConfig `yaml:"kafka_config"`
+
+	// Internal routing toggle set by app wiring (not user-configurable).
+	PushSpansToKafka bool `yaml:"-"`


Isn't this always true? Why have this param?

This is a placeholder. When we make the kafkaless mode this will be set to false instructing the distributor to not push to kafka

mapno · 2026-03-11T09:59:45Z

-	var generatorsPoolFactory ring_client.PoolAddrFunc = func(addr string) (ring_client.PoolClient, error) {
-		return generator_client.New(addr, generatorClientCfg)
+	if d.pushSpansToLocalFunc != nil {
+		d.generatorForwarder = newGeneratorForwarder(logger, d.sendToGenerators, o)


I don't think there is a strong argument to keep all this complexity. If the push fails we can log the error and continue. No need to keep channels, workers, etc.

The main advantage of this is the decoupling between the distributor and the generator execution. Not that much for error handling but for e2e latency. We can remove it, or we can simplify it to a single bounded channel to not spawn infinite goroutines

I think we should follow up this in a different pr because it has some tradeoff we need to discuss

If the concern is decoupling, a goroutine or a simple buffered channel is sufficient. The current queuing code is over-engineered for an in-process call.

I'm ok to leave it if it gets addressed in another PR.

mapno

LGTM, only a few comments

mapno · 2026-03-12T15:44:22Z

+	var pushSpansToLocalGenerator distributor.PushSpansFunc
+	if singleBinary {
+		pushSpansToLocalGenerator = func(ctx context.Context, req *tempopb.PushSpansRequest) (*tempopb.PushResponse, error) {
+			if t.generator == nil {
+				return nil, errors.New("metrics-generator not initialized")
+			}
+			return t.generator.PushSpans(ctx, req)
+		}
+	}


Thinking about more kafkaless changes, it'd be nice to instead have a call in the distributor .AddDirectPush() that appends these types of calls, because we're going to need it for the live-store as well. That way it also doesn't change the happy path.

I'm fine if you prefer it this way.

do you mind if we do it in a different pr? this is wired with the middleware and the shim and it involves some other changes

mapno · 2026-03-12T19:50:02Z

 	return t.generator, nil
 }

 func (t *App) initGeneratorNoLocalBlocks() (services.Service, error) {


I believe there is now no functional difference between the two can could be consolidated.

Yes, this is something I want to tackle on a different PR. We need first to allow the initGenerator to use a different ring and differect codecs for at least one release since this target (initGeneratorNoLocalBlocks) is already in use

…rafana#6618) * chore: route generator by deployment model and deprecate ingest.enabled * chore: remove metrics-generator query RPCs and handlers * chore: remove span-metrics leftovers and lazy-init generator clients * docs(changelog): add entry for grafana#6618 * breaking change * manifest * remove deprecated fields * fix errored test * remove generator grpc path * fmt * simplify code

javiermolinar added 5 commits March 5, 2026 18:58

chore: route generator by deployment model and deprecate ingest.enabled

bb6c664

chore: remove metrics-generator query RPCs and handlers

5f11713

chore: remove span-metrics leftovers and lazy-init generator clients

254582a

docs(changelog): add entry for grafana#6618

30d1c2f

breaking change

afc3f68

javiermolinar commented Mar 5, 2026

View reviewed changes

javiermolinar added 3 commits March 5, 2026 21:15

manifest

97244ca

remove deprecated fields

8390d81

fix errored test

552bd48

javiermolinar marked this pull request as ready for review March 6, 2026 09:35

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 March 6, 2026 09:35

chatgpt-codex-connector Bot reviewed Mar 6, 2026

View reviewed changes

mapno reviewed Mar 9, 2026

View reviewed changes

Comment thread modules/distributor/distributor.go Outdated

javiermolinar added 2 commits March 10, 2026 15:41

remove generator grpc path

a0f5a2c

fmt

f34eb0b

chatgpt-codex-connector Bot reviewed Mar 10, 2026

View reviewed changes

Comment thread cmd/tempo/app/modules.go

javiermolinar mentioned this pull request Mar 10, 2026

distributor silently drops all traces when no write path is configured #6629

Closed

mapno reviewed Mar 11, 2026

View reviewed changes

javiermolinar requested a review from mapno March 12, 2026 11:20

mapno reviewed Mar 12, 2026

View reviewed changes

simplify code

b1f434a

mapno approved these changes Mar 13, 2026

View reviewed changes

javiermolinar enabled auto-merge (squash) March 13, 2026 09:39

javiermolinar merged commit ec875f6 into grafana:main Mar 13, 2026
26 checks passed

electron0zero mentioned this pull request Mar 19, 2026

disable lagacy overrides electron0zero/tempo#9

Closed

knylander-grafana mentioned this pull request Mar 28, 2026

[DOCs] Add docs for PRs 6525, 6741, 6403, 6618, and 6359 #6818

Merged

3 tasks

javiermolinar mentioned this pull request Apr 8, 2026

Keep ingester without Kafka #6870

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: remove span-metrics leftovers and lazy-init generator clients - #6618

chore: remove span-metrics leftovers and lazy-init generator clients#6618
javiermolinar merged 11 commits into
grafana:mainfrom
javiermolinar:chore/model-driven-generator-routing

javiermolinar commented Mar 5, 2026 •

edited

Loading

javiermolinar Mar 5, 2026

javiermolinar commented Mar 6, 2026

chatgpt-codex-connector Bot left a comment

chatgpt-codex-connector Bot Mar 6, 2026

javiermolinar Mar 6, 2026

Uh oh!

javiermolinar commented Mar 10, 2026

chatgpt-codex-connector Bot left a comment

Uh oh!

mapno left a comment

mapno Mar 11, 2026

javiermolinar Mar 12, 2026

mapno Mar 11, 2026

javiermolinar Mar 12, 2026

mapno Mar 11, 2026

javiermolinar Mar 12, 2026

javiermolinar Mar 12, 2026

mapno Mar 12, 2026

mapno left a comment

mapno Mar 12, 2026

javiermolinar Mar 13, 2026

javiermolinar Mar 13, 2026

Uh oh!

mapno Mar 12, 2026

javiermolinar Mar 13, 2026

Uh oh!

Labels

2 participants

Uh oh!

Conversation

javiermolinar commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

javiermolinar commented Mar 6, 2026

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

chatgpt-codex-connector Bot Mar 6, 2026

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

javiermolinar commented Mar 10, 2026

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

mapno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Labels

2 participants

javiermolinar commented Mar 5, 2026 •

edited

Loading