Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 27, 2025

Conversation

flxbk
Copy link
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

CLAassistant commented Feb 12, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
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 🙂

Copy link
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.

@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
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.

Distributor: {Common, IngesterRing, MetricsGeneratorRing, PartitionRing},
Ingester: {Common, Store, MemberlistKV, PartitionRing},
MetricsGenerator: {Common, OptionalStore, MemberlistKV, PartitionRing},
MetricsGeneratorNoLocalBlocks: {Common, GeneratorRingWatcher},
Copy link
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
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
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
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
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
15 checks passed
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