fix(voice): one ChatItem serializer that carries every declared field - #2247
fix(voice): one ChatItem serializer that carries every declared field#2247u9g wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 012ee25 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
6db24d2 to
ddea0ef
Compare
A leaf that neither voice nor telemetry sits above, so both can share one encoder. Verbatim otherwise.
The RemoteSession encoder dropped extra and the speaking-at metrics, which the telemetry path and livekit/agents both send. Telemetry kept its own hand-written interfaces with no link to the schema, so it emitted playbackLatency, which the proto does not declare, and camelCase keys where livekit/agents emits proto field names. Both paths now share one encoder and the telemetry log matches livekit/agents field for field. The dropped playbackLatency needs a livekit/protocol field before either SDK can carry it.
Every ChatItem is saturated, encoded, then walked against the message descriptor; anything left unset is a drop, unless allowlisted as a parity gap this SDK cannot fill.
ddea0ef to
012ee25
Compare
| if (metrics.llmNodeTtft !== undefined) report.llmNodeTtft = metrics.llmNodeTtft; | ||
| if (metrics.ttsNodeTtfb !== undefined) report.ttsNodeTtfb = metrics.ttsNodeTtfb; | ||
| if (metrics.e2eLatency !== undefined) report.e2eLatency = metrics.e2eLatency; | ||
| return report; |
There was a problem hiding this comment.
🟡 Playback latency disappears from the recorded conversation logs
The playback delay measured for each agent turn is no longer included when the conversation entry is packaged for upload (encodeMetrics at agents/src/proto.ts:27-43), so this timing value silently vanishes from the observability data that was previously sent.
Impact: Dashboards and downstream analysis lose the per-turn playback latency measurement for every session.
Field dropped when the hand-written telemetry serializer was replaced by the shared proto encoder
The removed telemetry serializer explicitly emitted playbackLatency (old agents/src/telemetry/traces.ts:653-655) into the chat.item log body. The replacement path chatItemSpanAttribute (agents/src/telemetry/traces.ts:521-523) now serializes the protobuf message produced by encodeChatItem, and encodeMetrics never assigns playbackLatency even though MetricsReport in agents/src/llm/chat_context.ts:301 still carries it (populated at agents/src/voice/agent_activity.ts:2732 and :3202).
Either the wire schema declares playback_latency — in which case this is exactly the kind of accidental drop the PR's coverage test is meant to catch and it should be set (or allowlisted with a reason) — or the schema lacks it, in which case the telemetry log loses information it used to carry and the gap should be acknowledged deliberately.
Prompt for agents
The previous telemetry serializer in agents/src/telemetry/traces.ts emitted metrics.playbackLatency for chat items; the new shared encoder encodeMetrics in agents/src/proto.ts does not, so the telemetry chat item log silently loses that value. Check whether AgentSession.MetricsReport in @livekit/protocol declares playback_latency: if it does, set it in encodeMetrics (it is seconds in MetricsReport, same as the other latency fields) so the coverage test in agents/src/voice/chat_item_proto_coverage.test.ts stays honest; if the proto does not declare it, document the loss explicitly (e.g. a comment plus an entry in the gap allowlist rationale) so the regression is a decision rather than an oversight.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function msToTimestamp(ms: number): Timestamp { | ||
| return Timestamp.fromDate(new Date(ms)); | ||
| } |
There was a problem hiding this comment.
🟡 New shared helpers are published without the documentation the project requires
Two newly added shared helpers are exported without any documentation comment (msToTimestamp and encodeChatItem at agents/src/proto.ts:23-45), which the repository's contribution rules require for every new function.
Impact: Generated API documentation for these helpers is empty, and future callers get no guidance on units or behavior.
Rule reference
CONTRIBUTING.md: "If writing new methods/interfaces/enums/classes, document them. This project uses TypeDoc for automatic API documentation generation, and every new addition has to be properly documented." msToTimestamp in particular takes milliseconds while its callers pass seconds-based metrics scaled by 1000 (agents/src/proto.ts:30-33), which is exactly the kind of unit contract a TSDoc comment should state.
| export function msToTimestamp(ms: number): Timestamp { | |
| return Timestamp.fromDate(new Date(ms)); | |
| } | |
| /** | |
| * Convert a millisecond epoch value (`Date.now()` format) into a proto `Timestamp`. | |
| */ | |
| export function msToTimestamp(ms: number): Timestamp { | |
| return Timestamp.fromDate(new Date(ms)); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
u9g
changed the title
1 participant
Companion to livekit/agents#6756, which found the same class of bug in Python: the ChatItem-to-proto mapping was written twice per SDK, and each copy dropped whatever the other one carried.
What was wrong here
chatItemToProtoinvoice/remote_session.tsdroppedextraand the speaking-at metrics on messages, which the telemetry path and livekit/agents both send.The telemetry copy was worse.
telemetry/traces.tsdeclared its ownProtoMessage,ProtoMetricsReport,ProtoChatItemand four more interfaces instead of the generated types, so nothing tied it to the schema. It carried this:agents/package.jsonpins@livekit/protocolat^1.50.4, so that has been unblocked for a while. With no schema link it emittedplaybackLatency, whichMetricsReportdoes not declare, and camelCase keys where livekit/agents emits proto field names — the samechat itemlog body, structurally different payloads.The three commits
refactor: move proto serialization into its own module—agents/src/proto.ts, a leaf that neither voice nor telemetry sits above. Verbatim otherwise.fix(voice): carry every field the wire format declares— one encoder for both paths.chatItemSpanAttributebecomes a one-liner over it, structurally identical to Python's:test(voice): fail when a declared field is dropped— the guard, below.Telemetry payload changes
The emitted
chat itemlog changes in four ways, all of them toward what livekit/agents already sends. Anything querying these logs on camelCase keys or on nestedargumentsneeds updating.functionCall,callId,createdAtfunction_call,call_id,created_atarguments/outputextravalues{n: 1}){n: "1"}), as the proto map requiresplaybackLatencyMetricsReportThe re-parse came in with the original upload path in #851 and was never a considered divergence — the hand-written
arguments: string \| Record<string, unknown>existed to accommodate it. A schema-driven encoder has nowhere to put a parsed object.I checked the result rather than assuming it: encoding all five saturated item types through both SDKs and diffing produces identical payloads, key sets and values.
Tests
chat_item_proto_coverage.test.tssaturates every ChatItem, encodes it, then walks the result against the message descriptor; anything left unset is a drop, unless allowlisted as a parity gap this SDK cannot fill. Since both paths now share the encoder, the guard covers telemetry too.Two entries are allowlisted rather than fixed, because neither is an accidental drop:
metrics.llm_node_tps/metrics.llm_node_ttfs— feat(metrics): broadcast llm tps + ttfs agents#6373 added these in Python and they were never ported, soMetricsReporthere has no field to read them from. A test asserts every allowlist entry names a real proto field, so the list cannot rot into stale excuses.agent_config_updateis a case on the oneof and the encoder fills it, butchatItemsToProtofilters those items out of the chat context the RemoteSession sends. livekit/agents sends them.1593 passing across 116 files;
tsc --noEmitand eslint clean at each of the three commits.Follow-up, not in this PR
playbackLatencystops being emitted here and livekit/agents never sent it. Carrying it from either SDK needs aplayback_latencyfield in livekit/protocol.