[api][runtime][python] Add Agent Trace recording to Event Log - #924
[api][runtime][python] Add Agent Trace recording to Event Log#924joeyutong wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
Thanks for taking this on. One thing worth surfacing: the Python Event.id switch to a per-occurrence uuid4 also fixes a real collision. chat_model_action.py keys sensory-memory dicts on the request event's id, so two identical ChatRequestEvents on the same key used to collapse into one and splice their conversations together. The PR body frames it as identity-semantics alignment, so you may want to update the description to match what the code actually does.
A few questions inline, plus one below that spans files.
The description mentions this complements the Event lineage work in #923. Going through both diffs, I think there may be an interaction worth checking before either lands.
The new serializer writes the Event's attributes map rather than the Event itself: EventLogRecordJsonSerializer.java:86 emits eventAttributes via the helper at :90-91, where the version it replaces did mapper.valueToTree(event). #923 adds upstreamEventId and upstreamActionName as fields on Event with getters, not as attribute entries.
If I'm reading that right, those two fields wouldn't reach the Event Log once the flat shape lands, which is the artifact #841 set out to populate. The same applies to tools/reconstruct_trace_tree.py, which reads record["event"] and would hit a KeyError with no event key present. Nothing in a merge would surface either one, since Event.java isn't touched here.
More generally, should framework-owned metadata on Event have a home in the flat record? upstreamEventId and upstreamActionName read like the same family as executionId, so a top-level slot would seem consistent, but is there a reason to keep them out?
| chatAsync | ||
| ? ctx.durableExecuteAsync(callable) | ||
| : ctx.durableExecute(callable); | ||
| ExecutionReporters.started(ctx, ExecutionReporter.EntityTypes.LLM, model); |
There was a problem hiding this comment.
started here and succeeded at :389 bracket ctx.durableExecute(...), but on replay that returns the cached result and short-circuits (RunnerContextImpl.java:383-385) while the action body still re-runs. Your own testEarlierCheckpointReplayKeepsDurableState:1039 pins that: DURABLE_CALL_COUNTER stays at 1 while the output is produced again.
So after a fine-grained recovery the log gets a complete started plus succeeded pair, with a fresh executionId, for a model call that never happened. ToolCallAction has the same shape. Since counting LLM calls is the first thing anyone does with this log, cost dashboards would over-report spend and under-report latency.
What makes me read this as unintended: the Action level already has ExecutionLifecycleEvents.executionReused() for exactly this case, but nothing below Action can emit it, since ExecutionReporter has no such method.
Could a cached durable result surface a reused signal that the reporters emit instead of started/succeeded? Or does the reporting want to move inside the durable boundary? And if it's out of scope here, would naming it in the monitoring doc be enough for now?
There was a problem hiding this comment.
Good catch. Child durable-cache reuse remains out of scope because the current durable boundary does not expose cache-hit state to ExecutionReporter. I documented that cached LLM/Tool results may currently appear as new successful executions; a reused child signal is follow-up work.
| JsonNode eventNode = rootNode.get("event"); | ||
| if (eventNode instanceof ObjectNode) { | ||
| boolean truncated = truncator.truncate((ObjectNode) eventNode); | ||
| JsonNode attributesNode = rootNode.get("eventAttributes"); |
There was a problem hiding this comment.
Passing rootNode.get("eventAttributes") into truncator.truncate(...) narrows truncation in two ways. Same block at Slf4jEventLogger.java:179-181, so both loggers are affected.
The protections now point at the wrong names. JsonTruncator is untouched by this PR and still skips PROTECTED_FIELDS = {eventType, id, attributes} at isTopLevel (:55-56, :117-119). Those were envelope names; under the flat shape they are user attribute names, so an attribute called id escapes event-log.standard.max-string-length at STANDARD. JsonTruncatorTest.testProtectedFields:136-161 still pins the old shape and passes, so CI won't catch it. Worth noting #923 adds upstreamEventId and upstreamActionName to that same set, which won't take effect under the flat shape either.
Separately, entityMetadata sits as a sibling of eventAttributes (EventLogRecordJsonSerializer.java:73), so the truncator never sees it. It's fed by the public ToolExecutionMetadataProvider hook, and LoadSkillTool.java:68-80 copies the model-supplied name and path straight in, so an LLM-controlled value lands unbounded.
Would running the truncator over the whole record, with the new framework field names protected, close both at once? That was my first instinct, though I may be missing why the narrowing was deliberate. Either way, would testProtectedFields want re-pointing at what the production path now passes in?
There was a problem hiding this comment.
Thanks, the protected-name mismatch was a bug. Truncation remains scoped to eventAttributes; JsonTruncator now treats its entire input as payload, and the regression test covers eventType, id, and attributes as ordinary payload keys. I am keeping entityMetadata size policy separate rather than truncating the whole record.
There was a problem hiding this comment.
Protected names are settled, thanks. The test side is the part I'm still unsure about.
The guarantee now lives in the two call sites (FileEventLogger.java:212, Slf4jEventLogger.java:179) rather than in JsonTruncator, and I couldn't find a test that pins it. Flipping rootNode.get("eventAttributes") back to rootNode would fail none of the six candidate tests: the logger tests assert only eventAttributes.customData, the Python e2e one only that "truncatedString" appears somewhere in the line, and the JsonTruncatorTest units never see a record. eventId is a 36-char UUID that would be wrapped at max-string-length=10 and nothing checks it.
That leaves the promise at monitoring.md:219 ("Truncation only applies to large nested content under eventAttributes") resting on review rather than CI. Is one assertion in FileEventLoggerTest.testStandardLevelTruncation enough to close it, checking eventId is still textual at max-string-length=10?
There was a problem hiding this comment.
Yes. I added an assertion to FileEventLoggerTest.testStandardLevelTruncation that, with max-string-length=10, the top-level eventId still exactly matches the original UUID while the payload field is truncated. This pins the truncation boundary at eventAttributes.
There was a problem hiding this comment.
Confirmed on the FileEventLogger side.
Slf4jEventLogger.java:179 has the same block, and Slf4jEventLoggerTest has no truncation test at all. So someone could change that line back to rootNode and no test in the repo would fail. Worth adding a matching assertion there, or is one enough while the two blocks are the same?
There was a problem hiding this comment.
Added the matching Slf4jEventLoggerTest. With max-string-length=10, it asserts that the top-level eventId remains the original UUID while the long value under eventAttributes is truncated.
| outputEvents = actionTaskResult.getOutputEvents(); | ||
| generatedActionTaskOpt = actionTaskResult.getGeneratedActionTask(); | ||
| notifyFinished = isFinished; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Could the Action lifecycle guarantee cover the remaining failure paths here?
This catch (Exception) misses a raw Error, including one now unwrapped by JavaFunction.java:114. Before this PR that body was just return getMethod().invoke(null, args);, so every user throwable arrived wrapped in InvocationTargetException. A tool throwing AssertionError used to be caught at ToolCallAction.java:174 and reported via ExecutionReporters.failed at :179. Now it skips this catch too and fails the task at :306's catch (Throwable t), leaving _execution_started_event with no terminal Event.
Separately, processEvent(...) at :434 can throw after maybePersistTaskResult at :411 but before notifyActionFinished at :437. This catch has already closed at :429 and the finally at :439 only calls completeActionExecution, so that produces the same incomplete lifecycle, with the result persisted.
Would catching Throwable long enough to report and clean up before rethrowing, or emitting finished right after successful invocation and persistence, keep every started Action terminal without changing which failures stop the task?
There was a problem hiding this comment.
Good catch. I now catch Throwable around Action invocation so raw errors emit failed before propagating, and emit finished immediately after a completed result is persisted, before processing output Events. This keeps the Action lifecycle paired without attributing listener or routing failures to the Action. I added regression tests for both paths.
| try { | ||
| eventLogger.append(eventContext, event, traceContext); | ||
| eventLogger.flush(); | ||
| } catch (Exception logError) { |
There was a problem hiding this comment.
Best-effort writes look intentional, but this also changes what an append or flush failure does. At the merge base (6f020c50) the work sat in EventRouter.notifyEventProcessed (EventRouter.java:233-242), which had no try/catch and declared throws Exception, so a failure propagated and failed the task. Was making it non-fatal the intent, or a side effect of the move?
Either way, what would you want an operator to see when a write is dropped? BuiltInMetrics declares and registers eventLogTruncatedEvents (:40, :53) but has no equivalent for failed writes, so an operator whose disk filled gets a log that just stops while the job stays green. A first-failure WARN plus an eventLogWriteFailures counter next to the truncation one is the shape I had in mind, though you may be weighing log noise against it.
Smaller thing in the same block: flush() at :108 is skipped when append at :107 throws, so a partial line can sit in the PrintWriter buffer.
There was a problem hiding this comment.
Making Event Log writes non-fatal is intentional. Event Log is an observability side channel, now including Trace, so a logging backend failure should not change Event processing or job success. I split append and flush into independent best-effort steps, so flush is still attempted after an append failure. Each failed write attempt increments eventLogWriteFailures once even if both steps fail; the first failure is logged at WARN and subsequent failures at DEBUG. FileEventLogger now also surfaces I/O errors otherwise swallowed by PrintWriter, and the compatibility change is documented.
| visited.add(id(current)) | ||
| cause = current.__cause__ | ||
| if cause is None and not current.__suppress_context__: | ||
| cause = current.__context__ |
There was a problem hiding this comment.
_root_cause follows __cause__ then __context__ when __suppress_context__ is false (:71-73), while Java walks only getCause() (ExecutionLifecycleEvents.java:100-107). __context__ is set implicitly by any raise inside an except block, where getCause() is set only when a cause is passed explicitly.
So a MyError raised inside except JSONDecodeError records errorType: json.decoder.JSONDecodeError where Java records MyError. Same wrapped failure, two errorType values in one log file, and a cross-language query on that field splits. AGENTS.md asks that "Public API changes must keep Java, Python, and YAML APIs semantically aligned".
Is following __context__ deliberate? Restricting to __cause__ would match Java, though it may be buying something I can't see. Either way test_failed_execution_reports_deepest_cause wires only __cause__ (test_flink_runner_context_trace.py:39), so that branch is unexercised on both sides.
There was a problem hiding this comment.
Good catch. The previous test covered only explicit __cause__, so the Python-only implicit __context__ branch remained unaligned. I removed that fallback: Python now follows only explicitly chained causes, matching Java Throwable.getCause(), and added a regression test confirming implicit context is not used.
| ### Per-event-type log levels | ||
|
|
||
| You can override the level for individual event types using the `event-log.type.<EVENT_TYPE>.level` config key, where `<EVENT_TYPE>` is the event's routing type string (the same string that appears as `eventType` in the JSON log). Built-in events use short snake-cased names such as: | ||
| You can override the level for individual event types using the `event-log.type.<EVENT_TYPE>.level` config key, where `<EVENT_TYPE>` is the event's routing type string (the same string that appears as `eventType` in the JSON log). Although the field name uses camelCase, built-in Event type values remain snake-cased: |
There was a problem hiding this comment.
Would it help to list the four _execution_* routing types in the per-event-type table, with a note on how they compose with event-log.trace.enabled?
Since this line says the per-type key is the event's routing type string, Trace can be enabled while event-log.type._execution_started_event.level: OFF still removes the started Events. That interaction isn't visible from the table today, and the four lifecycle types aren't listed in it at all.
There was a problem hiding this comment.
Agreed. I added all four execution lifecycle routing types to the per-event-type table and documented the composition explicitly: event-log.trace.enabled controls whether lifecycle Events are produced, while the normal per-type level resolution still applies once Trace is enabled. Setting an _execution_* type to OFF therefore suppresses that lifecycle Event and may make the recorded Trace incomplete.
| @@ -291,4 +302,6 @@ Other per-type levels from `config.yaml` are preserved — the `-D` flag only ov | |||
| ### Compatibility Notes | |||
There was a problem hiding this comment.
nit: could the Compatibility Notes name the actual rewrites, event removed, event.id → eventId, event.attributes → eventAttributes, and mention that Python Event IDs changed from content-derived to per-occurrence UUIDs?
eventType was already top-level, so the current wording names the one field that didn't move, and a reader can't derive the rest. Grepping docs/ for content-hash or uuid4 returns nothing, so the Python change lives only in event.py, and anything relying on content-hash id equality or dedup behaves differently after upgrade.
There was a problem hiding this comment.
Agreed. The Compatibility Notes now name the exact record-shape changes: the nested event object is removed, event.id becomes top-level eventId, event.attributes becomes top-level eventAttributes, and top-level eventType remains where it was. I also documented that Python Event IDs changed from content-derived IDs to per-occurrence UUID4 values, so payload equality must no longer be used for ID-based deduplication.
a5b2c0e to
fbd15b5
Compare
3b7f47f to
a0d86c8
Compare
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. Five of the six threads look right to me, and the two new lifecycle tests would actually catch the bugs rather than just run the code. One thing on the truncation thread, plus two notes here on the #923 alignment now that it's landed.
The reader tool is flink-agents-trace-tree (python/flink_agents/cli/trace_tree.py). At :101-110 it reads the record with record.get("event"), so a flat record doesn't crash. It prints a MALFORMED_RECORD warning to stderr and skips that record. Against a log written after this PR, the tool finishes with exit code 0 and prints no trees at all. That is a quiet failure. Would a test on the merged format be worth adding?
The Python Event.id change is already on main via #923 (event.py:86), so that part of this PR will drop out when you rebase. The compatibility note will not, though. Main's Compatibility Notes (monitoring.md:421-424) only have the two level bullets, and workflow_agent.md:707-724 is about IDs being immutable rather than about deduplication. So your bullet at monitoring.md:316 looks like the only place that warns equal payloads no longer give equal IDs. Worth keeping it when you resolve the conflicts?
a0d86c8 to
1c55683
Compare
|
@weiqingy Rebased onto current |
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. The flat record shape and the Slf4j truncation scope both check out, so my two open items are closed. A few more questions inline, none blocking.
| chatAsync | ||
| ? ctx.durableExecuteAsync(callable) | ||
| : ctx.durableExecute(callable); | ||
| ExecutionReporters.started(ctx, ExecutionReporter.EntityTypes.LLM, model); |
There was a problem hiding this comment.
Discussion #929 asks for the resolved model, the provider, and per-call token usage to land in entityMetadata. Today neither report carries any: this line and :389 both pass just the model, so an LLM record has no entityMetadata at all, and entityName is only the alias _default_chat_model. One line further down at :390 the model name and the token counts are both in hand, and they go only to the cumulative counters. Python is the same (chat_model_action.py:375, :377-388).
Model and provider look easy, since both are known before the call, so the start and the end report would carry the same value. Usage is the tricky one: entityMetadata is part of what matches an end report back to its start (RunnerContextImpl.java:264-286, ReportedExecutionKey.java:56-67), and ExecutionReporter.java:28-29 asks it to stay the same across both. A number known only after the response would not match, so the end report would come out under a fresh executionId instead of closing the one it started.
The record shape is being settled here, so would it be worth carrying model and provider now, and letting usage find its own home later?
There was a problem hiding this comment.
Added entityMetadata.model to Java and Python LLM lifecycle reports. It is the configured/requested model or deployment identifier, while entityName remains the ChatModel Resource name; start and terminal reports use the same metadata. I left provider out because the current ChatModel abstraction does not expose a stable provider identity across integrations, and token usage remains outside lifecycle matching metadata for now.
| break | ||
| if ( | ||
| isinstance(record, dict) | ||
| and record.get("eventType") in EXECUTION_LIFECYCLE_EVENT_TYPES |
There was a problem hiding this comment.
This skip drops the record and writes nothing, so it is the one place the reader discards something without leaving a warning behind. If a user's own Event happens to use one of these type strings it disappears from the tree, and its children then report MISSING_PARENT (:316-324) for a parent that is sitting right there in the log.
Nothing reserves the _ prefix today: Event.java:82-84 only checks for null or empty, and Python's event.py:59-71 has no check at all. So _execution_* is a convention, the same way _input_event is (EventUtil.java:27-29 matches that one on the raw string too). None of that changes in this PR. What does change is that these names now live in two languages: EntityTypes, ProblemCategories and ToolExecutionMetadataKeys all got mirrored into Python, but these four are typed out again at :26-33 and listed a third time in monitoring.md:395-398, with nothing keeping the copies in step.
So is the namespace part of the contract? If it is, would a shared constant plus a warning on this skip be worth adding?
There was a problem hiding this comment.
Documented the four lifecycle types as framework-reserved and moved Python consumers to shared constants. Trace Tree now ignores only records whose type, expected status, and execution identity match the lifecycle shape; a reserved-name business Event is retained and emits RESERVED_EVENT_TYPE with its Event ID.
| throw modelError; | ||
| } | ||
| ExecutionReporters.succeeded(ctx, ExecutionReporter.EntityTypes.LLM, model); | ||
| recordChatTokenMetrics(chatModel, response); |
There was a problem hiding this comment.
recordChatTokenMetrics sits outside the durable boundary. On a cache hit, durableExecuteCompletionOnly returns at RunnerContextImpl.java:384 and never reaches the real call at :390, but the cached ChatMessage still carries its promptTokens / completionTokens. So the documented counters (monitoring.md:50-51) go up for a model call that never happened. Python does the same (chat_model_action.py:377-388).
The window is small. A finished action is skipped whole at ActionExecutionOperator.java:362-383 and never gets here, so this is only the replay of an action that did not finish, and only when actionStateStoreBackend is set (default null, AgentConfigOptions.java:65-66).
It also predates this PR: main:375 is the same line in the same spot, so there is nothing to change here. Noting it for a follow-up issue, since these counters are user-facing.
There was a problem hiding this comment.
Agreed. This behavior predates the trace change and remains unchanged in this PR; I will track durable-replay token overcounting as follow-up work.
| if not isinstance(event_type, str) or not event_type: | ||
| return None, None, "field 'eventType' must be a non-empty string" | ||
| if not isinstance(event_attributes, dict): | ||
| return None, None, f"field '{attributes_field}' must be a JSON object" |
There was a problem hiding this comment.
Nit: event_id_value is checked at :122, but this return and the one at :125 both pass None as the id, so a record with a perfectly good eventId shows up as MALFORMED_RECORD with nothing to search on. This runs after both shape branches close, so it hits flat and legacy records alike. The lineage check just below at :134-138 does pass the id through.
There was a problem hiding this comment.
Fixed for both flat and legacy records: once a valid Event ID has been parsed, later shape-validation warnings retain it. Added coverage for invalid eventType and invalid attributes.
1c55683 to
b9deb42
Compare
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for working through these. All four are resolved on my side, with one nit inline.
I'll leave the final call to the maintainers.
| } | ||
| }; | ||
| Map<String, Object> llmMetadata = | ||
| chatModel.getModel() == null ? Map.of() : Map.of(MODEL, chatModel.getModel()); |
There was a problem hiding this comment.
nit: MODEL at :63 doubles as the tool-request context key (:126, :501), where the value is the Resource name rather than the model id. Python writes the string directly too (chat_model_action.py:351), while the other execution metadata keys sit in the ToolExecutionMetadataKeys / tool_execution_metadata_keys.py pair. Since entityMetadata.model is now a documented field, would a dedicated constant on both sides be worth it?
There was a problem hiding this comment.
Agreed. I added LLMExecutionMetadataKeys.MODEL and the matching Python constant, and use them specifically for entityMetadata.model. The existing Java MODEL constant now remains scoped to the ToolRequest context, where it stores the ChatModel Resource name. The wire field remains model.
AI-Contributed/Feature: 0/2950 AI-Contributed/UT: 0/2714
AI-Contributed/Feature: 0/84 AI-Contributed/UT: 0/156
AI-Contributed/Feature: 0/159 AI-Contributed/UT: 0/175
Treat eventAttributes as the payload root and document durable replay limitations. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 65/65 AI-Contributed/UT: 37/37
Report raw Action errors as failed and emit successful Action completion before processing emitted Events. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 9/9 AI-Contributed/UT: 82/82
Keep Event Log writes best-effort while exposing failures through a counter and a first-failure warning. Attempt flush independently after append failures and surface PrintWriter I/O errors. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 57/57 AI-Contributed/UT: 43/43
Document execution lifecycle event level overrides, the flat Event Log field migration, and Python per-occurrence Event IDs. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 11/11 AI-Contributed/UT: 0/0
Follow only explicitly chained Python causes so failure attribution matches Java Throwable.getCause semantics. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 2/2 AI-Contributed/UT: 29/29
Assert that STANDARD payload truncation leaves the top-level Event ID unchanged. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 0/0 AI-Contributed/UT: 4/4
Adapt Trace Tree reconstruction and Event Log tests to the flat Trace record format while preserving legacy input compatibility. Restore MCP server attribution and update rebase-drifted tests to current routing APIs. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 147/147 AI-Contributed/UT: 157/157
Record the configured model on LLM executions, centralize lifecycle event vocabulary, retain reserved-type business events, and preserve event IDs in malformed-record warnings. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 147/147 AI-Contributed/UT: 138/138
Read memory Event payloads from the flat eventAttributes field used by the current Event Log schema. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 0/0 AI-Contributed/UT: 13/13
Define a shared model metadata key for LLM execution reports and use it consistently in Java and Python. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-5 AI-Contributed/Feature: 62/62 AI-Contributed/UT: 19/19
67e3f5b to
5cd9658
Compare
| if (!response.isSuccess() && response.getError() != null) { | ||
| error.put(id, response.getError()); | ||
| } | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Following up on the earlier lifecycle thread: the Action-level path now catches Throwable, but this child-execution boundary still catches only Exception.
JavaFunction.call() now unwraps an InvocationTargetException and rethrows its target Error directly. Consequently, a Function Tool throwing AssertionError or NoClassDefFoundError skips this catch: the Tool execution has already emitted started, but never emits a terminal failed event. The parent Action subsequently fails and clears the active child-execution map, leaving the Tool execution permanently incomplete.
The LLM and parser boundaries in ChatModelAction have the same catch (Exception) shape. Could all three boundaries catch Throwable long enough to report failed, then preserve the original failure when rethrowing? An operator-level regression test using a LinkageError would cover the complete path.
| throw new IllegalArgumentException("Event 'type' must not be null or empty."); | ||
| } | ||
| this.id = id; | ||
| this.id = id != null ? id : UUID.randomUUID(); |
There was a problem hiding this comment.
Could we align the explicit-null Event ID contract between Java and Python?
With this change, Event.fromJson("{\"id\":null,\"type\":\"x\"}") succeeds in Java and silently assigns a new UUID. The equivalent Python Event(id=None, type="x") fails validation because its UUID default is applied only when the field is absent.
The repository guidelines require equivalent Java/Python public APIs to have aligned null/None semantics. Could we choose one contract—either explicit null means “generate an ID” or explicit null is invalid—and add matching contract tests on both sides?
| ToolExecutionMetadataKeys.SKILL_NAME, | ||
| String.valueOf(parameters.getParameter("name"))); | ||
| } | ||
| if (parameters.hasParameter("path")) { |
There was a problem hiding this comment.
Could the execution metadata use the same default-path normalization as call()?
When path is missing or explicitly null, call() loads the default SKILL.md. This metadata path instead converts an explicit null with String.valueOf, recording "skillResourcePath": "null"; Python similarly records "None". The Tool succeeds, but its Trace describes a resource that was not actually loaded, and the Java/Python records also differ.
Could both call() and getToolExecutionMetadata() share the rule missing/null/None -> "SKILL.md" and record that normalized path? Tests for both an omitted path and an explicit null/None would keep execution and observability aligned.
| return AgentPlan( | ||
| actions=actions, | ||
| resource_providers=resource_providers, | ||
| agent_name=agent_name or agent.__class__.__name__, |
There was a problem hiding this comment.
Could we avoid using truthiness to select the default agent name?
Here an explicitly supplied empty string falls back to the Python class name, while the corresponding Java constructor falls back only for null and preserves an empty string. The same public input can therefore produce different agentName values in the serialized plan and Event Log.
Could we either use agent_name if agent_name is not None else agent.__class__.__name__ to match Java, or reject blank agent names consistently on both sides?
|
Reviewed this from the consumer side as well. I have the OTel exporter from #929 / #970 implemented against this PR’s record format and ran its test suite on top of 5cd9658. All 13 tests pass, covering span topology via From the exporter side, the flat record shape, per-occurrence event IDs, and preserved #923 lineage fields are enough to reconstruct the execution DAG without depending on runtime internals. The recording contract looks ready from the consumer side. The exporter PR is ready to open once this lands. |
Linked discussion: #900
Related: #710, #841, #923
Purpose of change
This PR implements the recording side of Agent Trace proposed in #900.
The existing Event Log records business Events but does not provide enough runtime context to identify one input run, distinguish concrete Action/LLM/Parser/Tool executions, reconstruct nested execution relationships, or attribute execution failures.
This PR adds execution identity and lifecycle recording to the existing Event Log path. It complements the business Event lineage implemented in #923 rather than defining another Event-to-Action lineage field:
executionId, connecting the two models.API and trace model
ExecutionTraceContextcarries run identity, execution hierarchy, entity identity, and entity metadata.ExecutionLifecycleEventsdefinesstarted,finished,failed, andreusedlifecycle Events.ExecutionReporterandExecutionReportersprovide an optional, best-effort capability for reporting nested executions.ToolExecutionMetadataProviderallows Tools to contribute small structured execution metadata.EventLoggeraccepts an optionalExecutionTraceContext; its default overload preserves compatibility with existing implementations.AgentPlancarries the Agent name used by trace records.event-log.trace.enabledcontrols Trace persistence and defaults tofalse.Runtime integration
ActionExecutionOperatorcreates one run context per processed input and one execution context per Action invocation.ActionTaskcarries the Action execution context and started-event marker across continuations and Flink state restoration.ActionTaskContextManagerkeeps child-execution start/terminal pairing transient and scoped by Action execution id across live continuation tasks.reusedwhen completed Action state is reused.EventRouter.ExecutionEventSinkwithout automatically submitting them to the business Event routing path.RunnerContextImplimplementsExecutionReporter, creates child execution contexts, and pairs start and terminal reports.ExecutionEventLoggersends Execution Events to the sharedEventLogWriter.EventLogWriterowns logger open, append, flush, and close operations; write failures remain best-effort.Action and resource instrumentation
ChatModelActionrecords one LLM execution per framework model invocation.entityNameremains the ChatModel Resource name.ToolCallActionrecords one Tool execution per concrete tool call, including error responses and thrown exceptions.load_skillcalls record the loaded Skill as Tool execution metadata.Python alignment
ExecutionReportercapability and entity/problem vocabulary as Java.ChatRequestEventoccurrences from colliding in sensory-memory correlation.Event Log format and compatibility
EventLogRecordcombinesEventContext, optionalExecutionTraceContext, andEvent.The JSONL representation is flattened for querying and aggregation. Framework-owned fields retain the existing camelCase convention, including
eventType,eventAttributes,inputRunId, andexecutionId. The record includes Event occurrence fields, run identity, execution hierarchy, entity information, lifecycle status, failure category, and Event attributes.The framework deserializer continues to read the previous nested Event Log format. Existing external consumers that parse the raw JSON shape must migrate to the normalized field names.
Trace persistence is disabled by default. When disabled, business Events continue to be logged without trace context and Execution Events are not persisted.
A restored
ActionTaskretains its run and execution identities. A source-replayed input not represented by restored Action state starts a new run.Fine-grained recovery currently records a cached durable LLM or Tool result as a new successful execution because cache reuse is not exposed to execution reporting. Distinguishing reused child executions is follow-up work.
This PR changes the pending
ActionTaskstate schema. Restoring savepoints containing that state from before this change would require a versionedActionTaskstate serializer, which is outside this PR.Tests
mvn -B --no-transfer-progress -pl runtime -am testAPI
This PR introduces the public Trace APIs described above and extends
EventLoggerwith a backward-compatible default overload accepting optional trace context.The normalized raw Event Log JSON shape is a compatibility change for external consumers. Legacy records remain readable through the framework deserializer.
Documentation
doc-neededdoc-not-neededdoc-included