Fix unhashable attributes crash in OpenAI streaming instrumentation #3436
Conversation
…raceloop#3428) This commit resolves a critical bug where OpenAI streaming instrumentation crashes with "TypeError: unhashable type: 'list'" when tool definitions contain lists or other unhashable types. **Problem:** - OpenAI tool definitions contain lists (e.g., "required": ["param1", "param2"]) - These unhashable values propagate to metric attributes via Config.get_common_metrics_attributes() - OpenTelemetry metrics require hashable attributes for aggregation keys - When creating frozenset(attributes.items()), TypeError occurs and crashes user code **Solution:** - Added _sanitize_attributes_for_metrics() function to convert unhashable values to JSON strings - Applied sanitization in all metric recording paths: - ChatStream._shared_attributes() (main streaming path) - _set_chat_metrics() (non-streaming path) - _build_from_streaming_response() (legacy OpenAI v0 compatibility) - _abuild_from_streaming_response() (async legacy compatibility) **Testing:** - Added comprehensive test suite covering edge cases - Verified fix preserves critical attributes while converting problematic values - All hashable values remain unchanged, unhashable values become JSON strings Fixes traceloop#3428
|
|
WalkthroughThis PR introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to a1c608d in 1 minute and 48 seconds. Click for details.
- Reviewed
271lines of code in2files - 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/opentelemetry/instrumentation/openai/shared/chat_wrappers.py:772
- Draft comment:
Good use of _sanitize_attributes_for_metrics in _shared_attributes. If nested unhashable structures can occur inside attribute values, consider whether a recursive sanitization might be needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/opentelemetry-instrumentation-openai/tests/test_unhashable_attributes_fix.py:17
- Draft comment:
The tests comprehensively cover various unhashable scenarios. Consider adding an extra test for nested unhashable types if such cases are expected in practice. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_rtLrmQxAbRhCvW5c
You can customize 
| for key, value in attributes.items(): | ||
| try: | ||
| # Test if the value is hashable by creating a frozenset | ||
| frozenset({key: value}.items()) |
There was a problem hiding this comment.
Consider replacing the frozenset({key: value}.items()) check with a direct call to hash(value) to test hashability. This is more idiomatic and may be clearer.
| frozenset({key: value}.items()) | |
| hash(value) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py (1)
234-244: Fix async exception metrics sanitization gap.
achat_wrapperstill mergesConfig.get_common_metrics_attributes()straight into the metric attributes during exception handling. If those common attributes carry lists/dicts (the original failure mode), the subsequentduration_histogram.record/exception_counter.addcalls will raise the sameTypeErrorand crash user code. Please sanitize here as well so the fix covers error paths.- common_attributes = Config.get_common_metrics_attributes() - attributes = { - **common_attributes, + common_attributes = _sanitize_attributes_for_metrics( + Config.get_common_metrics_attributes() + ) + attributes = { + **common_attributes, "error.type": e.__class__.__name__, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py(5 hunks)packages/opentelemetry-instrumentation-openai/tests/test_unhashable_attributes_fix.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/opentelemetry/instrumentation/openai/shared/chat_wrappers.pypackages/opentelemetry-instrumentation-openai/tests/test_unhashable_attributes_fix.py
🧬 Code graph analysis (2)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py (1)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/__init__.py (2)
metric_shared_attributes(364-377)_get_openai_base_url(275-281)
packages/opentelemetry-instrumentation-openai/tests/test_unhashable_attributes_fix.py (1)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py (3)
_sanitize_attributes_for_metrics(67-95)ChatStream(634-888)_shared_attributes(762-772)
🪛 Flake8 (7.3.0)
packages/opentelemetry-instrumentation-openai/tests/test_unhashable_attributes_fix.py
[error] 8-8: 'unittest.mock.patch' imported but unused
(F401)
| cause TypeError when streaming chat completions are used. | ||
| """ | ||
| import pytest | ||
| from unittest.mock import MagicMock, patch |
There was a problem hiding this comment.
Drop unused patch import.
Flake8 flags this (F401), so the test module won’t pass lint until patch is removed.
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from unittest.mock import MagicMock, patch | |
| from unittest.mock import MagicMock |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 8-8: 'unittest.mock.patch' imported but unused
(F401)
🤖 Prompt for AI Agents
In
packages/opentelemetry-instrumentation-openai/tests/test_unhashable_attributes_fix.py
around line 8, the import statement includes an unused `patch` (from
unittest.mock) which triggers flake8 F401; remove `patch` from the import so
only `MagicMock` is imported (or delete the entire unused import if not needed
elsewhere) to satisfy linting.
3 participants
Fixes #3428
Important
Fixes unhashable attributes crash in OpenAI streaming by sanitizing attributes for metric recording.
_sanitize_attributes_for_metrics()inchat_wrappers.pyto convert unhashable attributes to hashable types for metric recording._set_chat_metrics(),_shared_attributes(),_build_from_streaming_response(), and_abuild_from_streaming_response().test_unhashable_attributes_fix.pyadded to verify the fix.Nonevalues, and preserving hashable values.This description was created by
for a1c608d. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests