fix(openai-agents): ensure all content types are serialized consistently #3362
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix inconsistent content serialization in OpenTelemetry attributes. Previously only dict types were being serialized to JSON, but lists and other non-string types could still cause attribute type warnings. Now all non-string content is consistently serialized using json.dumps() before being set as span attributes. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughUpdated content serialization in on_span_end so non-string message content is JSON-serialized across ResponseSpanData, GenerationSpanData, and legacy paths; string content is preserved unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Hook as on_span_end
participant Msg as InputMessages
participant Ser as JSONSerializer
participant Span as SpanAttributes
Hook->>Msg: iterate messages (role, content)
alt content is string
Hook->>Span: set content as-is
else content is non-string
Hook->>Ser: json.dumps(content)
Ser-->>Hook: serialized string
Hook->>Span: set serialized content
end
Note over Hook,Span: Applied in ResponseSpanData, GenerationSpanData, and legacy paths
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to b804f3b in 1 minute and 6 seconds. Click for details.
- Reviewed
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.py:240
- Draft comment:
Changing the check toif not isinstance(content, str)ensures that any non-string content (including lists, numbers, etc.) is JSON serialized. This aligns with the fix goal of consistent serialization. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.py:372
- Draft comment:
Consistently applying theif not isinstance(content, str)check in both message branches ensures all non-string content is serialized, reducing type warnings. This uniform treatment is good, though consider if primitives should remain unsanitized if valid. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_8jRARb9HxkngEiF0
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.py (3)
244-249: Harden JSON serialization (bytes/datetime/numpy) and deduplicate via helperjson.dumps(content) can raise for bytes, datetimes, numpy types, or custom objects, which would drop all attributes for the span (even though dont_throw prevents crashes). Suggest a small helper to robustly serialize and cap size, then reuse it here.
Apply within this hunk:
- content = json.dumps(content) + content = _serialize_attr_value(content)Add once near the imports (outside this hunk):
# Helper for consistent, safe attribute serialization MAX_OTEL_ATTR_LEN = 16384 def _json_default(o): try: # Common non-JSON types from enum import Enum import datetime, decimal if isinstance(o, (datetime.datetime, datetime.date, datetime.time)): return o.isoformat() if isinstance(o, decimal.Decimal): return float(o) if isinstance(o, Enum): return o.value if isinstance(o.value, (str, int, float, bool)) else o.name except Exception: pass # Fallbacks try: from dataclasses import is_dataclass, asdict if is_dataclass(o): return asdict(o) except Exception: pass try: import numpy as np if isinstance(o, (np.ndarray, np.number)): return o.tolist() if hasattr(o, "tolist") else o.item() except Exception: pass return str(o) def _serialize_attr_value(value: Any) -> str: if isinstance(value, str): s = value elif isinstance(value, bytes): s = value.decode("utf-8", "replace") else: try: s = json.dumps(value, default=_json_default, ensure_ascii=False, sort_keys=True, separators=(",", ":")) except Exception: s = repr(value) # Cap very large attributes to protect exporters/backends return s if len(s) <= MAX_OTEL_ATTR_LEN else (s[:MAX_OTEL_ATTR_LEN] + "…")Happy to add unit tests for bytes, datetime, numpy, and deeply nested lists if useful.
375-377: Use the same safe serializer in the legacy pathMirror the robust handling here to avoid divergent behavior between semconv and legacy keys.
- if not isinstance(content, str): - content = json.dumps(content) + if not isinstance(content, str): + content = _serialize_attr_value(content)
383-385: Use the same safe serializer for dict-message legacy branchKeeps legacy serialization consistent and resilient.
- if not isinstance(content, str): - content = json.dumps(content) + if not isinstance(content, str): + content = _serialize_attr_value(content)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Lint
Fix inconsistent content serialization in OpenTelemetry attributes. The previous fix only handled dict types but missed lists and other non-string types, causing "Invalid type dict in attribute" warnings. Changes: - Updated all content serialization checks from isinstance(content, dict) to not isinstance(content, str) for comprehensive coverage - Added test_content_serialization_logic() to demonstrate the issue and verify the fix handles all non-string content types 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 91ed80d in 1 minute and 49 seconds. Click for details.
- Reviewed
44lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py:105
- Draft comment:
Avoid using print statements in tests (e.g. here and on line 109, 117) as they may clutter output. Consider using assertions or logging at an appropriate level. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The print statements here serve a specific purpose - they help demonstrate and document the bug being fixed. They show the before/after comparison clearly. The test already has proper assertions on lines 120 and 123. The prints are supplementary documentation, not the main test mechanism. Removing them would make the test less clear without adding value. Print statements in tests can be problematic if they clutter test output or are used instead of proper assertions. Maybe logging would be cleaner. While logging could be used, these prints serve a clear documentation purpose and are accompanied by proper assertions. The prints make the test's purpose more obvious to future readers. The print statements add value by documenting the bug fix clearly. They supplement rather than replace proper assertions. The comment should be deleted.
2. packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py:102
- Draft comment:
The simulated old logic (using 'if isinstance(content_old, dict)') intentionally misses lists. Consider adding a clarifying comment that this is a demonstration of the previous faulty behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py:112
- Draft comment:
New logic correctly serializes non-string types. Consider adding an explicit test case to ensure that content which is already a string remains unchanged. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment makes a valid point - the new logic handles non-strings by converting them to strings, but we don't verify that strings pass through unchanged. This could be a real edge case worth testing. However, the test's main purpose is to demonstrate the specific bug fix for list serialization, not to be a comprehensive test suite. The string case is also tested indirectly in test_dict_content_serialization which verifies string attributes. The comment identifies a real gap in test coverage. A string input could theoretically be modified by the new logic if there was a bug. While technically correct, this test's scope is specifically about fixing the list serialization bug. The string case is already covered by other tests. Delete the comment. While technically valid, it goes beyond the focused scope of this test which is demonstrating the list serialization bug fix. The string case is already tested elsewhere.
Workflow ID: wflow_2jjOejQ3rmhGHhFh
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py (2)
93-124: Make the test assertion-driven; drop prints; assert exact JSON; add S101 noqa.Removes noisy prints, strengthens checks by asserting equality to json.dumps(), and silences Ruff S101 in tests.
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 + + # 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}") - + expected_json = json.dumps(test_content) + # Test that old logic fails for lists (demonstrates the bug) - assert isinstance(content_old, list), "Old logic should leave lists unserialized (demonstrating the bug)" - + assert isinstance(content_old, list), "Old logic should leave lists unserialized (demonstrating the bug)" # noqa: S101 + # Test that new logic succeeds for all types - assert isinstance(content_new, str), "New logic should serialize all non-string content to JSON strings" + assert isinstance(content_new, str), "New logic should serialize all non-string content to JSON strings" # noqa: S101 + assert content_new == expected_json, "New logic should use json.dumps() result" # noqa: S101
93-124: Prefer an integration-style check over simulating “old vs new” logic.This unit test re-implements the branching logic instead of asserting the behavior of the instrumented spans. Consider parametrizing content types and asserting exporter span attributes are strings (and JSON for non-strings). Keep the existing dict test and add list/tuple/number/bool/None cases.
Example (place as a new test):
import json import pytest @pytest.mark.parametrize("content", [ {"a": 1}, [{"type": "text", "text": "hi"}], ("tuple", 1), 42, 3.14, True, None, ]) def test_non_string_content_is_serialized_on_spans(exporter, test_agent, content): # Arrange: send a message whose content is `content` messages = [{"role": "user", "content": content}] Runner.run_sync(test_agent, messages) # Assert: all prompt/content attributes are strings for span in exporter.get_finished_spans(): for k, v in span.attributes.items(): if "content" in k and ("prompt" in k or "gen_ai.prompt" in k): assert isinstance(v, str) # noqa: S101 if not isinstance(content, str): # Optional: verify it looks like JSON for non-strings try: json.loads(v) except Exception: pytest.fail(f"Content not JSON-serialized: {v!r}")Optional follow-up: add a case with non-JSON-serializable content (e.g., bytes, datetime) and ensure the hook handles it gracefully (fallback to str or safe-dumps).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py
120-120: Use of assert detected
(S101)
123-123: Use of assert detected
(S101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Lint
Comment on lines
+93
to
+124
| 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" | ||
|
|
There was a problem hiding this comment.
💡 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
doneLength of output: 10036
Suppress S101 for asserts in tests
- Ruff flags S101 for bare
assertusage in packages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.py (Lines 120, 123). Add# noqa: S101to those lines or configure a per-file‐ignore inruff.tomlunder[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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
• Fixes inconsistent content serialization in OpenTelemetry attributes that was causing type warnings
• Previously only dict types were serialized to JSON, but lists and other non-string types could still cause "Invalid type dict in attribute" warnings
• Now all non-string content is consistently serialized using json.dumps() before being set as span attributes
Test plan
test_dict_content_serializationpasses🤖 Generated with Claude Code
Important
Fixes content serialization in OpenTelemetry attributes by JSON-serializing all non-string types in
_hooks.py.on_span_end()in_hooks.pyby serializing all non-string types to JSON usingjson.dumps().test_content_serialization_logic()intest_openai_agents.pyto verify new serialization logic.test_dict_content_serialization()passes without warnings.test_openai_agents.pypass, ensuring no regression.This description was created by
for 91ed80d. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests