Skip to content

fix(openai-agents): ensure all content types are serialized consistently #3362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nirga wants to merge 2 commits into from
Closed

fix(openai-agents): ensure all content types are serialized consistently #3362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b804f3b
fix(openai-agents): ensure all content types are serialized consistently
nirga Sep 3, 2025
91ed80d
fix(openai-agents): ensure all content types are serialized consistently
nirga Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...metry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ def on_span_end(self, span):
if 'role' in message and 'content' in message:
otel_span.set_attribute(f"{SpanAttributes.LLM_PROMPTS}.{i}.role", message['role'])
content = message['content']
if isinstance(content, dict):
if not isinstance(content, str):
content = json.dumps(content)
otel_span.set_attribute(f"{SpanAttributes.LLM_PROMPTS}.{i}.content", content)

Expand Down Expand Up @@ -372,14 +372,14 @@ def on_span_end(self, span):
if hasattr(message, 'role') and hasattr(message, 'content'):
otel_span.set_attribute(f"gen_ai.prompt.{i}.role", message.role)
content = message.content
if isinstance(content, dict):
if not isinstance(content, str):
content = json.dumps(content)
otel_span.set_attribute(f"gen_ai.prompt.{i}.content", content)
elif isinstance(message, dict):
if 'role' in message and 'content' in message:
otel_span.set_attribute(f"gen_ai.prompt.{i}.role", message['role'])
content = message['content']
if isinstance(content, dict):
if not isinstance(content, str):
content = json.dumps(content)
otel_span.set_attribute(f"gen_ai.prompt.{i}.content", content)

Expand Down
33 changes: 33 additions & 0 deletions packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,39 @@ def test_dict_content_serialization(exporter):
# The test passes if no dict type warnings occurred (all content attributes are strings)


def test_content_serialization_logic():
"""Test that demonstrates the difference between old and new content serialization logic."""
import json

# Test content with list of dictionaries (multimodal format)
test_content = [{"type": "text", "text": "Hello"}, {"type": "image", "url": "test.jpg"}]

# OLD LOGIC (current broken implementation): only checks for dict, not list
content_old = test_content
if isinstance(content_old, dict): # This misses lists!
content_old = json.dumps(content_old)

print(f"Old logic result: {type(content_old)} = {content_old}")

# This should fail - the old logic leaves lists unserialized
if isinstance(content_old, list):
print("❌ OLD LOGIC PROBLEM: List content not serialized!")
# This would cause OpenTelemetry warnings in production

# NEW LOGIC (our fix): checks for any non-string type
content_new = test_content
if not isinstance(content_new, str): # This catches all non-strings!
content_new = json.dumps(content_new)

print(f"New logic result: {type(content_new)} = {content_new}")

# Test that old logic fails for lists (demonstrates the bug)
assert isinstance(content_old, list), "Old logic should leave lists unserialized (demonstrating the bug)"

# Test that new logic succeeds for all types
assert isinstance(content_new, str), "New logic should serialize all non-string content to JSON strings"

Comment on lines +93 to +124
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Sep 3, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Ruff S101 in tests — either ignore per-file or add noqa tags.

Ruff flags assert usage (Lines 120, 123). Tests commonly use asserts; either configure per-file ignores for tests or keep the inline # noqa: S101 as in the diff.

Run to check repo config:

🏁 Script executed:

#!/bin/bash
# Inspect Ruff/Flake8 configs for S101 handling in tests
fd -H --max-depth 3 'pyproject.toml|ruff.toml|setup.cfg|tox.ini' \
  | while read -r f; do
      echo ">>> $f"
      rg -nC2 -e 'tool\.ruff|ruff\]|flake8\]|per-file-ignores|ignore\s*=|\bS101\b' "$f" || true
    done

Length of output: 10036

Suppress S101 for asserts in tests

  • Ruff flags S101 for bare assert usage in packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py (Lines 120, 123). Add # noqa: S101 to those lines or configure a per-file‐ignore in ruff.toml under [lint.per-file-ignores] (e.g. "**/tests/*.py" = ["S101"]).
🧰 Tools
🪛 Ruff (0.12.2)

120-120: Use of assert detected

(S101)

123-123: Use of assert detected

(S101)

🤖 Prompt for AI Agents 
In
packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py
around lines 93 to 124, ruff flags S101 for bare assert usage on the test
assertions at lines ~120 and ~123; fix by adding inline noqa comments to those
assert lines (append "  # noqa: S101") or alternately add a per-file ignore in
ruff.toml under [lint.per-file-ignores] (for example: `"**/tests/*.py" =
["S101"]`) so tests keep bare asserts without linter errors.


@pytest.mark.vcr
def test_agent_spans(exporter, test_agent):
query = "What is AI?"
Expand Down
Loading