Skip to content

Conversation

@alexey-pelykh
Copy link
Contributor

@alexey-pelykh alexey-pelykh commented Nov 26, 2025

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

Important

Fix handling of OpenAI sentinel values NOT_GIVEN and Omit in responses_wrappers.py to prevent errors, with tests added in test_responses.py.

  • Behavior:
    • Introduces _get_openai_sentinel_types() in responses_wrappers.py to dynamically discover OpenAI sentinel types NOT_GIVEN and Omit.
    • Adds _sanitize_sentinel_values() to remove sentinel values from kwargs to prevent errors when accessing attributes.
    • Updates responses_get_or_create_wrapper() and async_responses_get_or_create_wrapper() to use sanitized kwargs.
  • Tests:
    • Adds test_response_stream_init_with_not_given_reasoning() and test_response_stream_init_with_omit_reasoning() in test_responses.py to verify ResponseStream initialization with sentinel values.
    • Ensures no AttributeError is raised when reasoning is set to NOT_GIVEN or Omit.

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

Summary by CodeRabbit

  • Bug Fixes

    • Request and response processing now sanitize special sentinel values to avoid sentinel-related errors when building traced attributes, creating spans, and initializing streamed responses across synchronous and asynchronous flows.
  • Tests

    • Added tests to verify streamed responses initialize correctly when sentinel values are present in request data, preventing AttributeError and ensuring traced request fields remain absent when appropriate.

✏️ Tip: You can customize this high-level summary in your review settings.

@CLAassistant
Copy link

CLAassistant commented Nov 26, 2025

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link

coderabbitai bot commented Nov 26, 2025

Walkthrough

Adds runtime detection of OpenAI sentinel types and strips those sentinel values from request kwargs before building spans, traced data, ResponseStream state, and invoking wrapped OpenAI functions. Tests cover ResponseStream initialization with sentinel values to prevent AttributeError.

Changes

Cohort / File(s) Summary
Sentinel Sanitization Core
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py
Added _get_openai_sentinel_types() and _OPENAI_SENTINEL_TYPES for dynamic sentinel discovery, _sanitize_sentinel_values(kwargs) to remove sentinel values, SPAN_NAME constant, and integrated sanitized kwargs across sync/async wrappers, TracedData/span population, ResponseStream construction, and response/error paths.
Sentinel Handling Tests
packages/opentelemetry-instrumentation-openai/tests/traces/test_responses.py
Added tests that instantiate ResponseStream with NOT_GIVEN and Omit sentinels (when available) to ensure no AttributeError and that request_reasoning_summary is None.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Instrumented caller
    participant Sanitizer as _sanitize_sentinel_values()
    participant Wrapper as Response wrapper
    participant Tracer as Tracer/Span
    participant Target as OpenAI API

    Caller->>Wrapper: call wrapper with kwargs (may include sentinels)
    Wrapper->>Sanitizer: sanitize kwargs for internal use
    Sanitizer-->>Wrapper: sanitized_kwargs (sentinels removed)
    Wrapper->>Tracer: create span (SPAN_NAME) & populate TracedData using sanitized_kwargs
    Wrapper->>Target: invoke underlying OpenAI function with sanitized_kwargs
    alt streaming response
        Wrapper->>Wrapper: ResponseStream.__init__ filters request_kwargs (defensive)
        Wrapper->>Tracer: update traced data from sanitized stream state
    end
    Wrapper-->>Caller: return traced result / stream
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review focus:
    • Correct discovery/caching of _OPENAI_SENTINEL_TYPES
    • Consistent use of sanitized kwargs across sync/async, success/error, and streaming paths
    • ResponseStream defensive sanitization and test coverage

Possibly related PRs

Suggested reviewers

  • nirga

Poem

🐰
I hop through kwargs, soft and keen,
Finding sentinels that hide unseen.
I pluck them out with gentle paws,
So spans and streams obey their laws.
A tidy trace — that's my applause!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(openai): recognize NOT_GIVEN and Omit' directly and concisely summarizes the main change: handling OpenAI sentinel values to fix attribute access errors.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 737649b and 6d72041.

📒 Files selected for processing (2)
  • packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py (23 hunks)
  • packages/opentelemetry-instrumentation-openai/tests/traces/test_responses.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/tests/traces/test_responses.py
  • packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py
🔇 Additional comments (9)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py (7)

91-106: LGTM - Clean implementation of sentinel value filtering.

The function correctly handles backward compatibility (empty tuple case) and efficiently filters sentinel values using a dict comprehension. The top-level filtering is sufficient since it removes sentinels before the chained .get() calls can fail.


496-513: LGTM - Correct placement of sanitization.

The sanitization is applied early at the wrapper entry point, and non_sentinel_kwargs is used consistently for all telemetry operations while the original kwargs is correctly passed to the wrapped function. This preserves the API contract.


648-665: LGTM - Consistent async implementation.

The async wrapper mirrors the sync implementation correctly, applying the same sanitization pattern.


794-808: LGTM - Cancel wrapper follows the same pattern.


823-837: LGTM - Async cancel wrapper consistent with sync version.


865-888: LGTM - Defensive sanitization in constructor.

The defensive sanitization ensures ResponseStream works correctly even if instantiated directly without prior filtering. The operation is idempotent, so double-sanitization from the wrapper is harmless.


35-52: I'll help you verify the import path for OpenAI sentinel types. Let me start by examining the repository structure and checking the OpenAI SDK module exports.
<function_calls>

#!/bin/bash

First, find the repository structure and check for requirements files

find . -type f -name ".txt" -o -name ".py" -o -name ".toml" -o -name ".cfg" | grep -E "(requirements|setup|pyproject|package)" | head -20


</function_calls>

I'll search for dependency information to understand which OpenAI SDK versions are supported:
<function_calls>

#!/bin/bash

Find the package definition for the openai instrumentation

fd -t f "pyproject.toml|setup.py|setup.cfg" packages/opentelemetry-instrumentation-openai/ 2>/dev/null


</function_calls>

Let me also check the actual test files to understand the usage:
<function_calls>

#!/bin/bash

Look for test files related to responses_wrappers

fd -t f ".*test.responses..py$" packages/opentelemetry-instrumentation-openai/


</function_calls>

Now, let me check the actual file mentioned in the review to understand the full context:
<function_calls>

#!/bin/bash

Get line count of the responses_wrappers.py file

wc -l packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py


</function_calls>

<function_calls>

#!/bin/bash

Read the beginning of the file to see imports and the function in question

head -60 packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py


</function_calls>

Let me search for any references to the sentinel types in the test files:
<function_calls>

#!/bin/bash

Search for NotGiven and Omit imports in the codebase

rg -n "NotGiven|Omit" packages/opentelemetry-instrumentation-openai/ -A 2 -B 2


</function_calls>

<function_calls>

#!/bin/bash

Check pyproject.toml for OpenAI dependency constraints

cat packages/opentelemetry-instrumentation-openai/pyproject.toml | grep -A 10 -B 2 "openai"


</function_calls>

Now let me verify with web search what the public API exports in different OpenAI SDK versions:
<function_calls>

OpenAI Python SDK NotGiven Omit public API export v1

</function_calls>

<function_calls>

OpenAI Python SDK _types NotGiven Omit internal vs public

</function_calls>

Let me also check the GitHub repository for the OpenAI Python SDK:
<function_calls>

site:github.com openai/openai-python NotGiven Omit init export

</function_calls>

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

510-555: Good test coverage for NOT_GIVEN sentinel handling.

The test correctly verifies the fix for issue #3472. One minor observation: the test imports from openai._types (private) while the wrapper imports from openai (public). Consider aligning these if possible, though using the private path in tests may be intentional to access the sentinel value directly.


558-596: Good test coverage for Omit sentinel handling.

The test correctly covers the second sentinel type variant. The parallel structure with the NOT_GIVEN test maintains consistency.


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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 68952b3 in 2 minutes and 29 seconds. Click for details.
  • Reviewed 440 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 draft 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/v1/responses_wrappers.py:11
  • Draft comment:
    Good implementation of _get_openai_sentinel_types. Consider adding a note that the sentinel imported from 'openai' (NotGiven) may differ from those exposed in 'openai._types' (e.g. NOT_GIVEN) and ensure they’re equivalent.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is asking the author to "consider" and "ensure" something about sentinel types from different import paths. This is speculative - it's suggesting there might be a difference between openai.NotGiven and openai._types.NOT_GIVEN, but provides no evidence that this is actually a problem. The code is importing from the public API (from openai import NotGiven), which is the correct approach. The comment doesn't point to any actual bug or issue in the code - it's just raising a hypothetical concern. According to the rules, comments that ask the author to "ensure" things or are speculative ("If X, then Y") should be removed. This comment doesn't provide a clear, actionable code change. The comment might be raising a legitimate concern about API consistency in the OpenAI SDK. If there are indeed different sentinel objects in different modules, this could be a real issue. However, without evidence that this is actually a problem, it's just speculation. While the concern might be theoretically valid, the comment provides no evidence that this is an actual issue. The code is using the public API imports, which is the standard practice. If there were a real problem with sentinel type equivalence, the comment should demonstrate it with specific evidence rather than asking the author to investigate. This is exactly the type of "ensure that..." comment that the rules say to remove. This comment should be deleted. It's speculative, asks the author to "ensure" something without providing evidence of an actual problem, and doesn't suggest a specific code change. The code is using standard public API imports, which is correct.
2. packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py:93
  • Draft comment:
    The _sanitize_sentinel_values function clearly removes top-level sentinels. Consider mentioning that it only sanitizes first-level keys (i.e. nested sentinel values are not handled) in the docstring, if relevant.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a documentation suggestion, not a code change request. Looking at the usage patterns in the diff, the function is used to sanitize top-level kwargs, and then code accesses nested values like safe_kwargs.get("reasoning", {}).get("summary"). The issue is: if kwargs["reasoning"] is a sentinel value, it gets removed (good). But if kwargs["reasoning"] is a dict containing sentinel values as nested values, those nested sentinels won't be removed. However, looking at the actual usage, the pattern is safe_kwargs.get("reasoning", {}) which returns an empty dict if "reasoning" is missing, then .get("summary") on that dict. So the real concern is whether "reasoning" itself could be a sentinel (handled) vs whether values inside the "reasoning" dict could be sentinels (not handled). The comment is technically correct that nested values aren't sanitized, but it's unclear if this is actually a problem in practice or if it matters. This feels like a "nice to have" documentation improvement rather than identifying an actual issue. According to the rules, I should only keep comments that require a clear code change, not purely informative comments. The comment is purely informative and doesn't request a code change. It's a documentation suggestion about clarifying existing behavior. Even if technically accurate, this violates the rule "Do NOT comment unless there is clearly a code change required. Do not make purely informative comments." While the observation about nested values is technically correct, this is a documentation-only suggestion with no code change requested. The rules explicitly state not to make purely informative comments. Additionally, there's no evidence in the diff that nested sentinel values are actually a problem - the usage patterns suggest only top-level sanitization is needed. This comment should be deleted because it's purely informative and doesn't request a code change. It's a documentation suggestion about clarifying behavior, which violates the rule against making comments that don't require code changes.
3. packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py:499
  • Draft comment:
    Nice use of safe_kwargs to prevent chained .get() calls from failing. Ensure that all references to mutable request parameters consistently use safe_kwargs to avoid similar issues.
  • Reason this comment was not posted:
    Comment was on unchanged code.
4. packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py:864
  • Draft comment:
    Good defensive programming in ResponseStream.init by normalizing request_kwargs. This ensures that even direct calls receive sanitized kwargs.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the existing code without offering any specific guidance or questions.
5. packages/opentelemetry-instrumentation-openai/tests/traces/test_responses.py:510
  • Draft comment:
    The tests for handling NOT_GIVEN and Omit sentinels are well written. Double-check that importing NOT_GIVEN or Omit from 'openai._types' is compatible with the sentinel types discovered via 'from openai import NotGiven/Omit'.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to double-check compatibility, which violates the rule against asking for confirmation or double-checking. It doesn't provide a specific suggestion or point out a clear issue.

Workflow ID: wflow_XtZq9Z137sWXhAve

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
packages/opentelemetry-instrumentation-openai/tests/traces/test_responses.py (2)

510-550: Consider adding skip decorator for SDK compatibility.

The test imports NOT_GIVEN from openai._types, which is a private module. If an older OpenAI SDK version doesn't have this type, the test will fail at import time rather than being skipped gracefully.

Consider adding a skip decorator similar to the reasoning tests above:

+@pytest.mark.skipif(
+    not hasattr(__import__('openai._types', fromlist=['NOT_GIVEN']), 'NOT_GIVEN'),
+    reason="NOT_GIVEN sentinel not available in this OpenAI SDK version",
+)
 def test_response_stream_init_with_not_given_reasoning():

Alternatively, move the import inside the test with a try/except and use pytest.skip():

def test_response_stream_init_with_not_given_reasoning():
    try:
        from openai._types import NOT_GIVEN
    except ImportError:
        pytest.skip("NOT_GIVEN sentinel not available in this OpenAI SDK version")
    # ... rest of test

553-586: Same SDK compatibility concern for Omit sentinel.

Similar to the above test, consider handling the case where Omit may not be available in older OpenAI SDK versions.

Apply the same pattern:

def test_response_stream_init_with_omit_reasoning():
    try:
        from openai._types import Omit
    except ImportError:
        pytest.skip("Omit sentinel not available in this OpenAI SDK version")
    # ... rest of test
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py (1)

806-811: Consider sanitizing kwargs in cancel wrappers for consistency.

The cancel wrappers (responses_cancel_wrapper and async_responses_cancel_wrapper) don't sanitize kwargs before passing to _set_request_attributes. While cancel operations typically have simpler kwargs, sanitizing for consistency would prevent potential future issues.

 @dont_throw
 @_with_tracer_wrapper
 def responses_cancel_wrapper(tracer: Tracer, wrapped, instance, args, kwargs):
     if context_api.get_value(_SUPPRESS_INSTRUMENTATION_KEY):
         return wrapped(*args, **kwargs)
+    
+    safe_kwargs = _sanitize_sentinel_values(kwargs)

     response = wrapped(*args, **kwargs)
     if isinstance(response, Stream):
         return response
     parsed_response = parse_response(response)
     existing_data = responses.pop(parsed_response.id, None)
     if existing_data is not None:
         span = tracer.start_span(
             SPAN_NAME,
             kind=SpanKind.CLIENT,
             start_time=existing_data.start_time,
             record_exception=True,
         )
-        _set_request_attributes(span, prepare_kwargs_for_shared_attributes(kwargs), instance)
+        _set_request_attributes(span, prepare_kwargs_for_shared_attributes(safe_kwargs), instance)
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 5bd93d9 and e056d13.

📒 Files selected for processing (2)
  • packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py (18 hunks)
  • packages/opentelemetry-instrumentation-openai/tests/traces/test_responses.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/tests/traces/test_responses.py
  • packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py
🧬 Code graph analysis (2)
packages/opentelemetry-instrumentation-openai/tests/traces/test_responses.py (1)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py (1)
  • ResponseStream (842-1071)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py (1)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/__init__.py (1)
  • _set_request_attributes (105-190)
🔇 Additional comments (5)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py (5)

11-28: LGTM - Robust sentinel type discovery.

The dynamic discovery pattern with try/except handles SDK version differences gracefully. The function correctly builds a tuple for efficient isinstance() checks.


93-108: LGTM - Clean sentinel sanitization implementation.

The function correctly handles the edge case when no sentinel types are available (older SDK) and uses an efficient dict comprehension for filtering.


498-500: LGTM - Correct placement of sanitization.

The sanitization is applied early, and the original kwargs is still passed to wrapped(*args, **kwargs) ensuring the OpenAI SDK receives its expected arguments while telemetry code uses the sanitized version.


863-864: Good defensive sanitization in ResponseStream.

The comment accurately describes the defensive nature of this sanitization - it protects against direct instantiation without pre-normalization, ensuring robustness.


650-652: LGTM - Consistent async implementation.

The async wrapper mirrors the sync wrapper's sanitization pattern correctly.

@alexey-pelykh alexey-pelykh force-pushed the fix/openai-NOT_GIVEN-Omit branch from e056d13 to ad3a6c7 Compare November 26, 2025 07:54
Copy link
Contributor

@galkleinman galkleinman left a comment

Choose a reason for hiding this comment

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

LGTM 💪

Weird design decision by OpenAI even though I found this related PEP https://peps.python.org/pep-0661/

@alexey-pelykh alexey-pelykh force-pushed the fix/openai-NOT_GIVEN-Omit branch from ad3a6c7 to 737649b Compare November 26, 2025 10:00
@galkleinman
Copy link
Contributor

one last lint issue, sorry for that :X

@alexey-pelykh alexey-pelykh force-pushed the fix/openai-NOT_GIVEN-Omit branch from 737649b to 6d72041 Compare November 26, 2025 10:19
@alexey-pelykh
Copy link
Contributor Author

@galkleinman should be fine now

@galkleinman galkleinman merged commit b2dc3f1 into traceloop:main Nov 26, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants