[OTLP] Pool buffers used for serialization#7451
Conversation
Pool buffers used for serialization to reduce the total amount of memory used for serializing logs, metrics and traces.
Rent the buffer and return it.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7451 +/- ##
==========================================
+ Coverage 90.29% 90.31% +0.01%
==========================================
Files 287 287
Lines 15617 15665 +48
==========================================
+ Hits 14102 14148 +46
- Misses 1515 1517 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Pull request overview
This pull request improves OTLP protobuf serialization memory behavior by switching from per-exporter long-lived buffers to per-export pooled buffers, reducing LOH retention and avoiding repeated allocation churn on buffer growth while keeping the contiguous back-patching serializer design intact.
Changes:
- Introduces a dedicated
ArrayPool<byte>inProtobufSerializerand uses it for renting/returning serialization buffers (including resize paths). - Updates OTLP trace/metric/log exporters to rent a buffer per export (with a “last size” hint) and return it in a
finally. - Updates tag array serialization to pool its scratch buffer, and adds/updates tests + adds a benchmark covering pooled/cold-start scenarios.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs | Updates a serialization expansion test to use the pooled serializer buffer API. |
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs | Updates metric serialization tests/helpers to use pooled serializer buffers. |
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs | Updates log serialization expansion test to use pooled serializer buffers. |
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Implementation/Serializer/ProtobufSerializerTests.cs | Adds coverage for rent/return/grow semantics of the pooled serializer buffer helpers. |
| test/Benchmarks/Exporter/ProtobufOtlpTraceSerializationBenchmarks.cs | Adds benchmarks for steady-state vs cold-start growth vs pooled-export behavior. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporter.cs | Rents/returns a pooled buffer per export and tracks last-used capacity as a hint. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporter.cs | Same pooling pattern applied to metric export. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs | Same pooling pattern applied to log export. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/ProtobufSerializer.cs | Adds the dedicated buffer pool plus RentBuffer/ReturnBuffer; resizes via the pool. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/ProtobufOtlpTagWriter.cs | Pools the attribute-array scratch buffer and returns it after copy into the main buffer. |
Comments suppressed due to low confidence (3)
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs:947
- This test rents a pooled buffer for serialization but does not return it. Wrap the serialization + parsing + assertions in try/finally so the (possibly grown) buffer is always returned to the pool.
var buffer = ProtobufSerializer.RentBuffer(50);
var writePosition = ProtobufOtlpMetricSerializer.WriteMetricsData(ref buffer, 0, ResourceBuilder.CreateEmpty().Build(), in batch);
using var stream = new MemoryStream(buffer, 0, writePosition);
var metricsData = OtlpMetrics.MetricsData.Parser.ParseFrom(stream);
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs:1169
- CreateMetricExportRequest rents a pooled buffer but never returns it. Since parsing happens from the buffer-backed MemoryStream, return the buffer in a finally after the request has been fully built to avoid leaking pooled arrays across tests.
var buffer = ProtobufSerializer.RentBuffer(4096);
var writePosition = ProtobufOtlpMetricSerializer.WriteMetricsData(ref buffer, 0, resource, in batch);
using var stream = new MemoryStream(buffer, 0, writePosition);
var metricsData = OtlpMetrics.MetricsData.Parser.ParseFrom(stream);
var request = new OtlpCollector.ExportMetricsServiceRequest();
request.ResourceMetrics.Add(metricsData.ResourceMetrics);
return request;
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs:1848
- This test rents a pooled buffer but never returns it. Wrap serialization + parsing + assertions in try/finally so the (possibly grown) buffer is always returned to the pool.
var buffer = ProtobufSerializer.RentBuffer(50);
var writePosition = ProtobufOtlpLogSerializer.WriteLogsData(ref buffer, 0, DefaultSdkLimitOptions, new(), ResourceBuilder.CreateEmpty().Build(), batch);
using var stream = new MemoryStream(buffer, 0, writePosition);
var logsData = OtlpLogs.LogsData.Parser.ParseFrom(stream);
var request = new OtlpCollector.ExportLogsServiceRequest();
request.ResourceLogs.Add(logsData.ResourceLogs);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Return rented buffers. - Update comment.
Address code review feedback from Codex.
|
Droping here some Codex feedback, which seems resonable to me: [P1] The published benchmark results no longer describe this implementation. IncreaseBufferSize now drops every intermediate rental. This consumes the configurable pool’s limited rental slots without replenishing them and reintroduces intermediate allocations. Meanwhile, ColdStartWithGrowth repeatedly uses the same static pool, so its measurements depend on warmup/pool history. The PR’s “576 B” and “allocation churn essentially eliminated” claims must be rerun and updated after this change. |
Remove dedicated pool.
|
Feedback should be addressed now. PR description updated. |
Fixes #6396
Changes
I saw #6396 earlier while looking for something else and decided to get Claude 🐙 to investigate the proposal to improve the performance of the OTLP serializer.
Once I reviewed the changes and made some tweaks and checked the tests, I ran the benchmarks. The net result seems positive with minimal code churn/complication, so I figured this would be worth opening for review.
Below is the Claude-authored (and slightly tweaked) summary of the investigation.
Claude Summary
The OTLP exporter serialized telemetry into a per-exporter
byte[]field (new byte[750000], ~732 KB) that lived for the lifetime of the exporter and grew, when needed, by allocating a new doubled array vianew byte[]and discarding the old one. This PR keeps the proven contiguous-buffer serialization design but changes the buffer's lifecycle: buffers are now rented from a pool per export and returned afterwards, and the resize path rents/returns from the same pool instead of allocating-and-discarding.Result: the permanent per-exporter Large Object Heap retention is gone, and the cold-start/resize allocation churn is essentially eliminated — with no measurable change to the steady-state export hot path.
Investigation
The issue proposed replacing the buffer with a segmented
IBufferWriter<byte>/PooledSegmentedBufferthat exposes aReadOnlySequence<byte>. I investigated this and concluded it is not a good fit for this serializer, for two structural reasons:WriteReservedLength, used throughout the trace/log/metric serializers). This requires a contiguous, randomly-addressable buffer. A segmented writer cannot back-patch a length that straddles a segment boundary without a substantial rewrite (e.g. a two-pass / size-precomputing serializer).byte[].OtlpExporterTransmissionHandler.TrySubmitRequest(byte[], int)and the HTTP/gRPC content types all require a single array. AReadOnlySequence<byte>would force rewriting the transmission/HTTP/gRPC layers as well.Adopting the segmented approach would therefore add complexity across the serializer and transport rather than remove it — consistent with the concern raised in the SIG discussion on the issue. Benchmarks also confirmed the steady-state serialization path is already zero-allocation, so the only real costs were (a) the permanently-retained ~732 KB buffer per exporter and (b) the allocation churn when the buffer has to grow.
What this PR does instead
A smaller, contiguous-buffer-preserving change that targets the actual costs:
ArrayPool<byte>.Shared. Buffers are now rented perexport from the shared array pool via new
RentBuffer/ReturnBufferhelperson
ProtobufSerializer, and returned in afinally. This is safe because theentire send is synchronous — the buffer is fully consumed before
Exportreturns. On modern .NET the shared pool serves arrays far larger than a typical
export buffer and trims unused arrays under GC pressure, so there is no
permanent per-exporter retention and no need for a custom pool. (On
net462/netstandard2.0 the legacy shared pool does not pool arrays above ~1 MB,
so payloads larger than that allocate per export — but they are still transient
and trimmed, never permanently retained.) The gRPC compression-flag byte is now
explicitly cleared because the rented buffer is not zero-initialized.
Exportrents aright-sized buffer (using the previous export's size as a hint to avoid
resizing), serializes into it, and returns it in a
finally.IncreaseBufferSizerents the larger buffer from thepool and returns the smaller one, instead of
new byte[]+ discard. The largerbuffer is swapped in before the smaller one is returned so growth always
succeeds and no intermediate buffer is dropped.
OtlpArrayTagWriternow rents itsscratch buffer from the same pool and returns it once the array contents are
copied into the main buffer, removing another
ThreadStatic-retained buffer.There is one intentional trade-off: the exception-driven resize retry (full
re-serialization after a grow) is unchanged. Removing it would require an invasive
rewrite of all serializers to thread a growable writer through every call; the
cost it represents is a one-time warm-up cost, so it was left as-is.
Benchmarks
Full export path —
OtlpHttpExporterBenchmarks(this branch vsmain)10,000-span (~4 MB) payloads per batch; net10.0; memory diagnoser enabled.
mainMeanmainAllocatedNo regression. Durations are within the (large) network-mock noise and allocations are identical. Rent/return-per-export adds no measurable overhead, and the dedicated pool serves the ~4 MB (>1 MB) payload with zero per-export buffer allocation — which
ArrayPool<byte>.Sharedcould not have done.Serialization path —
ProtobufOtlpTraceSerializationBenchmarks(5,000 spans, >1 MB)Re-measured on net10.0 after switching to
ArrayPool<byte>.Shared.The exporter model (
PooledExport) allocates 0 B even for a >1 MB payload,because the shared pool serves and reuses the large buffer. The
cold-start/resize path drops from ~5.25 MB of churn to ~800 B, and there is no
permanent per-exporter buffer retention (buffers return to the shared pool between
exports and are trimmed when idle).
Net effect
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)