Implement queue to forward traces to metrics-generator #1331
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mapno
force-pushed
the
distributor-generator-queue
branch
4 times, most recently
from
00b61b7 to
7848ea9
Compare
mapno
requested review from
joe-elliott,
annanay25,
mdisibio,
dgzlopes,
yvrhdn and
zalegrala
as code owners
joe-elliott
reviewed
Mar 16, 2022
joe-elliott
reviewed
Mar 17, 2022
There was a problem hiding this comment.
A few thoughts. I'm concerned about two things
- Each queue getting its own set of workers. It would be far simpler for all queues to share the same worker pool.
- Managing the overrides in the queueManager. It feels like there's a simpler pattern where the forwarder sees that an update occurred, creates a new queueManager for the tenant and adds it to the map with the new configuration, and then just tells the existing queueManager to drain itself.
yvrhdn
reviewed
Mar 23, 2022
mapno
force-pushed
the
distributor-generator-queue
branch
2 times, most recently
from
4f85511 to
e3afa0b
Compare
yvrhdn
reviewed
Apr 1, 2022
mapno
force-pushed
the
distributor-generator-queue
branch
from
c05c36a to
df8c71d
Compare
yvrhdn
reviewed
Apr 12, 2022
There was a problem hiding this comment.
Okay, I've left some more comments. Thanks for the replies, the structure is making more sense now 🙂
I think these are the essential metrics for monitoring monitoring distributor -> metrics-generator traffic:
tempo_distributor_forwarder_queue_length
tempo_distributor_forwarder_pushes
tempo_distributor_forwarder_dropped_pushes
tempo_distributor_metrics_generator_pushes_total
tempo_distributor_metrics_generator_pushes_failures_total
Should we rename tempo_distributor_forwarder_dropped_pushes to tempo_distributor_forwarder_pushes_failures_total?
Use eviciting queue to buffer and send push requests from the distributor to the generator. This queue will drop the oldest item in the queue if it's full and a new request is pushed.
mapno
force-pushed
the
distributor-generator-queue
branch
from
df8c71d to
ffd1033
Compare
yvrhdn
reviewed
Apr 22, 2022
yvrhdn
approved these changes
Apr 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
The distributor uses a forwarder to queue and forward requests to the metrics generators.
This queue will drop the oldest item in the queue if it's full and a new request is pushed.
Which issue(s) this PR fixes:
Contributes to #1303
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]