-
Notifications
You must be signed in to change notification settings - Fork 601
[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
Conversation
There was a problem hiding this 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 🙂
There was a problem hiding this 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.
9a79e48
to
9f1d931
Compare
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}, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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
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. |
6800e7f
to
2f8a759
Compare
Added changelog and rebased to resolve conflicts in |
What this PR does:
This PR makes it possible to run the metrics-generation features of metrics-generator (i.e.
spanmetrics
andservice-graph
processors, but notlocalblocks
) on a different Kafka-compatible data source. It introducestwothree new configuration options to achieve this:DisableLocalBlocks
: this defaults tofalse
, when set tofalse
there's no change in behavior. When this is set totrue
, metrics-generator will not run thelocalblocks
processor even when it's configured for a tenant.DisableGRPC
: this defaults tofalse
as well. When it'strue
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).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 containptrace.Traces
instead of thetempopb.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
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]