-
Notifications
You must be signed in to change notification settings - Fork 829
fix(request-model): Add check for the model attr in request object in openaiv2 #4017
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
Conversation
lmolkova
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.
Thanks!
...opentelemetry-instrumentation-openai-v2/src/opentelemetry/instrumentation/openai_v2/patch.py
Show resolved
Hide resolved
instrumentation-genai/opentelemetry-instrumentation-openai-v2/tests/test_chat_completions.py
Outdated
Show resolved
Hide resolved
|
@punitmahes Thanks for the PR, looks like some of the commits are done under a different name. Please fix the git history so that all commits are covered by the CLA |
|
@xrmx Done |
xrmx
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.
Please add an entry inside the instrumentation CHANGELOG
Description
This PR fixes an AttributeError that occurs when using the OpenAI client with with_raw_response. In this scenario, the result object returned is a LegacyAPIResponse which does not directly expose attributes like .model or .choices, causing the instrumentation to crash when attempting to set span attributes or record metrics.
Changes implemented:
Response Parsing: Added logic to detect if the result has a .parse() method (indicative of LegacyAPIResponse). If present, it calls .parse() to retrieve the actual response object containing the necessary attributes (model, choices, etc.).
Safe Attribute Access: Updated _set_response_attributes to use getattr for safer access to .model to prevent future AttributeErrors.
Fixes #4002
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
I have added new unit tests specifically for the with_raw_response flow for both synchronous and asynchronous clients, using VCR cassettes to mock the OpenAI API interaction.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.