feat(telemetry): full OTel GenAI semantic conventions + in-process PII stripping - #7104
Conversation
45033c8 to
45d4e4c
Compare
![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 6 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| # callers pass user data through `extra={"lk.pii.<name>": ...}` precisely | ||
| # because a log body cannot be redacted; drop those before export | ||
| log_record.attributes = pii.redact_attributes(attributes) |
There was a problem hiding this comment.
🟥 Redacted logs expose exception details
Redaction filters only structured attributes, leaving exception text in log bodies. Realtime failures can export customer content to every configured log destination.
Was this helpful? React with 👍 or 👎 to provide feedback.
…I stripping 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. Adds the convention's metrics (gen_ai.client.token.usage split by token type, client.operation.duration, client.operation.time_to_first_chunk, execute_tool.duration, invoke_agent.duration) alongside the lk.agents.* instruments, and sets error.type 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.gen_ai.set_capture_content(False) or OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT=false. PII stripping now happens in-process. telemetry/pii.py registers a span processor ahead of every exporter that removes each lk.pii.* attribute and each GenAI content attribute - whose names the convention fixes, so the marker cannot be applied - from span attributes, span events and log records whenever the session enabled redaction. Previously this only happened at the LiveKit Cloud collector, so a third-party exporter sharing the tracer provider received unredacted content. Also fixes JobContext._otel_metadata, which stamped the redaction flag for the per-session option but not for project-wide redaction.
45d4e4c to
5f8c130
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…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 on that one export path, since what Cloud may keep is the project's setting in the dashboard rather than the SDK's call. `set_tracer_provider(..., allow_pii=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, withholding PII from every destination. This replaces the process-wide set_redaction 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 - invoke_agent duration uses a monotonic clock, matching the tool duration - both _otel_metadata call sites stamp the same resolved redaction flag - gen_ai.output.type follows the session's configured modality instead of assuming speech, since a realtime model can be text-only - the span processor strips exception.message/stacktrace too: record_exception resolves redaction from the ambient context, which can disagree with the span's own stamp - adopted logger providers get a redaction processor, so log exporters the integrator attached are covered like span exporters - gen_ai.tool.definitions omits `parameters`, which the convention marks NOT RECOMMENDED by default, and which cost a schema build per request
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- contains_pii and conversation_id are internal, not public API - remove the section banners in trace_types: the constant names already carry the grouping (gen_ai.request.*, gen_ai.usage.*, ...) - drop a comment that restated the constant names below it - merge two tests that covered the same scenario from two angles
| "grok": "x_ai", | ||
| "mistral": "mistral_ai", | ||
| "moonshot": "moonshot_ai", | ||
| } |
There was a problem hiding this comment.
I don't think the key values match our plugin values. e.g. Mistral LLM reports MistralAI:
There was a problem hiding this comment.
This is part of the spec:
For the 16 enumerated providers (openai, anthropic, mistral_ai, aws.bedrock, gcp.vertex_ai, gcp.gemini, gcp.gen_ai, x_ai, perplexity, groq, cohere, deepseek, azure.ai.*, ibm.watsonx.ai, moonshot_ai) the registry spelling is MUST
There was a problem hiding this comment.
Yes, I was referring to "key" values. I think we missed Amazon.
| @abstractmethod | ||
| async def _run(self) -> None: ... | ||
|
|
||
| def _record_genai_request(self, span: trace.Span) -> None: |
There was a problem hiding this comment.
Codex informed met that the best practice for these data:
Full (buffered) content
Model instructions, user messages, and model outputs are considered sensitive and are often large in size.Recording large or sensitive content in telemetry may be problematic due to high storage costs, regulatory requirements, or the need to enforce different access models for operational and user data.
OpenTelemetry instrumentations SHOULD NOT capture them by default, but SHOULD provide an option for users to opt in.
so it is an opt-in vs opt-out case, wdyt?
| @@ -252,6 +258,7 @@ async def _traceable_main_task() -> None: | |||
| ) as span: | |||
| for name, attributes in _chat_ctx_to_otel_events(self._chat_ctx): | |||
| span.add_event(name, attributes) | |||
There was a problem hiding this comment.
do we still need the for loop here if we record the request in the following line?
…orters by default Review from @chenghao-mou. The provider alias table matched almost nothing the plugins actually report. Of 15 keys exactly one hit: plugins expose `provider` either as a display name ("MistralAI", "AWS Bedrock", "Vertex AI", "Gemini", "xAI") or, for the OpenAI-compatible clients, as the base URL's host ("api.openai.com", "api.anthropic.com", "api.mistral.ai"), while the table was keyed on short ids that were never used. So OpenAI and Anthropic reported a hostname as the provider, which the convention makes a MUST violation — the registry spelling is mandatory 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. A test walks the values the plugins report and asserts each resolves, plus that every mapping targets a real registry value. Content stays on by default and PII now reaches every exporter unless withheld (`allow_pii=False`, or LIVEKIT_TELEMETRY_ALLOW_PII=0): a GenAI backend can only render inputs/outputs and the chat view if it receives them, which is the point of the conventions. The project's redaction setting still overrides the grant. The legacy per-message span events (gen_ai.user.message and friends) duplicate gen_ai.input.messages / output.messages, and are no longer part of the convention — the current event model is gen_ai.client.inference.operation.details. They are kept for the backends that still read them, but now honour the content switch, which previously left them shipping the full chat context after set_capture_content(False).
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.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
There was a problem hiding this comment.
🟥 Sensitive sync errors bypass redaction
Realtime sync failures store str(e) under unmarked error attributes. Provider exceptions can expose customer content in exported logs despite redaction.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
…ep them 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 / _PIIFilteringLogProcessor and made private — nothing outside the framework installs them. redact_attributes is now filter_attributes. "Redaction" is left to mean the project setting (the LiveKit Cloud dashboard, or record={"redaction": True}); this module is what the client does about it, which the module header now states. Also pins the behaviour that decision rests on: with redaction enabled the filtering applies to every destination, LiveKit Cloud included, so the client never depends on a collector knowing a newly added gen_ai key. That already held for spans; the log path was untested, and now is.
The registry was carried over wholesale, which left 35 constants no code references: inference request knobs no plugin exposes, agent id/description/ version, the embeddings/retrieval/memory/evaluation/prompt families for operations LiveKit has no spans for, server.address/port, and three metrics we do not record. A constant we never set adds nothing a backend can read, so this costs no convention coverage — it only removes inventory, and 35 bookkeeping entries from the guard test's safe list. The content attributes of those families stay: they are registered as PII, where the entry is a cheap safety net should a plugin ever set one.
… exception status Review from @chenghao-mou: the AWS realtime model reports "Amazon", which the provider table missed. It runs on Bedrock, so it maps to aws.bedrock. This was the only remaining miss across every provider value the LLM and realtime plugins report. Review from Devin: - finish_reason_for checked function_calls before interrupted, so a generation that emitted a tool call and then failed reported tool_call rather than error. Interruption now wins. - record_exception writes the message onto the span status as well as the attributes and the `exception` event. The status was left intact, so a third-party exporter could still read it; it is now replaced with the same marker. A test asserts the message reaches none of the three.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Regression from the previous commit, caught by Devin: replacing the span status for third-party exporters left LiveKit Cloud with the redacted description too, since restore_pii only put back attributes and events. The status is now stashed alongside them, and the test asserts Cloud still receives the message on all three carriers while third parties receive it on none.
| tracer_provider: trace_api.TracerProvider, | ||
| *, | ||
| metadata: dict[str, AttributeValue] | None = None, | ||
| allow_pii: bool | None = None, |
There was a problem hiding this comment.
Is allow_pii too broad a name? As a user, I read allow_pii=False as “PII reaches no exporter,” but LiveKit Cloud still receives the original content unless project redaction is enabled. The docstring also starts with “this provider’s exporters,” which reinforces that interpretation.
There was a problem hiding this comment.
That’s fair but you only use this method when you setup an OTEL endpoint.
For LiveKit, it’s just there, there is no setup
| s | ||
| # PII stripped for third-party exporters is put back here: what LiveKit Cloud | ||
| # may receive is the project's setting, applied at its collector | ||
| pii.restore_pii(s) |
There was a problem hiding this comment.
nit: should we use
s
if telemetry_utils.redaction_enabled(s.attributes)
else pii.restore_pii(s)I know we have the project redaction check upstream, but if we ever change that part, this is an easy to miss leakage point.
@chenghao-mou's nit: restore_pii relied entirely on nothing being stashed once the project mandates redaction. The check now lives inside restore_pii, so every caller is covered rather than only the gated exporter, and a change to the upstream condition cannot turn into a leak.
@chenghao-mou read allow_pii=False as "PII reaches no exporter", and the docstring opened with "this provider's exporters", which reinforces that: LiveKit Cloud's exporter is attached to the same provider. The name stays, per the discussion on the PR; the wording now names the exemption and points at the dashboard setting that governs Cloud.
…e session Review from Sanjay (Guidewire), on their Datadog Agent Observability integration. llm_node and its child llm_request both claimed gen_ai.operation.name=chat and both carried the usage counts and the full content payload. A backend summing gen_ai.usage.* over inference spans therefore reported 2N calls and twice the tokens for N calls, and the whole chat context plus system prompt plus tool definitions was serialised onto both spans — the largest contributor to span size, growing fastest in the longest sessions. llm_request is the convention's inference operation: it is the provider call, it covers retries, and it only exists when an inference actually happened. llm_node is a framework step that a custom node can satisfy without calling a model at all — it may return a plain str — so it keeps its lk.* attributes and the model/provider identity, but no longer claims to be an inference. Also from the same review: - execute_tool spans now carry gen_ai.conversation.id. A backend that groups by that attribute rather than by trace membership was dropping tool executions out of the session view. - set_usage_attributes emits the unofficial `gen_ai.usage.input_cached_tokens` alongside the registry spelling, as the realtime path already did. #6852 left it off the pipeline path deliberately, which meant cached tokens — usually the largest cost lever in a multi-turn agent — were attributed for realtime sessions and silently absent for pipeline ones. Datadog's documented mapping table keys on the unofficial name.
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.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if provider: | ||
| attrs[trace_types.ATTR_GEN_AI_PROVIDER_NAME] = provider | ||
| if (normalized := trace_types.gen_ai_provider_name(provider)) is not None: | ||
| attrs[trace_types.ATTR_GEN_AI_PROVIDER_NAME] = normalized | ||
| current_span.set_attributes(attrs) | ||
|
|
||
| # the GenAI inference attributes belong to the nested `llm_request` span, which is the | ||
| # provider call the convention describes. Setting them here as well made a backend | ||
| # summing gen_ai.usage.* over inference spans report twice the calls and tokens, and | ||
| # serialised the whole chat context onto both spans. | ||
|
|
||
| llm_node = node(chat_ctx, tools, model_settings) | ||
| if asyncio.iscoroutine(llm_node): | ||
| llm_node = await llm_node |
There was a problem hiding this comment.
🟡 Custom-node setup failures lose telemetry
A custom node that raises while being called or awaited bypasses this handler. Its failed inference receives no GenAI attributes.
Was this helpful? React with 👍 or 👎 to provide feedback.
Regression from moving the inference attributes onto llm_request, reported by Devin. A custom Agent.llm_node can generate the response itself — returning a plain str, streaming its own chunks, or calling a third-party engine — and never construct an LLMStream. Those paths have no nested llm_request span, so the GenAI operation, content, response and usage attributes disappeared from them entirely, which is worse than the double-reporting the move fixed. LLMStream now marks the context when it creates its span, and the llm_node span records the attributes only when nothing did. The marker is a mutable holder in a ContextVar set by the inference task, which owns its context copy, so no reset is needed and concurrent turns cannot see each other's. Verified on all three shapes: the default node reports one inference span on llm_request with the real token counts, and a custom node returning a plain string or its own ChatChunks reports one on llm_node.
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.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| system_instructions=gen_ai_telemetry.to_system_instructions(chat_ctx), | ||
| input_messages=gen_ai_telemetry.to_input_messages(chat_ctx), | ||
| tool_definitions=gen_ai_telemetry.to_tool_definitions(tools), |
There was a problem hiding this comment.
🟡 Mutated prompts corrupt input telemetry
When a custom node edits chat_ctx, _record_uninstrumented_inference serializes the post-call context. Prompts removed before dispatch vanish even when the engine received them.
Was this helpful? React with 👍 or 👎 to provide feedback.
Re-read the convention to confirm before removing: gen_ai.system.message, gen_ai.user.message, gen_ai.assistant.message, gen_ai.tool.message and gen_ai.choice appear nowhere in the current spec — the event model is now gen_ai.client.inference.operation.details and gen_ai.evaluation.result. They were the pre-1.37 way to carry the conversation, superseded by the structured gen_ai.input.messages / gen_ai.output.messages attributes this PR adds. Keeping both meant the full chat history, system prompt and tool definitions were serialised twice on every LLM call — the largest contributor to span size, and the one that grows fastest in long sessions. Only the current form is emitted now (@chenghao-mou, and the second half of Sanjay's duplication comment). The event names stay in trace_types and in the PII event set: nothing of ours emits them, but the filter still drops them should a plugin or an integrator do so, which is the same rule the retrieval and memory content attributes follow. Also from the re-read: gen_ai.agent.name is conditionally required on the execute_tool span and was missing, so tool spans now carry the agent's label. gen_ai.conversation.id there remains an extension rather than a spec attribute — it is not in the execute_tool table, but Datadog groups a session by it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…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 - record the inference when a custom llm_node raises, with an error finish - a plain string return is no longer reported as a streamed response - execute_tool records the invoking agent, which a handoff can have replaced - match only the Bedrock hosts on amazonaws.com
A fallback adapter stands for several models and reports a placeholder for both its model and provider, which would name the adapter rather than anything that serves inference.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| model=getattr(self.llm, "model", None), | ||
| provider=getattr(self.llm, "provider", None), |
There was a problem hiding this comment.
🟡 Fallback agents report placeholder models
With either fallback adapter, set_agent_attributes records its placeholder model and provider. Agent-creation telemetry identifies the adapter instead of an inference model.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
No description provided.