feat: add an Aggregator for Traces and Stats #534
Merged
Conversation
This file contains hidden or 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
method to remove the boilerplate to start a trace agent, and only return the flushers, channel, and processor
from `manual_flush` to `flush` and `flush` to `send`
its `3.2` MB
generic `prost::Message` aggregator which batches given a `max_content_size_bytes`
a flusher should only flush, not receive messages, we abstract that from here
now the `TraceAgent` spins up a task which receives the stats so that they get properly aggregated
also modify the existing aggregator to be a stats agregator
also modified the naming import for `StatsFlusher`
it doesnt need the flushers to work, so removing that from it, and spawning own tasks for the data channels
duncanista
commented
Jan 30, 2025
it seems adding `prost::Message` import allows us to have the methods directly from the message we are treating with
alexgallotta
reviewed
Feb 3, 2025
alexgallotta
reviewed
Feb 3, 2025
| trace_aggregator: Arc<Mutex<trace_aggregator::TraceAggregator>>, | ||
| trace_processor: Arc<dyn trace_processor::TraceProcessor + Send + Sync>, | ||
| trace_flusher: Arc<dyn trace_flusher::TraceFlusher + Send + Sync>, | ||
| stats_aggregator: Arc<Mutex<stats_aggregator::StatsAggregator>>, |
Contributor
There was a problem hiding this comment.
these types are becoming a pain to handle. Can we start using type aliases?
Contributor
Author
There was a problem hiding this comment.
Maybe? We'd have to define them at the top for all of them, maybe not in this PR.
alexgallotta
reviewed
Feb 3, 2025
| buffer: Vec<SendData>, | ||
| } | ||
|
|
||
| impl Default for TraceAggregator { |
Contributor
There was a problem hiding this comment.
all this file is duplicate of trace stats aggregator, maybe dedup?
Contributor
Author
There was a problem hiding this comment.
Not really, this one has a different type, and they have different ways of handling how the data is treated.
Trace payload the length in the struct, whereas Stats Payload is a proto where we use method from the trait Message to get the length of the payload.
astuyve
reviewed
Feb 4, 2025
astuyve
reviewed
Feb 4, 2025
astuyve
approved these changes
Feb 4, 2025
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?
Adds aggregators for Traces and Stats.
Motivation
Large trace payloads are being dropped, because they're rejected by the backend.
Also SVLS-6250
Test