fix(openai): Don't let instrumentation raise into user code - #7223
Draft
FromCSUZhou wants to merge 1 commit into
Draft
fix(openai): Don't let instrumentation raise into user code#7223FromCSUZhou wants to merge 1 commit into
FromCSUZhou wants to merge 1 commit into
Conversation
Instrumenting a response whose `output` is `None` raised `TypeError: 'NoneType' object is not iterable` from inside the user's `create()` call, hiding the real error returned by the API. Guard `response.output` against `None` the same way `response.choices` already is, and restore the `capture_internal_exceptions()` protection around the request/response recording helpers, which was lost when the integration was restructured in getsentry#4612. The span is still finished when recording fails, so a failure cannot leak an unfinished span. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7222
What
The OpenAI integration can raise out of the instrumented
create()call, so an SDK problem surfaces as an application error and replaces whatever the API actually returned:Why
Two independent causes, both addressed here.
1.
response.outputis never checked forNone.response.choicesgot anis not Nonecheck after #5071, but the Responses API branch still does a barehasattr(response, "output")in_set_common_output_dataand_calculate_responses_token_usage.2. The non-streaming paths lost their
capture_internal_exceptions()wrapper. Before #4612 the whole response-handling block ran insidewith capture_internal_exceptions():. Splitting it into_set_*_input_data/_set_*_output_datamoved that code out from under the wrapper, so errors there now propagate into user code. The streaming iterators kept their protection; the non-streaming paths did not — which is why<2.34is unaffected.(1) fixes the crash that was actually hit; (2) makes this class of bug degrade to a missing span attribute rather than an application crash, per the SDK contract in
CONTRIBUTING.md("Users do not expect their application to crash").How it is reached in practice
Through an OpenAI-compatible gateway. When a gateway has already flushed
200 OK— for example after sending keep-alive padding while it buffers a large request — and only then learns the upstream call failed, it cannot change the status code, so it reports the failure in the body:{"error": {"message": "... At least one of the image dimensions exceed max allowed size: 8000 pixels", "code": 400}}The
openaiclient parses this into a model object withoutput/choicesunset. Instrumenting it raises, and the application seesTypeError: 'NoneType' object is not iterableinstead of the real message — so retry and fallback logic keyed on the upstream error stops working.Minimal reproduction, no network needed:
Changes
_calculate_responses_token_usageand_set_common_output_data: requireresponse.output is not None, mirroring the existingresponse.choices is not Nonecheck. A response withoutput=Nonenow falls through to the same branch a response withchoices=Nonealready does._set_responses_api_input_data,_set_completions_api_input_data,_set_embeddings_input_dataand_set_common_output_dataare now thin wrappers that run the recording work insidecapture_internal_exceptions(). The bodies moved unchanged into_record_*helpers._set_common_output_datafinishes the span after the guarded block rather than at the end of each branch, so a failure while recording cannot leave an unfinished span behind.Regarding #4853 (
asyncio.CancelledErrorswallowed bycapture_internal_exceptions()): that report concerned a wrapper spanning the user's iterator consumption. The blocks wrapped here contain only synchronous instrumentation code with noawaitpoints and no calls back into user code, so a task cancellation cannot be absorbed by them.Testing
Added to
tests/integrations/openai/test_openai.py:test_responses_api_none_output_does_not_crash/..._async— a Responses object withoutput=Noneis returned to the caller, and the span is still recorded and finished.test_chat_completion_none_choices_does_not_crash— regression guard for thechoices=Nonepath fixed in fix(openai): Check response text is present to avoid AttributeError #5081.test_instrumentation_output_error_does_not_propagate/test_instrumentation_input_error_does_not_propagate— an unexpected error while recording request or response data does not reach the caller, and the span is still finished.4 of the 5 fail on
masterand pass with this change; the fifth guards the already-fixedchoices=Nonepath.(694 before this change, 5 new.)
ruff format --checkandruff checkare clean on both touched files, andmypyreports no new errors relative tomaster.