packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.py (1)

264-273: Defensive iteration over choices to avoid NoneType errors

If an SDK chunk lacks 'choices', iterating over None raises. Guard with default empty list.

-    for choice in item.get("choices"):
+    for choice in (item.get("choices") or []):

packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py (4)

642-645: Async context manager does not trigger cleanup; spans may leak when using async with

aexit does not call _ensure_cleanup(); spans may remain open and metrics unfinalized for async context usage.

-    async def __aexit__(self, exc_type, exc_val, exc_tb):
-        await self.__wrapped__.__aexit__(exc_type, exc_val, exc_tb)
+    async def __aexit__(self, exc_type, exc_val, exc_tb):
+        cleanup_exception = None
+        try:
+            self._ensure_cleanup()
+        except Exception as e:
+            cleanup_exception = e
+            logger.debug("Error during ChatStream cleanup in __aexit__: %s", cleanup_exception)
+        await self.__wrapped__.__aexit__(exc_type, exc_val, exc_tb)

---

707-752: Keep span current during streaming finalization to ensure events carry span_id

emit_event in _process_complete_response runs after the outer with trace.use_span has exited. Wrap the body in a use_span context.

     def _process_complete_response(self):
-        _set_streaming_token_metrics(
+        with trace.use_span(self._span, end_on_exit=False):
+            _set_streaming_token_metrics(
                 self._request_kwargs,
                 self._complete_response,
                 self._span,
                 self._token_counter,
                 self._shared_attributes(),
             )
             # choice metrics
             if self._choice_counter and self._complete_response.get("choices"):
                 _set_choice_counter_metrics(
                     self._choice_counter,
                     self._complete_response.get("choices"),
                     self._shared_attributes(),
                 )
             # duration metrics
             if self._start_time and isinstance(self._start_time, (float, int)):
                 duration = time.time() - self._start_time
             else:
                 duration = None
             if duration and isinstance(duration, (float, int)) and self._duration_histogram:
                 self._duration_histogram.record(
                     duration, attributes=self._shared_attributes()
                 )
             if self._streaming_time_to_generate and self._time_of_first_token:
                 self._streaming_time_to_generate.record(
                     time.time() - self._time_of_first_token,
                     attributes=self._shared_attributes(),
                 )
             _set_response_attributes(self._span, self._complete_response)
             if should_emit_events():
                 for choice in self._complete_response.get("choices", []):
                     emit_event(_parse_choice_event(choice))
             else:
                 if should_send_prompts():
                     _set_completions(
                         self._span, self._complete_response.get("choices"))
             self._span.set_status(Status(StatusCode.OK))
             self._span.end()
             self._cleanup_completed = True

---

828-896: Wrap streaming builders in trace.use_span
The streaming builder functions in chat_wrappers.py emit events and record metrics outside of the span’s active context. Enclose their entire processing—from initializing complete_response through span.end()—in a with trace.use_span(span, end_on_exit=False): block to ensure that all child events and metrics are correctly attributed.

• File: packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py

• Lines 828–896: _build_from_streaming_response
• Lines 899–967: _abuild_from_streaming_response

Example diff for _build_from_streaming_response:

 def _build_from_streaming_response(
     span,
     response,
     instance=None,
     token_counter=None,
     choice_counter=None,
     duration_histogram=None,
     streaming_time_to_first_token=None,
     streaming_time_to_generate=None,
     start_time=None,
     request_kwargs=None,
 ):
-    complete_response = {"choices": [], "model": "", "id": ""}
+    with trace.use_span(span, end_on_exit=False):
+        complete_response = {"choices": [], "model": "", "id": ""}
         first_token = True
         time_of_first_token = start_time  # will be updated when first token is received

         for item in response:
@@
-    _set_response_attributes(span, complete_response)
+        _set_response_attributes(span, complete_response)
@@
-    span.set_status(Status(StatusCode.OK))
-    span.end()
+        span.set_status(Status(StatusCode.OK))
+        span.end()

Apply the same pattern to _abuild_from_streaming_response. Ensure you have from opentelemetry import trace imported at the top of the file.

---

651-663: Enhance exception recording in chat stream wrappers
Ensure that on streaming errors (non-StopIteration/StopAsyncIteration), the span records both the exception and its type before setting the error status.

Locations to update:

• packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py, in both __next__ (lines ~651–663) and __anext__ (lines ~667–681).

Suggested diff:

--- a/packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py
+++ b/packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py
@@ def __next__(self):
-                self._ensure_cleanup()
-                if self._span and self._span.is_recording():
-                    self._span.set_status(Status(StatusCode.ERROR, str(e)))
+                self._ensure_cleanup()
+                if self._span and self._span.is_recording():
+                    # Record exception type and details for better diagnostics
+                    self._span.set_attribute(ERROR_TYPE, e.__class__.__name__)
+                    self._span.record_exception(e)
+                    self._span.set_status(Status(StatusCode.ERROR, str(e)))
@@ async def __anext__(self):
-                self._ensure_cleanup()
-                if self._span and self._span.is_recording():
-                    self._span.set_status(Status(StatusCode.ERROR, str(e)))
+                self._ensure_cleanup()
+                if self._span and self._span.is_recording():
+                    # Record exception type and details for better diagnostics
+                    self._span.set_attribute(ERROR_TYPE, e.__class__.__name__)
+                    self._span.record_exception(e)
+                    self._span.set_status(Status(StatusCode.ERROR, str(e)))

These changes align with other OpenAI wrappers (e.g., completion_wrappers.py) and leverage the existing ERROR_TYPE import.

packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.py (2)

76-79: Consider setting OK status for non-streaming path before ending

For consistency with streaming path, you may want to set span.set_status(Status(StatusCode.OK)) before span.end() when not streaming.

-    _handle_response(response, span, instance)
-    span.end()
+    _handle_response(response, span, instance)
+    span.set_status(Status(StatusCode.OK))
+    span.end()

Also applies to: 112-116

---

132-139: Optional: include role in prompt events for consistency

Completion prompts are emitted without a role. If helpful for consumers, emit with a "user" role for prompt strings.

-            emit_event(MessageEvent(content=p))
+            emit_event(MessageEvent(content=p, role="user"))
...
-        emit_event(MessageEvent(content=prompt))
+        emit_event(MessageEvent(content=prompt, role="user"))

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/assistant_wrappers.py (2)

195-196: Brittle content access needs error handling

The code assumes content[0].get("text").get("value") structure exists, which can cause KeyError if the first item isn't "text" type or if content is empty.

The previous review comment about brittle access to message content still applies. Consider adding safe access pattern:

-            message_content = content[0].get("text").get("value")
+            message_content = None
+            if isinstance(content, list) and content:
+                first = content[0]
+                if isinstance(first, dict) and isinstance(first.get("text"), dict):
+                    message_content = first["text"].get("value")
+            if message_content is None:
+                message_content = str(content)

---

313-316: EventHandleWrapper assignment needs safety check

The code unconditionally assigns kwargs["event_handler"] which will fail if the key doesn't exist.

The previous review comment about guarding the event_handler wrapping still applies:

-        kwargs["event_handler"] = EventHandleWrapper(
-            original_handler=kwargs["event_handler"],
-            span=span,
-        )
+        if kwargs.get("event_handler"):
+            kwargs["event_handler"] = EventHandleWrapper(
+                original_handler=kwargs["event_handler"],
+                span=span,
+            )

packages/opentelemetry-instrumentation-openai/tests/traces/test_span_context_propagation.py (2)

4-4: Remove unused import

The MagicMock import is not used anywhere in the test file.

-from unittest.mock import MagicMock

---

35-41: Use contextlib.suppress for cleaner exception handling

The try-except-pass block can be simplified using contextlib.suppress for better readability.

+import contextlib
+
-    try:
-        mock_openai_client.chat.completions.create(
-            model="gpt-3.5-turbo",
-            messages=[{"role": "user", "content": "Test span context"}],
-        )
-    except Exception:
-        pass
+    with contextlib.suppress(Exception):
+        mock_openai_client.chat.completions.create(
+            model="gpt-3.5-turbo",
+            messages=[{"role": "user", "content": "Test span context"}],
+        )

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/assistant_wrappers.py (3)

195-210: Brittle content access pattern needs defensive handling.

The code assumes content[0].get("text").get("value") exists without validation. This can raise KeyError or AttributeError when the content structure differs from expectations.

This issue was identified in previous reviews and needs to be addressed with defensive handling for non-text content or empty arrays.

---

313-316: Guard against missing event_handler to prevent KeyError.

The code unconditionally wraps kwargs["event_handler"] without checking if the key exists, which will raise a KeyError when no event handler is provided.

This issue was identified in previous reviews and needs conditional wrapping only when the event_handler is present.

---

185-188: Ensure mutual exclusivity between event emission and span attributes for run instructions

The current logic always calls _set_span_attribute(...content, run["instructions"]) and then conditionally emits an event. It should mirror the pattern used elsewhere—emit the event or set both role and content attributes, not both.

Locations to update:

• File packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/assistant_wrappers.py
• Around lines 181–188

Suggested diff:

-        _set_span_attribute(
-            span,
-            f"{SpanAttributes.LLM_PROMPTS}.{prompt_index}.content",
-            run["instructions"],
-        )
-        if should_emit_events():
-            emit_event(MessageEvent(content=run["instructions"], role="system"))
-        prompt_index += 1
+        if should_emit_events():
+            emit_event(MessageEvent(content=run["instructions"], role="system"))
+        else:
+            _set_span_attribute(
+                span,
+                f"{SpanAttributes.LLM_PROMPTS}.{prompt_index}.role",
+                "system"
+            )
+            _set_span_attribute(
+                span,
+                f"{SpanAttributes.LLM_PROMPTS}.{prompt_index}.content",
+                run["instructions"],
+            )
+        prompt_index += 1

packages/opentelemetry-instrumentation-openai/tests/traces/test_span_context_propagation.py (1)

34-40: Consider using contextlib.suppress for cleaner exception handling.

The static analysis tool correctly identifies that the try-except-pass pattern can be simplified using contextlib.suppress(Exception) for better readability.

Apply this diff to simplify the exception handling:

+from contextlib import suppress
+
    # The mock_openai_client fixture should trigger instrumentation but not make real calls
-    try:
+    with suppress(Exception):
         mock_openai_client.chat.completions.create(
             model="gpt-3.5-turbo",
             messages=[{"role": "user", "content": "Test span context"}],
         )
-    except Exception:
-        pass

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro