feat(client)!: put conversation content on spans behind an opt-in flag - #29
feat(client)!: put conversation content on spans behind an opt-in flag#29apucacao wants to merge 3 commits into
Conversation
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 905f873. Configure here.
Python writes the text of every prompt and every model answer onto its spans today, unconditionally, with no way to turn it off. That text is PII, and it leaves for whatever collector the SDK points at whether or not anyone asked for it. TypeScript treats it as opt-in. This adds the layer that lets Python do the same. Nothing changes behaviour yet. The handlers still call the old writers; they move over one package at a time, and that is where the default takes effect. BREAKING CHANGE: once the handlers move onto this layer, prompt and completion content will be absent from spans unless the caller passes capture_content=True. Anyone reading gen_ai.prompt.0.content today will need to opt in. The gate is an argument rather than ambient state, so it is visible at every call site. Handlers check it a second time before building the argument, which looks redundant and is not: the guard here makes a forgotten call site harmless, and the guard there avoids walking a conversation and serialising JSON that would then be discarded, once per model turn, inside a loop. Three carriers hold the same content, deliberately. The canonical GenAI attributes are what the semantic conventions make normative. The OpenLLMetry indexed attributes are the only ones LaunchDarkly's trace view reads today, so canonical alone renders an empty transcript. The legacy span events are redundant and deprecated, but every published version has emitted them and removing them would silently break anyone who learned to read them. All three are written from the same messages behind the same gate, so they cannot disagree. The system prompt goes in twice, its own canonical attribute and index 0 of the OpenLLMetry carrier, because that shape has no slot for it and dropping it there hides the system prompt from the only view that renders. The two legacy events are asymmetric: the output side writes nothing at all when there are no messages, the input side still adds its event. That matches the TypeScript source rather than being tidy, and there is a test saying so, because implementing the two symmetrically is the obvious thing to do and would diverge. to_semconv_finish_reason maps Anthropic's and OpenAI's own words onto one enum. Passing them through untranslated made a consumer grouping by finish reason see `stop` and `end_turn` as two different outcomes for the same event. An unmapped word passes through verbatim rather than being coerced, which is the signal to add a row; pause_turn is deliberately absent, because no value in the enum means "did not finish". The two LangChain helpers live here rather than in each LangChain package, because both need exactly the same conversion and a copy in each package is how the span code drifted apart the last time. The narrowing is structural, so the client takes no dependency on LangChain.
905f873 to
5e76628
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
| "tool_calls": "tool_calls", | ||
| "content_filter": "content_filter", | ||
| "function_call": "tool_calls", | ||
| } |
There was a problem hiding this comment.
Missing Anthropic context-window mapping
Medium Severity
_SEMCONV_FINISH_REASONS omits Anthropic's model_context_window_exceeded, so to_semconv_finish_reason passes it through unchanged. That is another truncation outcome like max_tokens, which already maps to length. Consumers grouping by finish reason will again split one event into two labels, the fragmentation this table is meant to stop.
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
3 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
3 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
3 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
3 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e76628. Configure here.
Every carrier here is written from the same messages so they cannot disagree, and to_canonical omits an absent tool result because the key is simply not there. The OpenLLMetry pair and the legacy content events ran the same part through to_text, which serialised the missing result and rendered the literal text null. That is the pair the LaunchDarkly LLM trace view and conversation view read today, so a tool that returned nothing would be displayed as a tool that returned a null value. Reachable wherever a provider omits the field: an Anthropic tool_result block with no content, or an OpenAI function call result whose output is absent. An absent result now contributes nothing and drops out of the flattened text, which is what to_canonical already reports for it. None is the only spelling of absent here, because to_canonical already reads it that way. The TypeScript SDK can tell an absent result from one explicitly returned as null and reports the second as null; this dataclass cannot hold that distinction, so there is nothing for the two SDKs to disagree about. Two tests: an absent result leaves the transcript empty and agrees with the canonical attribute, and a falsy result that is not absent survives. The first fails without the fix; the second fails if the fix swallows any falsy value. Fixed here rather than after the stack merges, because this layer is what introduces the flattening to Python: a later fix would mean shipping the behaviour first and then changing an attribute's contents on a released package. Matches launchdarkly/js-ai-sdk#21, which fixes the same defect where it is already live. Found by Bugbot on this PR.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8351105. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8351105. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8351105. Configure here.
LangChain types message content as str | list[str | dict], so a bare string inside the list is what the library documents rather than a malformed input. _lang_chain_content_text kept only the blocks whose type is text, so those strings were dropped and the span showed less of the conversation than the model was actually given. Reachable from a caller: history content is passed straight into HumanMessage and AIMessage with no conversion, so whatever shape the caller supplies is the shape this function reads. A bare string now contributes its own text, in the order it appears. Blocks that are not text are still ignored, and a plain string content still passes through unchanged, each with its own test so the fix cannot quietly widen. Fixed here rather than after the stack merges, for the same reason as the tool result before it: this layer is what introduces the function to Python, so a later fix would mean shipping the behaviour first and then changing it on a released package. Matches launchdarkly/js-ai-sdk#22, which fixes the same defect where it is already live. I ran both SDKs over five inputs and they agree on all of them. Found by Bugbot on this PR.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fb5aae3. Configure here.
1 participant
Adds the layer that lets conversation content on spans be opt-in, matching the TypeScript SDK.
Python writes the text of every prompt and every model answer onto its spans today, unconditionally, with no way to turn it off. That text is personal data and it leaves for whatever collector the SDK points at whether or not anyone asked for it.
Additive. The handlers still call the old writers in this PR; they move over one package at a time above, and that is where the default takes effect.
What changed
capture_contentis an argument rather than ambient state, so the gate is visible at every call site.to_semconv_finish_reasonmaps Anthropic's and OpenAI's own words onto one enum. Passing them through untranslated made a consumer grouping by finish reason seestopandend_turnas two outcomes for the same event. An unmapped word passes through verbatim rather than being coerced;pause_turnis deliberately absent, because no value in the enum means "did not finish".Two details that look like bugs and are not, both with tests saying so: the system prompt goes in twice, because the OpenLLMetry shape has no slot for it and dropping it there hides it from the only view that renders; and the two legacy events are asymmetric, because the output side writes nothing for an empty message list while the input side still writes its event, which is what the TypeScript source does.
Breaking change
Declared here, takes effect in the handler PRs above. Once a handler moves onto this layer, prompt and completion content is absent from its spans unless the caller passes
capture_content=True.Where this sits
Builds on the usage layer (#28). All six handler PRs depend on this one.
Tests: 671 to 711.
Note
Overview
Adds a new
contentmodule that records prompts, completions, tools, and finish reasons on OTel spans only when callers passcapture=True(matching the TypeScript SDK’s opt-in model for PII).When enabled, writers emit the same conversation in three aligned carriers: canonical GenAI JSON attributes (
gen_ai.input.messages, etc.), OpenLLMetry indexed attributes (what LaunchDarkly’s trace UI reads today), and legacygen_ai.content.*span events. SharedSpanMessage/SpanMessageParttypes plusset_input_content_attributes,set_output_content_attributes, and tool helpers centralize that logic for upcoming handler migrations.Also adds
to_semconv_finish_reasonand LangChain-orientedlang_chain_finish_reasons/lang_chain_span_messagesso provider-specific finish reasons and message shapes normalize consistently without a LangChain dependency in the client package. Public symbols are re-exported fromlaunchdarkly_ai_server;test_content.pypins the capture gate and carrier behavior. Handler behavior stays unchanged until follow-up PRs wire this in—those will make content absent by default unlesscapture_content=True.Reviewed by Cursor Bugbot for commit fb5aae3. Bugbot is set up for automated code reviews on this repo. Configure here.