Skip to content

[Metrics Generator] Allow running on a different source of data#4686

Merged
mdisibio merged 8 commits intografana:mainfrom
flxbk:mgen-secondary-data-source
Feb 27, 2025
Merged

[Metrics Generator] Allow running on a different source of data#4686
mdisibio merged 8 commits intografana:mainfrom
flxbk:mgen-secondary-data-source

Conversation

@flxbk
Copy link
Copy Markdown
Contributor

@flxbk flxbk commented Feb 12, 2025

What this PR does:

This PR makes it possible to run the metrics-generation features of metrics-generator (i.e. spanmetrics and service-graph processors, but not localblocks) on a different Kafka-compatible data source. It introduces two three new configuration options to achieve this:

  1. DisableLocalBlocks: this defaults to false, when set to false there's no change in behavior. When this is set to true, metrics-generator will not run the localblocks processor even when it's configured for a tenant.

  2. DisableGRPC: this defaults to false as well. When it's true the gRPC server setup is skipped and metrics generator doesn't join the generator ring (which it only needs to do to make itself available to be queried via gRPC, so when gRPC is disabled joining the ring makes no sense).

  3. Codec: this controls what data type metrics generators expect to find in the Kafka records they consume. This PR adds support for consuming records that contain ptrace.Traces instead of the tempopb.PushBytesRequest that's already supported.

This PR also introduces a way of making metrics generator watch a configurable partition ring (currently watching the ingester ring is hardcoded).

Which issue(s) this PR fixes:
Fixes n/a

Checklist

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

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 12, 2025

CLA assistant check
All committers have signed the CLA.

Comment thread modules/ingester/ingester.go
Comment thread pkg/ingest/reader_client.go Outdated
Copy link
Copy Markdown
Contributor

@yvrhdn yvrhdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me but I'll let someone with more experience on the new Kafka ingest and local-blocks processor look into it 🙂

Comment thread modules/generator/config.go Outdated
Comment thread cmd/tempo/app/modules.go Outdated
Comment thread cmd/tempo/app/modules.go Outdated
Comment thread cmd/tempo/app/modules.go Outdated
Comment thread cmd/tempo/app/modules.go Outdated
Comment thread modules/generator/config.go Outdated
Comment thread modules/generator/encoding_test.go Outdated
Comment thread modules/generator/encoding.go Outdated
Comment thread modules/generator/encoding.go Outdated
Comment thread modules/generator/generator.go Outdated
Comment thread pkg/ingest/reader_client.go Outdated
Comment thread cmd/tempo/app/modules.go Outdated
Comment thread cmd/tempo/app/modules.go Outdated
Comment thread cmd/tempo/app/modules.go Outdated
Comment thread modules/generator/encoding.go Outdated
Comment thread pkg/ingest/reader_client.go Outdated
Comment thread pkg/ingest/config.go Outdated
Copy link
Copy Markdown
Contributor

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done an in-depth review. Still have some comments.

Comment thread pkg/ingest/reader_client.go Outdated
Comment thread cmd/tempo/app/modules.go Outdated
Comment thread modules/generator/encoding.go Outdated
Comment thread pkg/ingest/config.go Outdated
@flxbk flxbk force-pushed the mgen-secondary-data-source branch 2 times, most recently from 9a79e48 to 9f1d931 Compare February 24, 2025 13:13
@flxbk
Copy link
Copy Markdown
Contributor Author

flxbk commented Feb 24, 2025

I have rebased to resolve conflicts with #4721 and tried to address all the PR feedback that has come in so far.

Comment thread modules/generator/config.go Outdated
Comment thread cmd/tempo/app/modules.go
Distributor: {Common, IngesterRing, MetricsGeneratorRing, PartitionRing},
Ingester: {Common, Store, MemberlistKV, PartitionRing},
MetricsGenerator: {Common, OptionalStore, MemberlistKV, PartitionRing},
MetricsGeneratorNoLocalBlocks: {Common, GeneratorRingWatcher},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependence between "no local blocks" and the partition ring watcher don't make sense to me. Shouldn't this be something like MetricsGeneratorPartitionRing

Copy link
Copy Markdown
Contributor Author

@flxbk flxbk Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a different PR comment, @mdisibio suggested we could use a separate target for this metrics generator mode. That is what I did here, however that new mode still needs a way to watch a (configurable) partition ring, hence the dependency on GeneratorRingWatcher.

Copy link
Copy Markdown
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the amount of work put into this. The final result with the dedicated target is working well to keep all of the unique bits together, and I think it gives a clean separation between the modes, so that we can continue to iterate on naming or other things without breaking config or deployment changes. LGTM

@flxbk
Copy link
Copy Markdown
Contributor Author

flxbk commented Feb 26, 2025

Thanks for the review and all the suggestions that have helped me make this better!

I haven't added a changelog entry yet to avoid eternal rebases/conflicts with other entries, but happy to do that before this goes in.

@flxbk flxbk force-pushed the mgen-secondary-data-source branch from 6800e7f to 2f8a759 Compare February 26, 2025 19:58
@flxbk
Copy link
Copy Markdown
Contributor Author

flxbk commented Feb 26, 2025

Added changelog and rebased to resolve conflicts in changelog.md.

@mdisibio mdisibio merged commit 9064e84 into grafana:main Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants