perf(trace-utils)!: span pool to reduce alloc churn on the send path - #2382
Open
paullegranddc wants to merge 14 commits into
Open
perf(trace-utils)!: span pool to reduce alloc churn on the send path#2382paullegranddc wants to merge 14 commits into
paullegranddc wants to merge 14 commits into
Conversation
# Changes Pass a PooledChunks argument around to the trace exporter that can contain a reference to the span pool. Whenever spans get dropped, the chunks will get returned to the passed pool. Previous use cases that don't want to use the Pool can call PooledChunks::unpooled. # Motivation Allocation of collection is one of the most expensive things we perform in the tracer (with allocation of Strings). Being able to reuse allocated capacity should improve performance (to be benchmarked in actual code).
paullegranddc
changed the title
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 912ed46 | Docs | View more details | Give us feedback! |
Contributor
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
BenchmarksComparisonBenchmark execution time: 2026-09-04 17:35:31 Comparing candidate commit 912ed46 in PR branch Found 4 performance improvements and 9 performance regressions! Performance is the same for 156 metrics, 10 unstable metrics.
|
ekump
approved these changes
Aug 25, 2026
ekump
left a comment
ekump
left a comment
Collaborator
There was a problem hiding this comment.
My only real concern is the potential flaky test, otherwise LGTM
| return Ok(AgentResponse::Unchanged); | ||
| } | ||
| return self.send_otlp_traces_inner(traces, config).await; | ||
| // The OTLP mapper transforms spans into a different representation and consumes |
Collaborator
There was a problem hiding this comment.
I don't think this comment is accurate?
Suggested change
| // The OTLP mapper transforms spans into a different representation and consumes | |
| // The OTLP mapper borrows the spans and builds a separate OTLP | |
| // representation, the original spans are recycled to the pool when `traces` drops. |
| /// if span usage spikes, and then goes down. | ||
| fn drop_policy() -> bool { | ||
| const PCT_OF_SPANS_RETURNED_DROPPED: f64 = 0.1; | ||
| rand::thread_rng().gen_bool(PCT_OF_SPANS_RETURNED_DROPPED) |
Collaborator
There was a problem hiding this comment.
This is going to get called on every span being flushed, right? Isn't thread_rng() going to be expensive? Could you do it at the chunk level instead? Or, does it have to be random at all? Could you just drop every X spans deterministically in order to shrink the pool?
Collaborator
Author
There was a problem hiding this comment.
I changed it to run on every "chunk", which is less often and I changed the random generator to rand::SmallRng (with the state living in a thread local) for performance.
After a bit of benchmarking, the cost is kind of negligeable (a few %) vs a no-op
| mut traces: Vec<Vec<Span<T>>>, | ||
| mut traces: PooledChunks<'_, T>, | ||
| ) -> Result<AgentResponse, TraceExporterError> { | ||
| // `traces` is a `PooledChunks`: keeping it owned (rather than moving its inner `Vec` |
Collaborator
There was a problem hiding this comment.
Not sure if this is the right place, but should we document somewhere that only sampled spans make it to the pool?
Collaborator
Author
There was a problem hiding this comment.
Updated so that drop_chunks and filter_traces now return dropped spans and chunks to the pool
Comment on lines
226
to
227
Collaborator
There was a problem hiding this comment.
I think this comment is stale now, span isn't consumed anymore.
| fn returned_spans_are_recycled_and_reset() { | ||
| let pool = SpanPool::<crate::span::BytesData>::new(100); | ||
| { | ||
| // No drop policy control here, but with a single span it is very likely retained. |
Collaborator
There was a problem hiding this comment.
"very likely" is another way of saying flaky.
What is this test actually covering? won't s.name be the same whether it comes from the pool or is a new span?
Should we have a DropPolicy that's injectable? Something like...
#[derive(Debug)]
enum DropPolicy {
Random(f64),
EveryN { n: usize, counter: AtomicUsize }, // deterministic, no global
Never, // tests only?
}
impl DropPolicy {
fn should_drop(&self) -> bool {
match self {
DropPolicy::Random(p) => rand::thread_rng().gen_bool(*p),
DropPolicy::EveryN { n, counter } =>
counter.fetch_add(1, Ordering::Relaxed) % n == 0,
DropPolicy::Never => false,
}
}
}
#[derive(Debug, Clone)]
pub struct SpanPool<T: TraceData> {
queue: crossbeam_channel::Sender<Span<T>>,
receiver: crossbeam_channel::Receiver<Span<T>>,
drop_policy: Arc<DropPolicy>,
}
Collaborator
Author
There was a problem hiding this comment.
So the thing is, this test is not "flaky" in the sense it fail from time to time. If there were a bug it could pass.
I added more spans so that the likelihood it passes flakily is ridiculously low.
Collaborator
|
Should we have benchmark tests too? |
Collaborator
Author
Added benchmarks, as it turns out beating the allocator is not that easy and I had to add a thread local cache from which to pull chunks to make the perf gain obvious. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Motivation
Allocation is expensive, and spans use a lot of small collections (meta, metrics, span links, events...)
Being able to recycle the allocation should be beneficial in term of perf.
This PR adds a span pool to the trace exporter to do this
Changes
Add SpanPool trace utils
Add PooledChunks through the send path in libdd-data-pipeline: