Repository navigation
Serialize traces into a chunked buffer instead of one 5 MiB array - #13096
Conversation
MsgPackWriter.format() only caught BufferOverflowException. Any other throwable from a mapper (a concurrent modification, a bug) left the partial message in the buffer, and the next mark() absorbed it, so the whole payload failed to decode and every trace in it was lost. Add StreamingBuffer.rewind(), which drops everything written since the last mark. On a failure, format() now rewinds, sends the completed messages as their own payload (mapper state such as v1's string table may still refer to the rewound bytes), resets the mapper and rethrows. CiTestCovMapperV2 counted events as it wrote them, so a trace that failed or overflowed part-way left the payload's header count too high. It now counts once the whole trace is written, as LLMObsSpanMapper does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A toolbox buffer for serializing payloads of a few MiB without one large array. Each chunk is a regular-sized object (at most 256 KiB, header included) on G1, ZGC and Shenandoah, where a single multi-MiB array is humongous or otherwise a large-object allocation. - typed puts never straddle chunks, so the fast path is one bounds check - mark()/rewind() keep only completed messages - running out of capacity never throws: the buffer fails, discards writes, and the next mark() rewinds and returns false - chunks are allocated on first use, and reset() keeps only the chunks the completed messages needed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The trace writer kept a single 5 MiB array (2 + 2 MiB for v0.5) for its lifetime: a humongous object on heaps up to 16 GB, pinned in old gen and counted toward G1's concurrent-cycle trigger. PayloadDispatcherImpl now writes into a ChunkedStreamingBuffer: 256 KiB chunks, allocated on first use, trimmed after each send to what the last payload needed. - StreamingBuffer.mark() returns false when a message didn't fit and was rewound, so the chunked buffer never throws for overflow; format() handles that the same way as BufferOverflowException. Existing buffers return true and keep throwing. - ChunkedBuffer counts capacity in data bytes, so payload sizes and counts are unchanged. - Payload holds its body as parts, and writeTo no longer consumes it, so a payload can be written again. This also fixes LLM Obs payloads repeating their headers on a partial channel write. PetClinic at 128m (JDK 17, G1, 1 MB regions): live set -5 MB, heap used after full GC -5.9 MB, humongous regions 11 -> 5. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Buffer Add J13: an array object over 256 KiB is a large-object allocation on every region-based collector, and humongous on G1 with 1 MB regions past 512 KiB. A retained one adds to the app's live set for the tracer's lifetime (flag with confidence, SEV-2); a per-flush one takes the humongous slow path (flag as measure, SEV-3). Points to ChunkedBuffer and records the power-of-two header trap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- ChunkedBuffer.reset() keeps the chunks the last payload used plus half the surplus the previous reset kept, so payloads that vary in size don't free and reallocate chunks every cycle; a burst drains over a few resets. - Payload.writeFully replaces the hand-written write loops in the payload classes; CI Visibility payloads write duplicates of their header so a payload can be written again. - Benchmarks share DDSpanHelper.singleRootTrace; TraceMapperBenchmark uses the shared BlackholeBuffer instead of a nested copy. - J13: humongous threshold at 512 KiB for G1; 256-512 KiB and per-flush arrays are flag-as-measure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05993fdb9b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Silent chunked-buffer overflow advances mapper state for rejected traces, producing malformed CI Visibility and LLM Observability payload counts. It also clears cached LLM processor decisions, causing processors to run again during retry.
🤖 Bits Code Review · Commit 05993fd
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
With sticky failure, a message that overflowed let Mapper.map() run to the end before mark() rejected it. Mappers rely on overflow interrupting them: CiTestCycleMapperV1, CiTestCovMapperV2 and LLMObsSpanMapper count events at the end of map(), so the rejected trace was counted in the payload flushed before the retry. Its header then claimed more events than its body held, and the intake would reject it. LLMObsSpanMapper also cleared its processor decisions, so the retry ran the user's processor a second time. ChunkedBuffer gets an onCapacityExceeded() hook, a no-op by default (the toolbox buffer stays sticky). ChunkedStreamingBuffer overrides it to throw BufferOverflowException, matching FlushingBuffer, so mappers are interrupted exactly as before. Tests run the CI test cycle, CI coverage and LLM Obs overflow paths through the chunked buffer; they fail without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…LLMObs example Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The J13 retained-size check stays; the LLMObs example goes back to how it reads on master. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
The expected merge time in
|
What Does This Do
Replaces the trace writer's single 5 MiB serialization array with a chunked buffer, so the writer no longer holds a humongous array for the tracer's lifetime.
ChunkedBuffer(internal-api, toolbox): a write buffer made of chunks whose whole array object is at most 256 KiB, so no chunk is ever humongous on G1, ZGC or Shenandoah.reset(), keeps the chunks the last payload used plus half of any earlier surplus, so a burst drains over a few flushes.mark()rewinds the message and returnsfalse. A subclass can throw instead by overridingonCapacityExceeded().ChunkedStreamingBuffer(communication): theStreamingBufferused byPayloadDispatcherImpl, which also serves CI Visibility and LLM Obs.BufferOverflowExceptionat the first write that doesn't fit, likeFlushingBuffer. Mappers rely on being interrupted there: CI Visibility and LLM Obs update event counts and processor decisions at the end ofmap(), so lettingmap()finish would count the rejected trace in the flushed payload.StreamingBuffer.mark()returns a boolean to matchChunkedBuffer. It's alwaystruefor buffers that throw on overflow, which is every buffer used withMsgPackWriter.format().Payloadholds its body as a list of parts (one per chunk).writeTono longer consumes it, so a payload can be written again.format()now rewinds to the last mark, sends the completed traces as their own payload, and resets the mapper.BufferOverflowExceptionwas handled. Anything else left a partial trace in the buffer, the nextmark()absorbed it, and the agent rejected the whole payload, losing every trace sent with it.CiTestCovMapperV2counting events before a trace was fully written, which could make the header count too high when a trace failed or overflowed partway.byte[]buffers, and suggestChunkedBuffer. The guide's LLM Obs example now separates per-call cost, which a long LLM call absorbs, from retained size, which it doesn't.Motivation
The 5 MiB array (2 + 2 MiB for v0.5) is a G1 humongous object on heaps up to 16 GB. It's pinned in old gen, never compacted, and counted toward the concurrent-cycle trigger, a permanent addition to the app's live set.
Spring PetClinic at 128m (JDK 17, G1 with 1 MB regions, real Agent; class histogram after a full GC):
The buffer was about 27% of the agent's live-set overhead on this app.
Serialization CPU (
PayloadBufferBenchmark, v0.4, 5 MiB buffer, arm64): about +7% / +12% / +22% ns per formatted trace on JDK 8 / 17 / 21. This runs on the background writer thread, off the request path. It's an intentional trade for app-heap health.Additional Notes
FlushingBuffer).OkHttpUtils.ByteBufferRequestBodystill consumes its buffers onwriteTo, which matters if OkHttp retries a request after the body has started sending. Separate fix.Jira ticket: APMLP-2114
🤖 Generated with Claude Code