feat(telemetry): trace context compaction - #1233
Conversation
Long conversations grow session history without bound. Every turn
re-sends the whole history, so cost and latency climb with conversation
length and a long enough session eventually exceeds the model's context
window. Compaction replaces ranges of older events with a
model-generated summary so the substance survives but the token count
does not.
This adds the data model and the library. Nothing calls it yet; the
runner and the contents processor are wired up in a follow-up so that
change can be reviewed on its own.
- session.EventCompaction records the timestamp range a summary
covers plus the summary content, attached to a new event via
EventActions.Compaction. Covered events are never modified or
deleted, so a session stays complete and auditable.
- session/compaction holds what a user touches: Config, the
Summarizer extension point, and the default LLMSummarizer.
- internal/compactioninternal holds the algorithms: window selection,
summary substitution and function-call recovery. Keeping them
unexported leaves them free to change.
The summarizer renders thoughts and tool traffic as well as text, since
those carry the reasoning and evidence a text-only summary would lose.
Testing: unit tests throughout, using fake summarizers and models so
nothing touches the network.
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
The span is a genuine improvement over no telemetry at all, and the three-state
design is the right shape. Two things undercut it.
1. The cross-language claim is not true as written. The PR says dashboards
written against one implementation work against the others, and that sentence
also ships as a doc comment at compaction.go:82-85. But
gen_ai.compaction.start_timestamp / end_timestamp are emitted here as
RFC3339Nano strings (internal/telemetry/compaction.go:34,:136-137), while
adk-python puts a float of epoch seconds under the same keys
(event_actions.py:68-72, assigned raw in tracing.py:505-509). Same key,
incompatible OTLP value type — a strict-typed backend drops one or the other, so
a shared dashboard cannot work. Two smaller issues ride along: the timestamps
are not .UTC()-normalised, so the host timezone reaches the wire (observed
2026-01-01T01:00:00+01:00), and RFC3339Nano sorts lexicographically in the
opposite order to time for mixed offsets. Emitting epoch seconds matches Python
and fixes all three.
2. The three states are not actually distinguishable. Result attributes are
recorded on the error path too, so one span reports both "the summarizer failed"
and "it produced summary X" — and result_event_id there names an event that was
discarded. Separately, "ran and declined" is signalled only by the absence of
attributes, and for the shipped LLMSummarizer that absence is exactly what a
degraded model response produces — so a summarizer failing every call looks like
a 0% error rate. A panic leaves the span status Unset.
Two follow-ups worth considering while the file is open: the span is emitted as
its own trace root with no invocation_id (3 distinct trace ids in a single
run), so a failure cannot be traced back to the turn that caused it; and the
summarizer's UsageMetadata is on the very event handed to the recorder but is
dropped, which is why "how much it was shrinking" is still unanswerable. The
tests pin attribute names but not values — an RFC1123 format change ships green,
along with 7 other mutations.
Additional findings
[minor] The summarizer's own model call is untraced: session/compaction/llm_summarizer.go:122 calls s.model.GenerateContent(ctx, req, false) directly, bypassing the repo's only instrumentation site (StartGenerateContentSpan is declared at internal/telemetry/telemetry.go:92 and has exactly one caller, internal/llminternal/base_flow.go:844). That call is billable and appears in no generate_content span, so every cost and latency dashboard built on those spans under-counts by exactly the compaction traffic — end-to-end with the default summarizer, a run makes 3 model calls but produces 2 generate_content spans and 1 compact_events span, and the summarizer's model, finish reason and per-call latency are recorded nowhere. llm_summarizer.go is not touched by this change, which is why this is minor rather than a defect of the diff, but it is a gap a change whose stated purpose is making compaction observable is well placed to close. At minimum, put gen_ai.request.model and the token counts on the compact_events span; better, route the summarizer's call through the same instrumentation so it becomes a child generate_content span.
Four of these are data-loss or silent-failure defects in the core model,
so they are fixed here rather than layered over in the follow-ups.
Coverage is recorded as an inclusive timestamp range, but the window was
built by filtering while walking backwards, which could skip an event
that still fell inside the resulting range. Such an event was dropped
from the prompt without ever being summarized. The window is now a
contiguous slice between invocation boundaries, so a hole is
unexpressible, and the trim is pulled back off a timestamp tie for the
same reason. A test asserts the invariant directly: everything inside a
summary's range was in the set summarized.
A response carrying content with no parts was accepted as a summary,
which erased the covered turns and replaced them with nothing. That
shape is what a blocked or candidate-less generation produces, so this
was reachable rather than theoretical. Empty summaries are now rejected,
and a summarizer that produces nothing usable returns an error carrying
the finish reason instead of reporting "nothing to compact" -- which had
made a summarizer failing every call indistinguishable from an idle one.
On the Agent Engine backend a summary reached no storage slot at all: it
carries its content only on Actions.Compaction, and eventNeedsRawEvent
did not list it. The summarizer was called and billed, the result was
discarded, and the same range was re-summarized on every later trigger.
The shared session suite now asserts the round trip so no backend can
regress this quietly again.
Also:
- A function call with no ID opened no obligation, so the trim that
keeps a call with its response silently did not fire. It is now an
unconditional obligation, erring toward not summarizing.
- A tool call that never gets a response no longer stops compaction
for the rest of the session. The window steps past the blocked head
and summarizes the remainder, leaving the pending call raw.
- A Summarizer returning an event with no compaction record is
rejected rather than appended as a conversational turn.
- Tool output can no longer forge turns in the summarizer transcript.
- Package docs no longer imply the sliding window bounds prompt
growth. It is a constant-factor reduction; tail retention is what
bounds it.
Testing: every fix has a regression test, and each was confirmed to fail
against the previous behaviour rather than passing vacuously. Build,
race tests, lint and tidy are clean. One unrelated pre-existing failure
in telemetry.TestConfigureExporters (an OpenTelemetry schema version
conflict from the GCP detector bump) reproduces on the integration
branch with no compaction code present.
982b745 to
17eda0c
Compare
52e1a59 to
40a9484
Compare
|
Triaged all fifteen against the current code rather than replying from the summary. One is already fixed, one is partly overtaken by upstream changes, and thirteen are open. Nothing in this PR's own code has been revised yet, so that is expected. Two replies posted inline where the ground has moved under a comment ( The blocker ( Same for
|
42c8cf8 to
cf699f0
Compare
|
All nine agreed decisions are implemented and pushed. Summary of what changed and where, plus the things I found along the way that were not in the plan. Landed
The #1236 cassette was also re-recorded with a fourth tool-calling turn, so pairing across a summary is exercised for the first time. Four things worth flaggingThe The A2A cleanup hang is filed as #1298, with the evidence that it hits both Three of the seven open #1234 items turned out to be shared with adk-python, not Go defects: the zero retention size, the tie at the compaction boundary, and the absent cooldown. That changed what I did with them: the first is rejected in Go because it has no legitimate use anywhere, the other two are recorded in the cross-language notes rather than diverged on unilaterally. Two of my own tests were vacuous and I only caught them by mutating the source. The both-strategies test passed whether or not the gate existed, because the configuration never made the two strategies want the same turn. And an assertion I added last session checked for the absence of Still openDeliberately not done, with reasoning in the replies: the partially overlapping ranges from the two producers, which decision 4 largely prevents and which is better judged after it lands; the coverage model, left timestamp-based to match the reference; and recovered-call ordering, which interacts with the stream ordering changed on #1232. Every branch was verified independently: build, |
|
Ready for another look. Before asking, I went back over every inline comment on #1231 to #1236 and checked the code rather than trusting my own earlier replies. Two things came out of that. 50 of your comments never got an inline answer. On #1232 I replied to all 19 in a commit message, which from your side looks identical to being ignored. Same for most of #1231. Every one of those now has a reply on the thread itself. One thing I reported as fixed was not. The claim that compaction "keeps prompts small" is wrong, because the sliding window never re-summarizes a summary. I said I had corrected the wording. I had corrected it in one place, and the original had already been copied into three more, one of which my own search missed because it wraps across two lines. Fixed in all four now. Thread stateThreads are resolved only where the fix is real and I can point at it. Anything partly done or not done is left open on purpose, including four on #1231 that were marked resolved earlier but should not have been.
So: 52 done, 38 still open. The open ones split into things that need a decision from you and things that just need doing, and each thread says which. The larger open questions, if you would rather answer them here than thread by thread: whether coverage should be keyed on event identity instead of timestamps, which closes two defects at once but changes the stored shape and diverges from the Python version; whether summaries should go through the plugin pipeline, which the Python version effectively does and this does not; and whether the mid-turn strategy needs a cooldown, which neither implementation has today. State of the codeEvery branch passes build, race tests with shuffling, lint and tidy in both modules. I dispatched the Go workflow manually to confirm that, because these pull requests target the integration branch and so get no automated checks beyond the CLA. That gap is worth fixing separately. Also opened #1297, which adds an API compatibility check, since nothing in CI would have caught the signature break you found here. |
25533df to
f838737
Compare
…'s role Review of the interface surfaced three things an implementer could not learn from the documentation. SummarizeEvents documented that a nil event means "skip this compaction" but never said which of the two nil returns signals it. The distinction matters: (nil, nil) is a decline and leaves history alone, while (nil, err) is a failure that gets reported and traced. A summarizer that reports failures as declines is indistinguishable from an idle one while the prompt keeps growing. NewSummaryEvent read as a validator, because its documentation led with the chronology check. It is a constructor. Say what it builds first, and say that implementations are expected to call it rather than assemble the event themselves. Nothing explained that the returned event comes back without an ID, invocation ID or timestamp, which from outside looks like a bug rather than the framework's job. Documenting it also answers why this constructor takes no context.Context when session.NewEvent does. Comments only, no behavior change.
Wires the compaction library into the run loop, so it now does
something. Two halves:
- The contents processor substitutes each stored summary for the
events it covers when assembling a prompt. Compaction events are
exempt from the empty-content filter because they carry their
summary on Actions.Compaction rather than on Content.
- The runner runs sliding-window compaction once an invocation has
finished and all of its events have been persisted, then appends
the summary. Enabled with runner.Config.EventsCompactionConfig,
off by default.
The window covers every invocation since the last compaction, plus
OverlapSize earlier ones so consecutive summaries overlap and context is
not lost at the seam. It is trimmed so a summary can never split a
function call from its response.
The summary is appended but not yielded: it is bookkeeping for the next
prompt, not part of the conversation. A compaction failure is returned
rather than swallowed, since silently never compacting would let a
session grow until some later turn fails against the context limit, far
from the cause.
Testing: prompt-assembly tests covering substitution and the paused
long-running-tool case, plus runner tests for the trigger, config
validation and error propagation.
The most serious finding is not about compaction at all. A compaction
record instructs prompt assembly to drop a span of history and put
content of the record's choosing in its place. Two separate paths let
that record come from outside the framework: the REST create-session
body mapped Actions.Compaction verbatim onto the stored event, and
EventActions is handed to tool code and to callbacks, whole, and then
copied onto the event that gets persisted. Either one was an
erase-and-inject primitive over the conversation.
It is fixed in layers, because the two halves fail differently. Prompt
assembly now ignores compaction records entirely unless the run has
compaction configured, which covers every application that never opted
in -- the default. For the applications that did opt in, gating is not
enough, so the record is also cleared wherever caller-supplied actions
become an event, and the REST layer no longer maps it inbound at all.
Reads still return records, so a client can see summaries it did not
write.
Compaction also ran only when the consumer drained the whole stream.
Breaking out of the range loop on the terminal event is the ordinary
streaming idiom and is what the A2A executor does, so for those callers
compaction silently never happened. The hook now runs from a defer via
an idempotent closure, in both the agent and node paths. On an early
exit the error is logged rather than yielded, because yield must not be
called after it has returned false.
Two concurrent invocations on one session lost conversation. The window
was selected from the session snapshot the invocation began with, so
events another invocation had appended were inside the recorded range
but were never summarized, and every later prompt dropped them. Not a
data race, so -race stayed clean while turns went missing. The session
is re-read before the window is chosen, and again after summarizing,
because the model call is itself long enough for an append to land
inside the chosen range. A summary whose range was raced is discarded.
The window is now bounded at CompactionInterval new invocations rather
than running to the end of the session. Uncapped it was O(session)
instead of O(interval), so enabling compaction on an existing deployment
handed a whole live conversation to one model call, and it compounded:
a summarizer error records nothing, so the next turn recomputed from the
same start over a strictly larger window and was more likely to fail
again. A retry is now the same size as the attempt that failed, and a
backlog drains one bounded window per turn.
Also:
- The summarization call ignored the application's
GenerateContentConfig, so the one call that sees the entire
transcript fell back to provider defaults for safety settings and
output limits. It now inherits the root agent's config, minus the
system instruction, tools and tool config.
- A summary carried no branch or isolation scope, so it passed every
branch filter and leaked a sub-agent's content to the parent. It now
inherits both from the window, and a window is trimmed to a single
scope before being summarized.
- An invocation that ended in an error is no longer summarized. The
window would be a question with no answer, and that summary is
stored permanently.
- Compaction failures are wrapped in an exported ErrCompaction, so a
caller can tell a bookkeeping failure from a failed turn instead of
matching on an error string. The A2A executor currently turns the
former into a task-failed event for the end user.
- Non-text parts are dropped from a summary. A hallucinated
FunctionCall in one would have reached the model unpaired. A part
that survives is copied whole rather than rebuilt from its text, so
metadata the model expects back with it, a thought signature above
all, is not lost. A summary left with no prose at all is refused.
- Prompt assembly no longer sorts the result by timestamp, which could
put a function response ahead of the call it answers whenever clock
skew or the SQL backend's microsecond truncation left the two out of
order. Raw events keep their stream order, and each summary is
placed where the first event it covers sat, so it still precedes any
turn it does not cover.
- OverlapSize is documented as repeating content rather than sharing
it. The repetition is the point of the option, and by the time the
summaries exist it lives in their prose, not in their ranges, so it
cannot be trimmed away afterwards.
Testing: every fix has a regression test, each confirmed to fail against
the previous behaviour rather than passing vacuously. The gaps the
review named are covered too. A non-LLM root agent, so the hook in
Runner.Run is exercised rather than only runNode's. The branch that
reports a failure to store a summary. An OverlapSize case that tells
overlap from no overlap by comparing stored ranges. And a compaction
event authored by another agent, which pins the foreign-reply clause.
Build, race tests, lint and tidy are clean in both modules. One
unrelated pre-existing failure in telemetry.TestConfigureExporters (an
OpenTelemetry schema version conflict from the GCP detector bump)
reproduces on the integration branch with no compaction code present.
Five findings from the review of #1231 that had no design question attached. The code they concern was introduced there, but it is amended here rather than in that commit so the reviewed history stays readable. A compaction record carrying no content could evict a real summary. Subsumption keyed on "declares a compaction" while substitution kept only records with content, so the weaker predicate won where it mattered: the contentless record subsumed the usable one, both were then dropped, and nothing represented the range while the boundary calculation went on pointing at the useless record. Subsumption now uses IsCompactionEvent, so only a record that could stand in for the range may displace one that already does. A streamed fragment could be stored as the whole summary. The loop returned on the first response carrying content without checking Partial, so a chunking model yielded a summary of its first fragment and lost the usage metadata that only the final response carries. This summarizer asks for a non-streaming call, so a well-behaved model never does it, but model.LLM is an exported interface and Partial exists to mark exactly this. Three nil dereferences were reachable from exported entry points: a nil element in the event list panicked both NewSummaryEvent and Apply, and a nil part panicked the transcript builder. NewSummaryEvent now reports the offending index, and the other two skip. Being total over the input is the right posture for a package whose callers include third-party Summarizer and model.LLM implementations. An event whose parts are all inline data, file data, executable code or a code-execution result rendered as nothing at all, so after compaction there was no record the turn had happened. Each now renders a short placeholder naming the kind. Dropping the bytes is deliberate; dropping the turn is not, and the same argument already justifies rendering thoughts and tool traffic. Finally, the Config doc claimed that enabling neither strategy disables compaction entirely, which Validate contradicts by rejecting exactly that. The doc was the wrong half: a Config that costs a configuration step and does nothing is almost always a mistake, and a nil Config already says "disabled" unambiguously. Testing: five regression tests, each confirmed to fail against the previous behaviour rather than passing vacuously, two of them by panic. Build, race tests, lint and tidy are clean in both modules.
… events Two review findings on #1231 that needed a decision rather than a patch. A stored compaction event satisfied IsFinalResponse. It carries a record and no content, which is exactly the shape that clause treats as a final answer, so a streaming consumer deciding what to surface would show an empty final response every time compaction ran. A compaction event is bookkeeping and now reports false. The summarization call had no bound on the transcript it built. Only tool arguments and responses were truncated, so the same payload cost 2,710 characters as a tool response and 100,663 as a text part, decided purely by which kind of part it arrived in. Text is not the more trustworthy of the two: it carries pasted documents and tool results re-emitted as text. The per-part cap now applies to every rendered part. A whole-transcript budget sits above that, and it declines rather than trims. Dropping the oldest turns would be the obvious fix and is the wrong one: every event in the window is inside the range the compaction records as covered, so removing them from the transcript while still deleting them from history would lose them with nothing standing in their place. Exceeding the budget means the window is too large, so that is what the error says. One intermediate step shrinks oversized parts first, so a couple of large payloads do not refuse an otherwise reasonable window. Testing: three regression tests, each confirmed to fail against the previous behaviour.
Four items that needed work rather than a decision. Chronology is now checked across the whole window instead of only its ends. The range is the interval between the first and last event and prompt assembly deletes everything inside it, so an interior event stamped past the last one was summarized, fell outside the range, and survived in the prompt as well: the model saw that turn twice. Widening the range to cover the true span would have been the wrong repair, since a window is a contiguous slice and stretching its range past its own ends can swallow an event that was never summarized, turning a duplicate into a deletion. I tried that first and a test caught it. A thought-only summary is refused. The transcript builder skips thought parts of a stored summary, so one rendered as nothing: the covered turns were deleted and replaced by an empty line. The author and tool names in a transcript line are escaped, not just the free text around them. Both are attacker-influenced, Author over the REST surface and tool names through a dynamically loaded tool set, and both were interpolated raw into a line whose shape the summarizer trusts. The REST mapping has a direct test. It pins the direction: a client-supplied record is dropped on the way in, a stored one is returned on the way out. Testing: four regression tests, each confirmed to fail against the previous behaviour.
…ime out Two decisions from the review, both settled by looking at what the reference implementation does. Summaries bypassed the plugin pipeline. Every other event the runner persists passes through the event callback, which is where a plugin sees, rewrites or rejects what enters a session, and a derived summary is exactly the kind of content a redaction plugin cares about. adk-python reaches the same place from the other direction: its sliding window yields the event and lets the runner append it, so persistence stays at the runtime's synchronisation point. Go now offers the summary to plugins before storing it, and stores whatever comes back. The summarizer call had no bound. It is synchronous inside the run loop, so one that hangs holds up the turn behind it with nothing to show for it. LLMSummarizerConfig gains a Timeout, defaulting to zero, which means no timeout and matches the behaviour of every ADK implementation today. It can only shorten the wait, never extend it, since the caller's own deadline still applies. Testing: the plugin test rewrites the summary and asserts the rewrite is what gets stored, so it pins that the returned event is the one persisted, not merely that a callback ran. Both confirmed to fail against the previous behaviour.
The part filter and the nil-element check arrive with this change, so the constructor's contract has to grow with them. Dropping non-prose parts is a security control rather than tidying: whatever the summarizer returns is injected into later prompts verbatim, so a function call it invented or was tricked into emitting would arrive unpaired and a model may act on it. An implementer needs to know that before wondering where their part went. Comments only, no behavior change.
Compaction previously left no trace, so there was no way to tell from production whether it was firing, how much it was shrinking, or whether the summarizer was failing. Each summarization now runs inside a "compact_events <trigger>" span carrying the trigger, session, summarizer type, event count, configured thresholds and the resulting range. Three states are distinguishable in a trace: - no span: the trigger was evaluated and declined - span with no result attributes: the summarizer ran and declined - span with error status: the summarizer failed The span name and the gen_ai.compaction.* attribute keys are part of ADK's cross-language telemetry contract, so dashboards written against one implementation work against the others. Testing: span assertions via an in-memory exporter, covering attribute values, the omission of the strategy that did not fire, and all three states above.
Five findings from the review of #1233 with no contract question attached. The parity items, which depend on what adk-python emits, are left alone. A failed compaction produced a span that was simultaneously an error and a success. A Summarizer may return a usable event alongside an error, and the caller discards that event, but the span recorded its identity and range anyway, so a trace showed a failure naming an event no session ever held. Both sides are fixed: TraceCompactionResult returns once it has recorded the error, and summarizeTraced no longer stamps a result it is about to throw away. That second half also stops a discarded event consuming a UUID. A panicking summarizer left the span status Unset, which reads as success, so third-party code blowing up looked like a healthy compaction that happened to produce nothing. The span now ends through a deferred recover that sets an error status before re-panicking. The panic itself still propagates. The telemetry tests pinned that attributes existed rather than what they contained, which left the wire format unprotected. Reproduced before fixing: switching the timestamp layout to RFC1123, and sourcing the start bound from EndTimestamp, both left the suite green. The assertions now compare rendered values, so both mutations fail, and result_event_id is required to be non-empty so a trace can be joined to the stored summary. Finally, a comment claimed a span shows only the strategy that fired. Both strategies can be configured at once and the branches gate on configuration, not on firing. The comment now says what is true and points at the trigger attribute, which is what actually names the strategy. Testing: two new tests, both confirmed to fail against the previous behaviour, one of them exercising a summarizer that returns an event and an error together, which the existing failure test could not reach since its fake returns a nil event. Build, race tests, lint and tidy are clean in both modules.
…butes Two of the three parity findings on #1233, settled by reading adk-python rather than leaving them open. Both are contract keys that a consumer joining traces across the two implementations has to match on. The range bounds were emitted as RFC 3339 strings. adk-python declares EventCompaction.start_timestamp as a float in seconds and puts that float straight on the span under the same key, so the two implementations disagreed on the type behind a shared key. They are now epoch seconds. That also removes both side defects the review found, since the host zone offset reaching the wire and the fractional zeros breaking sort order were consequences of rendering a string in the first place. The summarizer type was Sprintf("%T"), which emits a Go type expression rather than a name: "*compaction.LLMSummarizer" where the reference implementation, which uses type(summarizer).__name__, has "LlmEventSummarizer". Go now emits the bare type name. The third finding, the missing gen_ai.system, is confirmed real but left alone. The key is easy; the value is a choice between two semconv generations, since adk-python emits "gemini" or "vertex_ai" while this repo is on semconv v1.36.0 where the constants are "gcp.gemini" and "gcp.vertex_ai". No Go span sets the attribute today, and the compaction path has no backend handle to derive it from, so it is not compaction's decision to make alone. Testing: the assertions pin the attribute type as well as the value, so regressing the timestamps to strings fails with a type mismatch rather than passing on a coincidence. Both fixes were confirmed to fail when reverted. Build, race tests, lint and tidy are clean in both modules.
The reference implementation sets gen_ai.system on every compaction span and Go set it on none, so a trace could not say which system produced a summary. The backend comes from the summarizer through the same optional interface the rest of the framework already uses to tell Vertex AI from the Gemini API, rather than a new parameter threaded down from the runner. A third-party Summarizer with no model, or one that does not care to say, leaves the attribute unset instead of being made to invent a value. One deliberate difference from the reference. The values here come from this repo's semconv version, which prefixes them "gcp.", while adk-python is on an older generation emitting bare "gemini" and "vertex_ai". Mixing semconv generations inside one implementation is worse than differing from another implementation on a value, and the gap is repo-wide rather than compaction's to close. Testing: a table covering both backends and a summarizer that reports neither, which must leave the attribute absent rather than guess.
Two gaps from the #1233 review, plus a stale comment. The span carried no invocation id. It is not a child of the turn's span, so there was no way to ask which turn a compaction belonged to. It now carries the invocation of the newest event in the session, which is the turn that triggered it. The newest event rather than the newest one in the window, deliberately: the window is what is being summarized, and tail retention excludes the turn in progress from it, which is exactly the turn worth naming. The summarizer's token usage was dropped. Compaction spends a model call to save tokens later, so a span that does not record what it spent cannot show whether it paid for itself. Input and output tokens are now on the span when the summarizer reports them. Also corrected a test comment that still said a span shows the strategy that "fired". Both strategies can be configured at once, and the branches gate on configuration; the trigger attribute is what names the strategy. The production comment was fixed earlier and its twin in the test was missed.
… covers The individual assertions only check the keys they name, so adding, renaming or dropping one passed unnoticed. The whole key set is now compared, since these keys are shared with adk-python and a dashboard written against one implementation depends on them. Also narrowed two claims in the doc comment that were larger than the truth. The parity with adk-python is real and was read from source, but nothing enforces it, and adk-kotlin has no compaction telemetry at all, so "cross-language contract" meant two implementations rather than all three. Investigated and deliberately not changed: the span still wraps the summarizer call rather than the whole compaction. Starting it earlier would emit a span for every evaluation that declines, which breaks the property that a span means compaction really ran, and what it would add is an in-memory scan measured in microseconds against a model call. The comment now says so instead of leaving the scope unexplained. Also reverted a change I had made and could not justify: passing the invocation context to the compaction hook so the span would nest under the turn. It made no difference, because the outer context already carries the caller's span, and the turn's own span has ended by the time compaction runs so it cannot be the parent anyway. A test now pins what is actually true: compaction shares the caller's trace and names the invocation it followed.
df752df to
8d88b32
Compare
f838737 to
ff5b84b
Compare
There was a problem hiding this comment.
Reviewed at f838737. The doc-only commits pushed since then do not change any line cited below.
The attribute set and span name do match adk-python, and the claim in the doc comment that they were read from the Python source rather than assumed checks out. Four things stop this delivering what the description promises.
1. A compaction that was discarded emits a span that says it succeeded. The span is ended inside summarizeTraced , before the five paths in compactAfterInvocation that can throw the summary away. With a plugin that rejects compaction events, the run returned context compaction failed: plugin rejected the summary event, nothing was stored, and the span was status=Unset carrying result_event_id, start_timestamp and end_timestamp — an ID that exists in no session. The other four paths (cancelled context, reload failure, RangeRaced, append failure) have the same shape.
2. The span is a root in its own trace by default. Started three turns with no ambient caller span and got six distinct trace IDs, with the compaction span's parent invalid every time; with a caller span it becomes a sibling of invoke_agent, never a child. Under head sampling the two take independent decisions, so a sampled turn usually loses its compaction span, and compaction spans turn up as orphan single-span traces with nothing to join them to. gcp.vertex.agent.invocation_id is the only fallback correlator, and it is inferred from the newest session event rather than passed down — with two invocations in flight on one session, both compaction spans can carry the same invocation ID, so at least one of them names a turn that did not cause it. The authoritative ID is already in hand at the call site that invokes the compactor.
3. Two attributes diverge from adk-python in presence and in meaning. gen_ai.system is set only for the two Google backends , while adk-python sets it unconditionally in the dict literal — over a non-Google model the attribute is simply absent here, and a test pins that absence. And gen_ai.usage.output_tokens is candidates-only, where TraceGenerateContentResult in the same package sets the same key as candidates + thoughts with a semconv citation. Two spans in one trace mean different things by one key. The > 0 gating also makes a genuine zero absent rather than zero.
4. A zero timestamp is emitted as the year 1754. epochSeconds on a zero time.Time gives -6.795e+09. This is reachable through the shipped path, not only a hand-built record: NewSummaryEvent takes events[0].Timestamp verbatim and the in-memory service does not stamp a missing timestamp, so I got end - start ≈ 271 years for three seconds of history, on a span reporting success. adk-python omits the key when the bound is absent; matching that is better than substituting 1970.
On the tests: the key-set assertion pins the eleven attributes the minimal fixture happens to emit, and a legal both-strategies config emits fourteen. Renaming gen_ai.compaction.token_threshold, renaming event_retention_size, and flipping token_threshold from INT64 to a string all leave the suite green — a type flip on a shared key is exactly the silent cross-language break the description warns about. Adding an attribute that carries conversation text on any conditional branch is green too, so the no-content-leak property has nothing enforcing it. The sub-second timestamp path is unpinned because the fixture lands on whole seconds, and TestCompactionSpanJoinsTheCallersTrace manufactures the parent it then asserts exists, and never reads the correlation attribute.
Two smaller notes. The panic path sets an error status but records no exception event, so an alert keyed on exception.type misses every panicking summarizer. And the summarizer's own model call emits no span at all — compact_events is a childless leaf wrapping an untraced LLM call, so when a compaction fails after spending tokens there is no child span to recover the cost from.
One thing not to change: the "lossless" epochSeconds rewrite (float64(t.Unix()) + float64(t.Nanosecond())/1e9) is not lossless. Measured across 2000 consecutive nanoseconds at current epoch magnitudes it improves the worst-case error from 244 ns to 119 ns and leaves ~200 ns granularity, because 61 bits do not fit in a 53-bit mantissa however the arithmetic is arranged. Ordering is preserved in both forms. Float seconds is the cross-language contract, so this is a cost of the contract rather than a defect.
Two independent defects in how a compaction record is stored and reported. InMemoryService cloned StateDelta, ArtifactDelta and RequestedToolConfirmations but stored the caller's *EventCompaction by pointer. That is the one field on EventActions that must not be editable after the append: it names the range of history every future prompt drops, so a producer holding the pointer could move EndTimestamp afterwards and change what the agent sees. It also raced. The record and its content are now deep-copied like their neighbours. The compaction span published a zero timestamp as epoch seconds, which reads as the year 1754, so a compaction covering three seconds of history announced a range 271 years wide on a span that otherwise reported success. A zero time means no bound was recorded, and the key is now omitted, which matches the reference implementation and is the only form a consumer can distinguish from a real reading. Reachable through the shipped path, since NewSummaryEvent takes the covered events' timestamps verbatim and nothing stamps a missing one. Addresses finding 6 of the #1231 review and finding 4 of the #1233 review.
The compaction span ended when the summarizer returned, before the caller had
decided anything. Five paths then discarded the result: a cancelled turn, a
failed re-read, a competing compaction, a plugin rejecting the summary, and a
failed append. Every one left a span reporting success and carrying a
result_event_id naming an event that exists in no session, so a trace could not
tell a compaction that shrank a prompt from one that spent a model call and
changed nothing.
The span now stays open until the caller reports the outcome, and a discarded
summary is recorded on the same key as a decline: to anything reading the trace
the two mean the same thing.
Four smaller corrections in the same area:
- A panicking summarizer set an error status but recorded no exception event,
so an alert keyed on exception.type never saw one.
- Output tokens counted candidates only, where TraceGenerateContentResult in
this same package counts candidates plus thoughts and cites semconv for it.
Two spans in one trace meant different things by one key, and a thinking
model's summary was under-reported.
- gen_ai.system had a second copy of the backend mapping. It now uses the one
the rest of telemetry uses, so a future change reaches both. The two
divergences from the reference implementation are unchanged and are now
documented at the test that pins them, rather than the test simply freezing
the current behaviour.
- The invocation the span names was read from the newest event in the
session, which is a guess that goes wrong exactly when it matters: with two
invocations in flight, both compactions read the same event and at least
one named a turn that did not cause it. The caller passes it now.
Not fixed, and now stated as a known gap in the test: with no ambient caller
span the compaction span is a root of its own rather than joining the turn.
Closing it needs the invocation's span context to reach the runner, and the
agent derives it internally and passes it only to its own children. An attempt
to parent from the InvocationContext was a no-op and is not in this change.
Addresses findings 1 and 3 of the #1233 review, its two smaller notes, and part
of finding 2.
|
Fixes are in #1316. Finding 1. Fixed, and it needed more than moving the Finding 2. Half fixed, and I want to be straight about which half. The correlation attribute is fixed: the caller passes the invocation ID instead of the compactor reading the newest event in the session. Your concurrency case was exactly right, both compactions read the same event and at least one named a turn that did not cause it. The parenting is not fixed. I tried attaching the invocation's span context in the runner and it was a no-op, because the agent derives Finding 3. Output tokens fixed: candidates plus thoughts, matching
Finding 4. Fixed. A zero bound is omitted rather than published as 1754. Reachable through the shipped path, as you said, since nothing on the append path stamped a missing timestamp. On the tests. The three rename and type-flip cases were already covered by the time I got here. The content-leak branch and the sub-second path are still not pinned. The two notes. The panic path records an exception event now. The summarizer's own model call still emits no span. epochSeconds. Left alone. I had my agent check your numbers with |
The compaction span ended when the summarizer returned, before the caller had
decided anything. Five paths then discarded the result: a cancelled turn, a
failed re-read, a competing compaction, a plugin rejecting the summary, and a
failed append. Every one left a span reporting success and carrying a
result_event_id naming an event that exists in no session, so a trace could not
tell a compaction that shrank a prompt from one that spent a model call and
changed nothing.
The span now stays open until the caller reports the outcome, and a discarded
summary is recorded on the same key as a decline: to anything reading the trace
the two mean the same thing.
Four smaller corrections in the same area:
- A panicking summarizer set an error status but recorded no exception event,
so an alert keyed on exception.type never saw one.
- Output tokens counted candidates only, where TraceGenerateContentResult in
this same package counts candidates plus thoughts and cites semconv for it.
Two spans in one trace meant different things by one key, and a thinking
model's summary was under-reported.
- gen_ai.system had a second copy of the backend mapping. It now uses the one
the rest of telemetry uses, so a future change reaches both. The two
divergences from the reference implementation are unchanged and are now
documented at the test that pins them, rather than the test simply freezing
the current behaviour.
- The invocation the span names was read from the newest event in the
session, which is a guess that goes wrong exactly when it matters: with two
invocations in flight, both compactions read the same event and at least
one named a turn that did not cause it. The caller passes it now.
Not fixed, and now stated as a known gap in the test: with no ambient caller
span the compaction span is a root of its own rather than joining the turn.
Closing it needs the invocation's span context to reach the runner, and the
agent derives it internally and passes it only to its own children. An attempt
to parent from the InvocationContext was a no-op and is not in this change.
Addresses findings 1 and 3 of the #1233 review, its two smaller notes, and part
of finding 2.
8d88b32 to
25a4e4c
Compare
|
Closing. Telemetry is no longer a separable slice of this stack. Review of the stack changed the compactor so that it calls telemetry directly: the span is opened before summarization and stays open until the caller reports what became of the summary, which is what stopped the five discard paths reporting success for an event that reached no session. That means Keeping this PR would have meant hand-splitting The telemetry work is unchanged and now lands in #1232 alongside the engine that calls it. Nothing was dropped: the whole stack tip is byte-identical to what it was before the re-cut. The review threads here stay readable. Sorry for the churn. |
2 participants
Compaction previously left no trace, so there was no way to tell from
production whether it was firing, how much it was shrinking, or whether
the summarizer was failing.
Each summarization now runs inside a "compact_events " span
carrying the trigger, session, summarizer type, event count, configured
thresholds and the resulting range.
Three states are distinguishable in a trace:
The span name and the gen_ai.compaction.* attribute keys are part of
ADK's cross-language telemetry contract, so dashboards written against
one implementation work against the others.
Testing Plan
go build,go test -race -count=1 -shuffle=on ./...,golangci-lint runandgo mod tidy -diffare clean on this commit alone.Stack
PR 3 of 6 for context compaction. Targets
baptmont/compaction-02-sliding-windowrather thanmain, so the feature lands onmainas one pre-reviewed merge frombaptmont/context-compaction.PR 3 of 6 is this one.
Every commit in the stack builds and passes
go test -race -count=1 -shuffle=on ./...on its own.Part of #298.