feat(client)!: put conversation content on spans behind an opt-in flag - #29
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.
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 |
|
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 444eeeb. Configure here.
444eeeb to
9aa2934
Compare
|
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 9aa2934. 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.
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.
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.
9aa2934 to
c11adca
Compare
|
bugbot run |
…shing Two defects in the same file, both reachable, neither caught by the tests that were already there. A tool call with no arguments wrote "arguments": null into the OpenLLMetry carrier while to_canonical omitted the key. The module docstring promises the three carriers cannot disagree about one conversation, and here they did: one said the model passed a null argument bag, the other said it passed none. The absent tool result beside it has been handled this way since the null result fix; the tool call was the half that was missed. An empty bag is still written, because a tool called with no arguments is a real thing a model sends and only None means absent. The three content writers also assumed a span. Handlers hold None for every span when the OpenTelemetry extra is absent, so capture_content=True without the extra raised AttributeError from inside the telemetry path, after the provider had already billed the turn. Telemetry may report nothing. It may not break the call it is reporting on. A None span is now a no-op, which is what end_span_once and every other helper in this family already promised. Four tests. Reverting either fix fails one. Found by Bugbot on the content and openai-messages layers.
… the method lang_chain_span_messages read the role off _get_type(). Older LangChain exposed that as the canonical accessor, and the langchain-core these packages depend on replaced it with a plain type field and dropped the method. Nothing raised. getattr returns None for a missing attribute, so every real SystemMessage, HumanMessage and AIMessage fell through to role user and the recorded transcript was simply wrong: the assistant's own turns were attributed to the person talking to it. langchain-messages carried a local adapter for this and said in its docstring that the shared helper was the right place to fix it. langchain-agents called the helper directly and had no way to know it needed one, so its spans were the ones that showed the wrong roles. The method still wins where it exists, because older releases expose both and the method was canonical there. Two tests, one per accessor. Removing the fallback fails one. Found by Bugbot on the langchain-agents layer.
|
bugbot run |
set_tool_definition_attributes writes the tool catalog to a span and never checked it for None, so a caller with capture_content on and the OpenTelemetry extra absent still got an AttributeError from inside the telemetry path. The guard added for the other three writers carried a comment claiming it matched every helper in this family. It covered three of the four. The comment was right about the rule and wrong about the code. The new test walks this module and asserts the rule directly: every public function that writes to a span must check it for None. That is the claim the comment makes, so it is the thing worth testing, rather than the three cases someone happened to remember. A writer added later cannot quietly opt out. Removing the fourth guard fails it. 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 d637b95. Configure here.
|
bugbot run |
A ChatMessage names its speaker in `role` and reports a type of `chat`. The conversion fell through to using the type as the role, so every such turn was recorded as role `chat`, which names the container rather than anybody in the conversation. I checked this by constructing a real langchain_core ChatMessage rather than by reading the code: type is `chat` and role is whatever the caller set. A ChatMessage with no role still falls back to `user`, the same as any other unrecognised type. Two tests. Removing the branch fails one. 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 d56103a. 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 d56103a. Configure here.
2 participants
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 and exports it from the Python client so handlers can record LLM prompts, completions, tools, and finish reasons on spans only when callers passcapture=True(PII off by default).Writers populate three aligned carriers from the same message shapes: OTel GenAI attributes (
gen_ai.input.messages,gen_ai.output.messages, etc.), OpenLLMetry indexed attributes for LaunchDarkly’s trace UI, and legacygen_ai.content.*span events.to_semconv_finish_reasonandlang_chain_finish_reasonsnormalize provider finish reasons;lang_chain_span_messagesconverts LangChain messages without adding a LangChain dependency (fixes role/typeaccessor and content-block handling).Public writers no-op on
span is Noneso missing OpenTelemetry does not break billed turns. This PR is additive—handlers still use old writers until follow-up PRs; the breaking default applies once they switch and omitcapture_content=True.Reviewed by Cursor Bugbot for commit d56103a. Bugbot is set up for automated code reviews on this repo. Configure here.