-
Notifications
You must be signed in to change notification settings - Fork 868
fix(openai): recognize NOT_GIVEN and Omit #3473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(openai): recognize NOT_GIVEN and Omit #3473
Conversation
WalkthroughAdds 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
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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (9)
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 |
68952b3 to
e056d13
Compare
There was a problem hiding this 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
440lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft 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 betweenopenai.NotGivenandopenai._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 likesafe_kwargs.get("reasoning", {}).get("summary"). The issue is: ifkwargs["reasoning"]is a sentinel value, it gets removed (good). But ifkwargs["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 issafe_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%<= threshold50%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%<= threshold50%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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this 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_GIVENfromopenai._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
Omitmay 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 testpackages/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_wrapperandasync_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.
📒 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.pypackages/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
kwargsis still passed towrapped(*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.
e056d13 to
ad3a6c7
Compare
galkleinman
left a comment
There was a problem hiding this 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/
...lemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py
Outdated
Show resolved
Hide resolved
ad3a6c7 to
737649b
Compare
|
one last lint issue, sorry for that :X |
737649b to
6d72041
Compare
|
@galkleinman should be fine now |
feat(instrumentation): ...orfix(instrumentation): ....Important
Fix handling of OpenAI sentinel values
NOT_GIVENandOmitinresponses_wrappers.pyto prevent errors, with tests added intest_responses.py._get_openai_sentinel_types()inresponses_wrappers.pyto dynamically discover OpenAI sentinel typesNOT_GIVENandOmit._sanitize_sentinel_values()to remove sentinel values fromkwargsto prevent errors when accessing attributes.responses_get_or_create_wrapper()andasync_responses_get_or_create_wrapper()to use sanitizedkwargs.test_response_stream_init_with_not_given_reasoning()andtest_response_stream_init_with_omit_reasoning()intest_responses.pyto verifyResponseStreaminitialization with sentinel values.AttributeErroris raised whenreasoningis set toNOT_GIVENorOmit.This description was created by
for 68952b3. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.