openllmetry icon indicating copy to clipboard operation
openllmetry copied to clipboard

Fix unhashable attributes crash in OpenAI streaming instrumentation

Open zwanzigg opened this issue 2 months ago • 2 comments

Fixes #3428


[!IMPORTANT] Fixes unhashable attributes crash in OpenAI streaming by sanitizing attributes for metric recording.

  • Behavior:
    • Adds _sanitize_attributes_for_metrics() in chat_wrappers.py to convert unhashable attributes to hashable types for metric recording.
    • Applies sanitization in _set_chat_metrics(), _shared_attributes(), _build_from_streaming_response(), and _abuild_from_streaming_response().
  • Tests:
    • New test file test_unhashable_attributes_fix.py added to verify the fix.
    • Tests include handling of lists, dicts, None values, and preserving hashable values.
    • Simulates original issue conditions to ensure the fix prevents crashes.

This description was created by Ellipsis for a1c608d3ae122df955fa9a4c47254ca01e24c3c2. You can customize this summary. It will automatically update as commits are pushed.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where OpenTelemetry metrics collection could fail when encountering certain attribute value types in OpenAI instrumentation.
  • Tests

    • Added comprehensive test coverage for metrics attribute handling with various data types and edge cases.

zwanzigg avatar Nov 09 '25 18:11 zwanzigg

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Nov 09 '25 18:11 CLAassistant

Walkthrough

This PR introduces a _sanitize_attributes_for_metrics() helper function to convert non-hashable metric attribute values (lists, dicts) to JSON strings or string representations, ensuring OpenTelemetry metric compatibility. The function is integrated into five metric recording locations across the ChatStream class and related functions.

Changes

Cohort / File(s) Summary
Metric attribute sanitization
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py
Added _sanitize_attributes_for_metrics() helper to convert non-hashable types (lists, dicts) to JSON strings. Applied sanitization in _set_chat_metrics(), ChatStream._shared_attributes(), ChatStream._process_complete_response(), and streaming response builders before metric recording.
Test suite for sanitization fix
packages/opentelemetry-instrumentation-openai/tests/test_unhashable_attributes_fix.py
New test module validating sanitization behavior across edge cases: lists, dicts, None/empty values, and pre-existing hashable values. Includes reproduction of original TypeError crash and verification that sanitized attributes remain hashable.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Sanitization logic correctness: Verify JSON serialization handles edge cases and fallback string conversion is appropriate
  • Integration points: Confirm all five metric recording locations are covered and sanitization is applied before OpenTelemetry record() calls
  • Test coverage: Validate test scenarios comprehensively cover unhashable types (lists, dicts) and ensure mocking of Config.get_common_metrics_attributes() accurately reproduces the production crash
  • Streaming path: Pay special attention to _process_item() and streaming builders where the crash originally occurred

Possibly related PRs

  • PR #3155: Modifies chat_wrappers.py and ChatStream methods including _process_complete_response() and streaming metric emission—direct overlap in implementation areas.

Poem

🐰 Lists and dicts caused quite a fright,
Making metrics crash in streaming's night,
But JSON strings save the day,
Now hashable hops keep crashes at bay! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix unhashable attributes crash in OpenAI streaming instrumentation' directly and accurately summarizes the main change: fixing a critical bug where unhashable attribute values (lists/dicts) crash metric recording in OpenAI streaming instrumentation.
Linked Issues check ✅ Passed The PR successfully addresses all key coding requirements from issue #3428: adds _sanitize_attributes_for_metrics helper to convert unhashable values to hashable forms, integrates sanitization into metric recording paths (_set_chat_metrics, ChatStream._shared_attributes, _process_complete_response, streaming builders), and includes comprehensive tests validating the fix with various attribute types.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the unhashable attributes issue: the new sanitization function, its integration points for metric recording, and the corresponding test module. No unrelated modifications to other functionality or files are present.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 09 '25 18:11 coderabbitai[bot]