feat(telemetry): full OTel GenAI semantic conventions + in-process PII stripping - #2411
Conversation
🦋 Changeset detectedLatest commit: 185e602 The changes in this PR will be included in the next version bump. This PR includes changesets to release 38 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}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 5 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| { | ||
| const finishReason = genAI.finishReasonFor({ functionCalls: data.generatedToolCalls }); |
There was a problem hiding this comment.
🟡 Abnormal completions report normal stops
finishReasonFor labels every non-tool, non-interrupted response as stop. Token-limited and filtered responses therefore publish a false successful finish reason.
Was this helpful? React with 👍 or 👎 to provide feedback.
7c9b3c6 to
8ed7f20
Compare
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 3 new potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if (registerSpanProcessor) { | ||
| installPIIRedaction(provider, registerSpanProcessor); |
There was a problem hiding this comment.
🟥 Custom exporters bypass conversation redaction
Without registerSpanProcessor, setTracerProvider cannot install the redactor but still emits captured conversations. Existing exporters receive unredacted customer content.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 4 new potential issues.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| } finally { | ||
| inferenceSpan.end(); |
There was a problem hiding this comment.
🟡 Late realtime metrics split inference spans
inferenceSpan.end() closes the request span before delayed provider metrics can arrive. Late metrics create a separate response-only child span.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
5 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| piiRedactionInstalled.add(provider); | ||
| // PII flows to every exporter unless withheld: the GenAI conventions are only useful to a | ||
| // backend that can render the conversation | ||
| registerSpanProcessor(new PIIFilteringSpanProcessor(allowPii ?? allowPiiFromEnv() ?? true)); |
There was a problem hiding this comment.
🟥 Third-party PII export defaults on
Without explicit configuration, installPIIRedaction permits PII. Existing third-party exporters begin receiving conversations and tool payloads after this upgrade.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 new potential issues.
5 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| failed = true; | ||
| logger.error({ error }, 'error in llm node'); |
There was a problem hiding this comment.
🟨 Inference errors can leak customer data
Failures log the original error under an unmarked key. Provider payloads, credentials, prompts, and stack details can enter non-redactable logs.
Was this helpful? React with 👍 or 👎 to provide feedback.
chenghao-mou
left a comment
chenghao-mou
left a comment
There was a problem hiding this comment.
_PIIFilteringLogProcessor equivalent seems missing
| "LIVEKIT_SIP_OUTBOUND_TRUNK", | ||
| "LIVEKIT_SUPERVISOR_PHONE_NUMBER", | ||
| "LIVEKIT_TELEMETRY_REDACTION", | ||
| "GOOGLE_API_KEY", |
There was a problem hiding this comment.
It is different from LIVEKIT_TELEMETRY_ALLOW_PII
chenghao-mou
left a comment
chenghao-mou
left a comment
There was a problem hiding this comment.
Approving first to unblock release. Left two comments above.
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 new potential issues.
6 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
There was a problem hiding this comment.
🟨 Session redaction can miss telemetry exports
With recording categories disabled, session redaction never stamps existing providers. Third-party exporters can receive customer content despite record.redaction being enabled.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
…I stripping Port of the same change in livekit/agents. Emit the complete `gen_ai.*` attribute set from the OpenTelemetry GenAI semantic conventions (open-telemetry/semantic-conventions-genai) so a LiveKit trace is understood by Datadog Agent Observability, Langfuse and any other GenAI-aware backend without a LiveKit-specific mapping. Span mapping: agent_session -> invoke_workflow, agent_turn -> invoke_agent, llm_request/llm_node -> chat, function_tool -> execute_tool, start_agent_activity -> create_agent, realtime turns -> generate_content with output.type=speech. Existing lk.* attributes and span names are unchanged. error.type is now set on every recorded exception. Message content (gen_ai.input.messages, output.messages, system_instructions, tool.definitions, tool.call.arguments/result) is captured by default, matching the lk.pii.* content already recorded. Turn it off process-wide with telemetry.genAI.setCaptureContent(false) or OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT=false. PII stripping now happens in-process. PIIRedactingSpanProcessor removes each lk.pii.* attribute and each GenAI content attribute - whose names the convention fixes, so the marker cannot be applied - from span attributes and events whenever the session enabled redaction. It runs in onEnding, which the SDK dispatches to every registered processor before any processor's onEnd and while the span is still mutable, so registration order cannot leak: an exporter the integrator attached first is covered too. Previously this only happened at the LiveKit Cloud collector.
…ndings
PII stripping is no longer all-or-nothing. Conversational content, tool payloads
and exception details are stripped in-process before any exporter that is not
LiveKit Cloud's; the pre-redaction payload is stashed and restored in
UploadGateTraceExporter, since what Cloud may keep is the project's setting in
the dashboard rather than the SDK's call. `setTracerProvider(provider, {
allowPii: true })` grants a provider's exporters the content
(LIVEKIT_TELEMETRY_ALLOW_PII for setups that adopt the ambient provider and have
no call site), and the project's redaction flag overrides that grant.
This replaces the process-wide setRedaction switch, which could weaken a
project-mandated redaction and implied the SDK could grant Cloud something the
dashboard had not.
Review fixes:
- realtime turns get a nested `realtime_inference` span, so the inference
attributes and the provider metrics no longer overwrite the agent_turn span's
`invoke_agent` identity
- recordRealtimeMetrics no longer reports `metrics.label` ("openai_realtime") as
the model; it uses the metadata the metrics already carry, matching Python
- gen_ai.output.type follows the session's configured modality instead of
assuming speech, since a realtime model can be text-only
- an aborted or failed llm_node reports finish_reason `error`, not `stop`
- the span processor strips exception.message/stacktrace too
- telemetry/gen_ai.ts no longer imports llm at runtime: it dragged the whole llm
graph in behind the telemetry barrel, which job.ts imports, breaking partial
vi.mock()s and inverting the layering
- the span-stash helpers live in the import-free redaction.ts for the same reason
- gen_ai.tool.definitions omits `parameters`, which the convention marks NOT
RECOMMENDED by default
setTracerProvider still never calls addSpanProcessor: when an integrator's
provider offers no registrar we warn that redaction cannot be installed rather
than attaching to their provider uninvited.
- collapse a double filter over span events in the redaction processor - align the trace_types header with the Python side and remove the section banners: the constant names already carry the grouping - drop a comment that restated the constant names below it - merge two tests that covered the same scenario from two angles
…orters by default Port of the Python fix for @chenghao-mou's review. The provider alias table matched almost nothing the plugins report: they expose `provider` either as a display name ("MistralAI", "Vertex AI", "xAI") or as the client's base-URL host ("api.mistral.ai"), while the table was keyed on short ids that were never used. The convention makes the registry spelling a MUST for an enumerated provider, since backends use the attribute as the discriminator for provider-specific parsing. Both shapes are now mapped: by host (with suffix rules for Azure, Bedrock and Vertex endpoints), then by the display name reduced to lowercase alphanumerics. A provider outside the registry keeps its own id, which the convention allows. Content stays on by default and PII now reaches every exporter unless withheld (`allowPii: false`, or LIVEKIT_TELEMETRY_ALLOW_PII=0): a GenAI backend can only render inputs/outputs and the chat view if it receives them. The project's redaction setting still overrides the grant. The changeset is a patch again, since nothing an existing exporter receives today is taken away.
…p it internal Review from @chenghao-mou: the SDK removes whole fields, it does not mask identifiable entities within them, and "PIIRedactingSpanProcessor" read like the latter. Renamed to PIIFilteringSpanProcessor, redactAttributes to filterAttributes, and dropped both from the public barrel along with isPIIAttribute — the processor installs itself and none of it is API. "Redaction" is left to mean the project setting; this module is what the client does about it, which the module header now states.
Same trim as the Python side, plus the ones dead only here: all eight GenAI metric names (this package records no metrics — they were copied across), gen_ai.token.type, and the per-modality cache/image token counts the realtime metrics never populate. Also removes filterAttributes, left unused once the processor started deleting keys in place. A constant we never set adds nothing a backend can read, so this costs no convention coverage. The content attributes stay: they are registered as PII, where the entry is a cheap safety net should a plugin ever set one.
… exception details Port of the Python fixes, plus one that only applied here. - the AWS realtime model reports "Amazon", which the provider table missed; it runs on Bedrock, so it maps to aws.bedrock (@chenghao-mou) - finishReasonFor checked functionCalls before interrupted, so a generation that emitted a tool call and then failed reported tool_call rather than error - the `exception` event's message and stacktrace, and the span status message, were left intact for third-party exporters. Python already filtered the event; neither filtered the status. Both now do, with a test asserting the message reaches none of the three.
Same regression as the Python side: replacing the span status for third-party exporters left LiveKit Cloud with the redacted description, since restorePii only shadowed attributes and events. The status is stashed alongside them now, and the stashed events are deep-copied so filtering a kept event's attributes in place no longer mutates what Cloud gets back.
@chenghao-mou: - turbo.json still declared LIVEKIT_TELEMETRY_REDACTION, left over from the set_redaction design that allow_pii replaced. Lint could not catch it because the variable is read through an identifier rather than a literal. - the _PIIFilteringLogProcessor equivalent was missing. PIIFilteringLogProcessor is now exported alongside the other log processors, and the pino exporter — which is this package's only log path — filters PII keys itself once the project mandates redaction, rather than leaving them to the collector. The classification (isPIIAttribute and its sets) moved into the import-free redaction module so the log paths can use it: logging.ts and pino_otel_transport.ts sit near the top of the telemetry barrel, which job.ts imports, so reaching pii.ts from them would have pulled the whole job graph in behind it.
The framework-owned provider is constructed with the filtering processor in its spanProcessors, but never recorded as installed. The setTracerProvider call immediately after therefore found no registrar and warned that redaction could not be installed — on the one path where it demonstrably had been. Reported by Devin. The assertion is folded into the existing default-provider test rather than added as its own: that describe can only call setupCloudTracer once, since the provider is set-once and the cloud telemetry state is module-level.
0ed53d9 to
a7f2924
Compare
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 3 new potential issues.
6 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| stash[RAW_ATTRIBUTES] = { ...span.attributes }; | ||
| stash[RAW_EVENTS] = span.events.map((event) => ({ | ||
| ...event, | ||
| attributes: { ...event.attributes }, | ||
| })); | ||
| stash[RAW_STATUS] = { ...span.status }; |
There was a problem hiding this comment.
🟡 Later attributes disappear from Cloud
When later onEnding processors add attributes, stashPii has already copied the span. Cloud restoration replaces those additions with the stale snapshot.
Prompt for agents
Change PII stashing/restoration in agents/src/telemetry/redaction.ts so it preserves only removed or replaced sensitive fields, events, and status details rather than snapshotting the entire span. restorePii must overlay the stashed sensitive values onto the final ReadableSpan, retaining safe attributes and event changes made by later onEnding processors.
Was this helpful? React with 👍 or 👎 to provide feedback.
| registerSpanProcessor: SpanProcessorRegistrar | undefined, | ||
| allowPii: boolean | undefined, | ||
| ): void { | ||
| if (piiRedactionInstalled.has(provider)) return; |
There was a problem hiding this comment.
🟡 PII policy updates are ignored
After one setTracerProvider call installs filtering, later calls cannot change allowPii. The provider keeps its original content policy.
Prompt for agents
Make allowPii reconfiguration explicit for repeated setTracerProvider calls on the same provider. Since installed span processors cannot simply be removed, keep mutable per-provider policy state that the single PIIFilteringSpanProcessor reads, or reject policy changes clearly. Do not silently retain the first value.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }); | ||
| } | ||
| } catch (rawError) { | ||
| logger.error( |
There was a problem hiding this comment.
🟥 Tool errors expose customer content
Tool exception messages are logged under error without a PII marker. Provider payloads or user data inside the message bypass field redaction.
Was this helpful? React with 👍 or 👎 to provide feedback.
…s in session Port of the Python fixes for Sanjay's (Guidewire) review, plus the trace-shape one that only applies here. - llm_node and its child llm_request both claimed to be the inference operation and both carried usage and the full content payload, so a backend summing gen_ai.usage.* reported twice the calls and tokens and the chat context was serialised twice. llm_request owns the inference now; llm_node keeps its lk.* attributes and the model/provider identity. - execute_tool spans carry gen_ai.conversation.id. - setUsageAttributes emits the unofficial cached-token spelling alongside the registry one, matching the realtime path and Datadog's mapping table. - start/resume/drain_agent_activity were detached to ROOT_CONTEXT, which made them separate traces and broke "one trace per agent session". They are parented to the session root instead, which keeps them out of whichever speech task is current while keeping them in the session's trace. The comment claiming this matched Python was wrong: Python inherits the session context there.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Port of the Python fix for the regression Devin reported. A custom Agent.llmNode can return its own ReadableStream without constructing an LLMStream, so there is no nested llm_request span and the GenAI attributes disappeared from those calls. LLMStream marks the active context in its constructor — not in mainTask, which startSoon defers out of the node's context — and performLLMInference records the attributes on the node span only when nothing did. The existing generation_telemetry test already drives a custom node, so it now also asserts the node span carries the operation, finish reason and output messages.
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
7 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| const finishReason = genAI.finishReasonFor({ | ||
| functionCalls: data.generatedToolCalls, | ||
| }); |
There was a problem hiding this comment.
🟡 Cancelled generations report successful completion
When the abort signal ends a custom generation, nodeError remains unset and finishReasonFor records stop. Cancelled responses look successfully completed.
Was this helpful? React with 👍 or 👎 to provide feedback.
…bution - create_agent carries the required gen_ai.provider.name and the model - only credit the configured model/provider when that LLM served the call - a failed or aborted custom node no longer reports a successful finish - execute_tool records the invoking agent, which a handoff can have replaced - match only the Bedrock hosts on amazonaws.com
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…spans - a fallback adapter names no single model, so create_agent omits both fields - error.type never carries a thrown non-Error value verbatim Also releases the GenAI semantic conventions as a minor.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
6 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| model: this.llm?.model, | ||
| provider: this.llm?.provider, |
There was a problem hiding this comment.
🟡 Fallback agents report nonexistent models
With FallbackAdapter, the create-agent span reports FallbackAdapter as its model and unknown as its provider. Telemetry attributes the agent to services that never perform inference.
Prompt for agents
The create-agent span now copies model and provider from every configured LLM. agents/src/llm/fallback_adapter.ts exposes wrapper placeholders (`FallbackAdapter` and inherited `unknown`) because it represents multiple child models and can choose a different provider per request. Avoid setting model and provider on the create-agent span for this multi-model adapter, while preserving both attributes for single-model LLM and realtime configurations. Add coverage for both cases.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
No description provided.