Skip to content

feat(llmobs): carry typed messages with media on non-LLM span kinds - #19805

Open
joizddog wants to merge 7 commits into
mainfrom
jose/mlob-6408-nonllm-media-widening
Open

feat(llmobs): carry typed messages with media on non-LLM span kinds#19805
joizddog wants to merge 7 commits into
mainfrom
jose/mlob-6408-nonllm-media-widening

Conversation

@joizddog

@joizddog joizddog commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

feat(llmobs): carry typed messages with media on non-LLM span kinds

Labels: semver-minor

What does this PR do?

Lets agent, workflow, task, step and tool spans keep typed messages alongside their
collapsed value string, so image_parts and audio_parts annotated on them survive instead of
being flattened into text.

Precedent

This is the tracer-side payoff of work already merged:

That read-side change was the only thing gating this. The enabled set was deliberately built as a
single frozenset so that widening it would be a one-line follow-up once the serving API caught up.

Why it matters

Measured on dd.instrumentation_telemetry_data.mlobs.span.finished, 30d, {!org_id:2}, aggregator
sum:

span kind 30d spans share of all spans
task 920,508,446 31.9%
workflow 450,411,867 15.6%
tool 301,619,225 10.5%
agent 152,490,003 5.3%
step 19,821,873 0.7%

Before this, only llm (28.0%) and agent could carry typed media. These kinds are 58.7% of all
LLM Observability spans
, and media annotated on them was silently stringified into the value.

Changes

  • _utils.pyMEDIA_MESSAGE_SPAN_KINDS widened from ("agent",) to
    ("agent", "workflow", "task", "step", "tool"), with the comment updated to record that the
    serving-API precondition is now met.
  • Everything else (the media predicate, the value/messages coexistence, the collapse helper and the
    media stripping) shipped with the agent-span work in this same branch and is unchanged.

Two things

tool keeps the JSON value form. _SCALAR_VALUE_SPAN_KINDS is
("agent", "workflow", "task", "step") — deliberately no tool — so a lone plain message collapses
to bare text for the other kinds while tool stays JSON. That mirrors the trace indexer's
nonLLMValueString. The test parametrize encodes the expected value per kind rather than assuming
they are uniform.

step has no public context manager. LLMObs exposes llm, tool, task, agent,
workflow, embedding and retrieval, so step spans are integration-created only. It is
included because the read side handles it; the branch is simply unreachable from the public API and
therefore untested here.

Size guarding

This PR adds no size guard and bypasses none. Media only reaches a span through the integration
capture paths, which already apply the per-image budget from _capture_inline_image (#19690) or
format_image_part_with_guard (#19148); an over-budget image degrades to a marker before it can push
the event over the cap.

What does change is exposure: four more span kinds can now carry media, so there are more places for
payloads to accumulate within a trace. The cumulative case remains open — N images that each fit
the per-image budget can together exceed the per-event limit — exactly as on the merged PRs, and it
stays tracked under MLOB-6408 as a writer-side fix. Flagging it explicitly so it is not
re-litigated here.

Testing

scripts/run-tests --venv 8f50d1d -- -- -v -k "media or image_parts or audio_parts"

26 passed, 0 failed. Covers:

  • media on workflow / task / tool now emitting typed messages, with the per-kind value shape
  • regression pin: media-free spans of those kinds unchanged, so widening the set cannot leak
    messages onto spans that should not have them
  • all pre-existing agent-media cases (attachment_key, multi-message JSON value, lone system message,
    tool-call structure, malformed parts raising) still green

Follow-up, deliberately not here

dd-trace-js has the same gap: sdk.js routes only spanKind === 'llm' to the typed-message tagger,
and its tagLLMIO / tagTextIO are mutually exclusive, so messages and value cannot coexist there
yet. That needs the JS equivalent of the collapse-and-strip work in this branch and is its own PR.

Only span_kind llm could route to typed messages, so image_parts and
audio_parts annotated on an agent span were JSON-stringified into
meta.input.value and the structure was lost. Route agent spans carrying
media through the message tagger, keeping the collapsed value alongside.

The enabled set is a single frozenset so widening to the other non-LLM
kinds is a one-line change once the serving API populates Messages for
them.
The serving API now populates Messages for workflow, task and step via
defaultSpanFromEvent and for tool via toolSpanFromEvent, so these kinds can
keep typed messages alongside the collapsed value string. Widen the enabled
set accordingly and flip the regression pin that asserted they stayed on the
value-only path.

tool is not a scalar-value kind, so it keeps the JSON value form while
workflow and task collapse a lone plain message to bare text. The test
parametrize encodes that per kind.
@joizddog
joizddog requested a review from ZStriker19 August 21, 2026 00:56
@datadog-official

datadog-official Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🔄 Datadog auto-retried 1 job - 1 passed on retry View in Datadog

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: dcfcdc4 | Docs | View more details | Give us feedback!

@ZStriker19 ZStriker19 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-reviewed this with two independent reviewers (Claude code-review at high effort, plus two Codex runs) and validated every finding against the checked-out source. The approach is sound and the happy-path test coverage is good, but the routing trigger is applied to unvalidated user input on a path that previously could not fail, which produces one blocking regression plus several data-loss and data-retention issues. Six inline comments below.

Note the branch is behind its base and no longer applies cleanly, so it will need a rebase.

Comment thread ddtrace/llmobs/_llmobs.py
)
elif span_kind == "experiment":
cls._tag_freeform_io(span, input_value=input_data, output_value=output_data)
elif span_kind_keeps_messages(span_kind, input_data) or span_kind_keeps_messages(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this branch can raise into user application code.

messages_carry_media only checks that the media key holds a non-empty list, never that its entries are dicts. So image_parts: ["/tmp/a.png"], audio_parts: ["not-a-dict"], and a valid part sitting next to a non-string content all get diverted here, Messages() raises TypeError, and line 3117 converts it to LLMObsAnnotateSpanError. Pre-PR every one of those inputs went to _tag_text_io and could not raise.

The blast radius is decorators.py. _llmobs_decorator calls LLMObs.annotate at lines 237, 258, 261, 286, 316 and 319 with no try/except, and _automatic_io_annotation defaults to True. On the input side the annotate call precedes resp = func(*args, **kwargs), so the user's function body never runs.

Two fixes, both worth doing:

  1. In messages_carry_media, require the media list's elements to be dicts, so malformed payloads stay on the pre-PR _tag_text_io path. That matches the docstring's stated intent of leaving malformed input on whatever path it takes today.
  2. Wrap the six _llmobs_decorator annotate calls in try/except LLMObsAnnotateSpanError: log.debug(...), mirroring the guard _model_decorator already has at decorators.py:119.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. messages_carry_media now requires media elements to be dicts so non-dict payloads stay safely on the _tag_text_io path.

For the decorator calls, I added a _annotate_or_log helper to keep all six guards clean without repeating try/except blocks, and added test coverage.

Comment thread ddtrace/llmobs/_llmobs.py

input_messages = llmobs_input.get(LLMOBS_STRUCT.MESSAGES)
if span_kind == "llm" and input_messages is not None:
if input_messages is not None and span_kind_keeps_messages(span_kind, input_messages):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A later annotate() no longer overrides an earlier media annotation.

annotate(input_data=[media msg]) followed by annotate(input_data="corrected") emits the first one. This line flips input_type back to "messages" from the stale messages, and line 550 then regenerates value from them. Neither writer clears its sibling key (there is no pop of MESSAGES or VALUE anywhere in ddtrace/llmobs/), and meta.input is a read-modify-write struct that persists across annotate calls, so the two representations coexist. This breaks the override contract documented at line 2929, and a caller re-annotating specifically to redact leaves the original media payload in place.

Fix: in _annotate_llmobs_span_data, have the value path pop(LLMOBS_STRUCT.MESSAGES) and the messages path pop(LLMOBS_STRUCT.VALUE), so only one representation is ever live. Worth a test for the two-call override sequence in both orders, since the reverse order currently only works by accident.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as suggested. _annotate_llmobs_span_data now pops the sibling key (MESSAGES or VALUE) on every write so only one representation remains live.

Added tests covering both re-annotation orders across all four span kinds.

Comment thread ddtrace/llmobs/_llmobs.py Outdated
# value tagging it has today rather than being reshaped into a message.
media_input = input_data if span_kind_keeps_messages(span_kind, input_data) else None
media_output = output_data if span_kind_keeps_messages(span_kind, output_data) else None
annotation_error_message, error = cls._tag_llm_io(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding one image part silently erases the rest of a freeform payload.

Messages.__init__ rebuilds each message from a strict whitelist (content, role, tool_calls, tool_results, audio_parts, image_parts) and drops everything else with no log or error. Because collapse_messages_to_value derives value from those already-stripped messages, the dropped keys are gone from both fields:

input_data = {"user_id": 42, "query": "find me a hat", "image_parts": [<valid part>]}

before: value = '{"user_id": 42, "query": "find me a hat", "image_parts": [...]}'
after:  messages = [{"content": "", "role": "", "image_parts": [...]}]
        value    = ''

image_parts is a plausible key in arbitrary user data, so a caller who never intended media semantics gets a blank span, and telemetry.record_llmobs_annotate still records error=None.

Fix: requiring dict entries in messages_carry_media handles the malformed cases. For the valid-media case above, either carry unrecognized keys through Messages or log.warning when the strict path drops any, so the loss is not silent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Malformed cases are safely routed to text by the dict-entry check, while valid media payloads now trigger a log.warning when unrecognized keys are dropped.

I also added an upgrade entry to the release note detailing this behavior change.

Comment thread ddtrace/llmobs/_utils.py
return span_kind in MEDIA_MESSAGE_SPAN_KINDS and messages_carry_media(messages)


def _strip_media_parts(messages: list[Message]) -> list[dict]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Media parts bypass redaction span processors.

Stripping media here means value loses it while messages keeps it, and post-PR the processor receives the real typed message list instead of the old single collapsed Message. A processor that scrubs content therefore never touches the media. This repo's own documented idiom shows the effect: tests/llmobs/test_llmobs.py:177 does for message in span.input + span.output: message["content"] = "", which now ships messages: [{"content": "", "image_parts": [{...base64...}]}] while value reads "". The span looks scrubbed but is not.

LLMObsSpan.input is publicly typed list[Message] and Message declares image_parts, so a strictly correct processor should already handle this, but no existing processor expects media on an agent span. Suggest calling it out in the release note as a behavior change for processor authors, and adding one test that registers a processor against a media-bearing non-LLM span.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added test_content_scrub_leaves_media_on_non_llm_span to assert how scrubbed spans behave when media is present.

Updated the release note with an upgrade entry advising processor authors to explicitly pop image_parts and audio_parts keys to scrub media.

Comment thread ddtrace/llmobs/_llmobs.py
# Non-LLM spans carry both: value for readers that only understand value, and
# messages for the typed media parts value cannot represent. Derived here, after
# the user span processor, so processor edits reach both fields.
meta_input[LLMOBS_STRUCT.VALUE] = collapse_messages_to_value(span_kind, llmobs_span.input)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicating the text halves the size headroom, and truncation then drops the media too.

messages and the derived value both carry the text, with no size guard. Measured on an agent span with a 4 MiB image part (the existing LLMOBS_IMAGE_INLINE_MAX_BYTES cap) against the 5,000,000 byte default:

text=390KB   before 4,594,247 ok          after 4,993,895 ok
text=400KB   before 4,604,487 ok          after 5,014,375 truncated
text=780KB   before 4,993,607 ok          after 5,792,615 truncated

Past the limit _truncate_span_event (_writer.py:1161) replaces meta.input and meta.output wholesale, so the field added to carry media is exactly what gets dropped. The budgets in _integrations/utils.py:401 do not cover this: they guard only integration capture paths, which are all kind="llm" and never duplicate.

Fix: skip the derived value (or emit a short marker in its place) when it would push the projected event size past config._llmobs_event_size_limit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. collapse_messages_to_value now checks projected event sizes against _llmobs_event_size_limit and emits [value omitted: event size limit] if exceeded, preserving the typed media.

Added tests to verify the omission trigger and prevent false positives on standard payloads.

---
fixes:
- |
LLM Observability: Fixes an issue where images and audio annotated on non-LLM spans were lost.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two behavior changes are missing from this note, and the annotate docstring is now stale.

"Spans without media, and non-message input, are unchanged" reads as a no-behavior-change guarantee, but for agent/workflow/task/step/tool this PR also (a) drops non-whitelisted keys from message-shaped payloads and (b) makes LLMObs.annotate able to raise. Both are worth stating, along with the span-processor shape change.

Separately, per AGENTS.md rule 11 ("Update docs when changing internal or public APIs"), LLMObs.annotate's docstring still reads - other: any JSON serializable type. for input_data (_llmobs.py:2962) and output_data (:2975), and scopes all image_parts/audio_parts documentation to llm spans. That line is now false for the five widened kinds.

Minor: the PR title is feat(...) but the note is filed under fixes:.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed all three. Updated the release note with features: and upgrade: sections, removed the outdated guarantee sentence, and corrected the annotate docstrings for non-LLM spans.

Keep the media routing trigger from reaching unvalidated user input on a path
that previously could not fail, and stop the two representations of a span's
I/O from diverging.

- messages_carry_media now requires media list entries to be dicts, so a
  malformed payload stays on the value path instead of reaching Messages()
  and raising into the caller.
- Route the decorator's automatic annotations through a guarded helper, so an
  annotation failure cannot take down the function being traced.
- Clear the sibling key when writing input/output, restoring the documented
  last-write-wins contract; re-annotating to redact no longer leaves the
  original media in place.
- Warn when a message-shaped payload carries keys outside the message schema,
  rather than dropping them silently.
- Omit the derived value string when duplicating the text would take the event
  past the size limit, since the writer would otherwise drop the whole input
  and output along with the media.
- Document the widened kinds on annotate and record the behavior changes in
  the release note.
@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codeowners resolved as

Resolved from the full PR diff against main using the target branch CODEOWNERS file.
CODEOWNERS team requests not listed below are not required by the current file set.

ddtrace/llmobs/_constants.py                                            @DataDog/ml-observability
ddtrace/llmobs/_llmobs.py                                               @DataDog/ml-observability
ddtrace/llmobs/_utils.py                                                @DataDog/ml-observability
ddtrace/llmobs/decorators.py                                            @DataDog/ml-observability
ddtrace/llmobs/utils.py                                                 @DataDog/ml-observability
releasenotes/notes/llmobs-agent-span-media-parts-fc6e57c41f7c178f.yaml  @DataDog/apm-python
tests/llmobs/test_llmobs.py                                             @DataDog/ml-observability
tests/llmobs/test_llmobs_decorators.py                                  @DataDog/ml-observability
tests/llmobs/test_llmobs_service.py                                     @DataDog/ml-observability

@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 21, 2026

Copy link
Copy Markdown

Circular import analysis

⚠️ Existing circular imports

There are 3 circular imports that already exist on the base branch and have not been changed by this PR.

ddtrace.llmobs -> ddtrace.llmobs._evaluators -> ddtrace.llmobs._evaluators.format -> ddtrace.llmobs._experiment -> ddtrace.llmobs
ddtrace.errortracking._handled_exceptions.bytecode_injector -> ddtrace.errortracking._handled_exceptions.callbacks -> ddtrace.errortracking._handled_exceptions.collector -> ddtrace.errortracking._handled_exceptions.bytecode_reporting -> ddtrace.errortracking._handled_exceptions.bytecode_injector
ddtrace.appsec._asm_request_context -> ddtrace.appsec._iast._iast_request_context_base -> ddtrace.appsec._iast._iast_env -> ddtrace.appsec._iast.reporter -> ddtrace.appsec._exploit_prevention.stack_traces -> ddtrace.appsec._asm_request_context

@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 21, 2026

Copy link
Copy Markdown

Dependency direction analysis

⚠️ Existing dependency direction violations

There are 250 dependency direction violations that already exist on the base branch and have not been changed by this PR.

Show existing violations (showing 5 of 250 highest severity)
ddtrace.internal.tracemethods -×-> ddtrace.trace  (internal-core -> product:tracing, score=135)
ddtrace.internal.ci_visibility.recorder -×-> ddtrace.trace  (product:ci_visibility -> product:tracing, score=133)
ddtrace.debugging._exception.replay -×-> ddtrace.trace  (product:debugging -> product:tracing, score=133)
ddtrace.internal.opentelemetry.span -×-> ddtrace.trace  (product:opentelemetry -> product:tracing, score=133)
ddtrace.llmobs._integrations.crewai -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=133)

To see all violations, download the layers-base.json and layers-pr.json artifacts from this CI job and run:

uv run --script scripts/import-analysis/layers.py compare layers-base.json layers-pr.json

@joizddog
joizddog marked this pull request as ready for review August 21, 2026 15:01
@joizddog
joizddog requested review from a team as code owners August 21, 2026 15:01
@joizddog
joizddog requested a review from P403n1x87 August 21, 2026 15:01

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dcc30aa84b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread ddtrace/llmobs/decorators.py Outdated
Comment thread ddtrace/llmobs/_utils.py
Comment thread ddtrace/llmobs/utils.py Outdated
Comment thread ddtrace/llmobs/_llmobs.py
Four issues raised on the previous commit, each verified against the code
before fixing.

The output guard in the non-LLM decorators only checked output.value. Media
annotated manually lands on output.messages and leaves value unset, so the
guard read it as unannotated and the automatic annotation then cleared the
messages, losing the caller's media. Guard on both, matching the LLM decorator.

A dict now counts as a message only when it carries content or role. The
decorators annotate a traced function's arguments as a plain dict, so a
parameter named image_parts otherwise took every other argument down the
message path, which drops what it does not recognize.

The public Messages wrapper is unwrapped before the media check. It previously
fell through to value tagging and serialized as an object repr, losing both the
media and the message content.

Logging the dropped message keys coerces them to strings first. A mix of key
types made sorted() raise inside the message constructor, which rejected the
whole message instead of dropping the unknown keys as documented.

@ZStriker19 ZStriker19 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at dcc30aa with the same two-reviewer cross-check. Verified each of the six previous points against the source rather than the replies: the sibling-key fix (stale messages) is genuinely fixed and the decorator crash path is closed, which were the two biggest ones. Thanks for the fast turnaround.

The fixes did introduce some new issues, though, and two of them are more severe than anything in the first round. Six inline comments below. Two of them independently match what the codex bot already posted, which I have noted inline.

Also worth an explicit decision rather than a fix: test_content_scrub_leaves_media_on_non_llm_span pins the media-past-scrubber behavior as intended. Running that scenario against pre-PR sources gives meta.input = {"value": ""} with the media gone, so existing content-redaction processors will start emitting media on five span kinds. The upgrade: note is the right disclosure, but it is a behavior change users should sign off on, not a fix.

if _automatic_io_annotation and resp is not None and operation_kind != "retrieval":
if get_llmobs_output_value(span) is None:
LLMObs.annotate(span=span, output_data=resp)
if get_llmobs_output_value(span) is None and get_llmobs_output_messages(span) is None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sibling-key pop deletes manually annotated media here.

This gate checks value only. A media write now pops output.value, so the gate reads None, the automatic annotation overwrites the user's, and its value path then pops messages.

@agent
def run(q):
    LLMObs.annotate(output_data=[{"role": "assistant", "content": "here it is",
                                  "image_parts": [{"mime_type": "image/png", "content": "AAAA"}]}])
    return "plain return value"

After the manual annotate meta.output holds only messages, the gate passes, and meta.output becomes {"value": "plain return value"}. The media is gone. Same at line 285 in the async wrapper. Pre-PR the manual annotate wrote output.value, the gate saw it, and the decorator skipped.

Fix: check get_llmobs_output_messages(span) is None as well, the way _model_decorator already does at line 134. It is already imported at line 16.

The codex bot flagged this independently, as did a third reviewer, so it is worth prioritising.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b3fc5a99. The decorator check now verifies both get_llmobs_output_value and get_llmobs_output_messages are None before auto-annotating, matching _model_decorator.

Added test coverage across agent, workflow, task, and tool decorators.

Comment thread ddtrace/llmobs/utils.py Outdated
log.warning(
"Dropping message keys %s: not part of the LLM Observability message schema. "
"Record additional fields with LLMObs.annotate(metadata=...).",
# Coerced before sorting: keys reaching here are unvalidated, and a mix of

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A diagnostic log line can now reject a valid message, including on LLM spans.

sorted() is an argument expression, so it evaluates before log.warning is entered, even when logging is disabled. Two unrecognized keys of non-orderable types raise TypeError, which _tag_llm_io catches as "Failed to parse input messages" and converts into LLMObsAnnotateSpanError.

This is not confined to the media path. Messages.__init__ is the shared path for every llm annotation, so {"content": "hi", "role": "user", 1: "x", "extra": "y"} on a plain llm span returned a valid message before this PR and raises after it.

Fix, two parts:

  1. Stringify before sorting: sorted(map(repr, unrecognized)).
  2. Make the whole key-drop diagnostic non-fatal, so any failure inside it degrades to a log.warning and the message is still recorded rather than rejected. A logging statement should never be able to fail an annotate call, and wrapping it is cheap insurance against the next variant of this.

The codex bot flagged the sorted crash too but scoped it to the media path. The llm path is the wider exposure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b3fc5a99. Key coercion was updated and wrapped so formatting failures can no longer crash annotate calls, ensuring diagnostic logging remains strictly non-fatal.

Verified that unorderable message key combinations now pass cleanly without throwing TypeError.

Comment thread ddtrace/llmobs/_utils.py
if isinstance(messages, Messages):
messages = messages.messages
if isinstance(messages, dict):
messages = [messages]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A parameter named image_parts makes the decorator discard every other argument.

_get_span_inputs yields {argname: value}, and this predicate treats a bare dict as a single message, so the parameter name alone routes the whole argument map through Messages.

@agent
def render(image_parts, user_id, prompt): ...

render(image_parts=[{"mime_type": "image/png", "content": "AAAA"}], user_id=5, prompt="go")

Result: meta.input.messages keeps only the media, value collapses to "", and user_id and prompt are dropped. The span reads as having no input at all. Before this PR value held all three arguments.

Fix: force decorator-generated argument maps down the value path. They are a {param: value} mapping and never a message payload, so passing a flag through _annotate_or_log to skip the media routing would do it. Inferring message semantics from a key name is the underlying fragility here, and this is the case where it misfires on completely ordinary code.

Also flagged by the codex bot and by a third reviewer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b3fc5a99. messages_carry_media now requires a dictionary to explicitly carry content or role before counting as media.

This keeps parameter maps like {image_parts: ..., user_id: ...} safely on the value path without discarding extra arguments.

Comment thread ddtrace/llmobs/_llmobs.py Outdated
)
cls._tag_text_io(
span,
input_value=input_data if media_input is None else None,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A failed media parse now drops the whole side, and the new decorator guard makes it silent.

On the failure path media_input is not None, so input_value is passed as None here and nothing is recorded for that side. _tag_llm_io also returns early on input failure, so a valid output side is never processed either.

Verified on an agent span, input_data={"image_parts": [{"url": "u"}], "user_id": 5} with output_data="the answer":

meta = {'input': {}, 'output': {'value': '"the answer"'}}

# media on both sides, input malformed:
meta = {'input': {}, 'output': {}}

The emitted event has no meta.input at all. Under a decorator _annotate_or_log now swallows the exception, so this becomes total silent loss of span I/O where the payload was previously recorded as a value string.

Fix: on parse failure, fall back to the value path for that side instead of dropping it, and let _tag_llm_io still process the output side when the input side failed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Input and output sides are now tagged independently, and any side failing media parsing safely falls back to the value path instead of being silently dropped.

Verified that malformed inputs no longer suppress valid outputs, and added tests covering both single-side and double-side fallbacks.

Comment thread ddtrace/llmobs/_utils.py
else:
meta[LLMOBS_STRUCT.INPUT].pop(LLMOBS_STRUCT.MESSAGES, None)
meta[LLMOBS_STRUCT.INPUT][LLMOBS_STRUCT.VALUE] = safe_json(input_messages, ensure_ascii=False) or ""
if input_value is not None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope this pop to non-LLM span kinds.

The pop is unconditional on span kind, which reverses the messages-wins precedence _build_llmobs_span has always applied for llm spans (lines 428-431).

The reachable case is an integration writing input_value/output_value at operation end after a user annotated media on the same span, for example _integrations/google_adk.py:100. The user's image_parts are dropped with no log:

after user annotate:            {"messages": [{"content": "hi", "image_parts": [...]}]}
after integration input_value:  {"value": "integration input"}

The same widening applies on llm spans (verified), though I found no in-tree caller that hits it today, so that half is latent rather than active.

Fix: gate the pop on annotated_span_kind, or move it into the non-llm branch. The sibling-key fix itself is correct and now works in both directions on both sides, which I verified across five annotate sequences. Only its scope is too broad.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Sibling key pops are now strictly gated on non-LLM span kinds so standard LLM spans retain their message-precedence rules.

Added test coverage ensuring user-annotated media on LLM spans survives subsequent integration value writes.

- |
LLM Observability: When a message carries inline media, the collapsed value string derived for
a non-LLM span duplicates the message text. If that duplication would take the span event past
``DD_LLMOBS_EVENT_SIZE_BYTES``, the value is replaced with ``[value omitted: event size limit]``

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two bullets in this note do not match the code.

This bullet promises the whole input and output are not dropped, but the guard in collapse_messages_to_value is per-side and has no visibility into the opposite side, metadata, tags, or the envelope. Recomputed against the real code at the 5,000,000 default:

scenario guard trips? event size writer truncates?
in: 400KB text + 4MiB image; out: 780KB text no 5,774,718 yes, media dropped
4MiB image on each side, tiny text no 8,389,142 yes, media dropped
in only: 4MiB image + 780KB text yes 4,974,726 no

Row 1 is the exact scenario from the previous review round. The promise holds only for the single-side, text-dominated case. Either budget across both sides before omitting either derived value (the codex bot suggested the same), or soften the bullet to match what the guard does.

The bullet above it scopes the dropped-key warning to the five non-LLM kinds, but the warning lives in Messages.__init__ and fires for every Messages(...) construction. Verified on a plain llm span with standard OpenAI messages: Dropping message keys ['tool_call_id'] and ['name', 'refusal']. Either gate the warning on span kind or drop the scoping from the note.

Also missing: annotate can now raise on these kinds when the media parts are all dicts but individually invalid. That is deliberate and tested, so it belongs in upgrade:.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed all three. Scoped the dropped-key warning to media-bearing messages, clarified that the event size guard operates per-side, and added upgrade notes for annotate error handling and argument logging.

Three follow-ups from review, each verified against the code before fixing.

A side whose media failed to parse was dropped rather than recorded. The two
sides are now tagged independently, so a malformed input cannot suppress a
valid output, and a side that fails falls back to the value path instead of
going unrecorded. Under a decorator the annotation error is logged rather than
raised, which made that loss silent.

Clearing the sibling key is now scoped to the non-LLM kinds. It was
unconditional, which reversed the messages-wins precedence LLM spans have
always applied and let an integration writing a value at operation end drop
media a user had annotated on the same span.

The dropped-key warning now fires only for a message that carries media, and
the whole diagnostic is wrapped. It was firing for every message construction,
including ordinary provider fields on LLM spans, and a logging statement should
not be able to fail an annotate call.

Release note corrected: the size guard checks one side in isolation, so say so
rather than promise more, and record that annotate can now raise on these kinds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants