fix(openai): expose headers on streaming with_raw_response wrapper#147
Open
YuxiangJiangCT wants to merge 1 commit into
Open
fix(openai): expose headers on streaming with_raw_response wrapper#147YuxiangJiangCT wants to merge 1 commit into
YuxiangJiangCT wants to merge 1 commit into
Conversation
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes a regression where with_raw_response + stream=True chat completions lost access to the original response headers after parsing, by carrying headers through the stream wrapper.
Changes:
- Capture raw response headers before
parse()discards the legacy response object (sync + async paths). - Extend chat stream wrappers to expose
raw_response.headers. - Add sync/async regression tests asserting headers are accessible and streaming still works.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| instrumentation/opentelemetry-instrumentation-genai-openai/tests/test_chat_completions.py | Adds sync regression test verifying .headers is exposed on streaming raw responses. |
| instrumentation/opentelemetry-instrumentation-genai-openai/tests/test_async_chat_completions.py | Adds async regression test verifying .headers is exposed on streaming raw responses. |
| instrumentation/opentelemetry-instrumentation-genai-openai/src/opentelemetry/instrumentation/genai/openai/patch.py | Captures headers pre-parse and passes them into stream wrappers for sync/async instrumentation. |
| instrumentation/opentelemetry-instrumentation-genai-openai/src/opentelemetry/instrumentation/genai/openai/chat_wrappers.py | Adds headers property and plumbs headers through ChatStreamWrapper and AsyncChatStreamWrapper. |
Comment on lines
+1399
to
+1400
| @pytest.mark.asyncio() | ||
| async def test_chat_completion_with_raw_response_streaming_exposes_headers( |
Author
There was a problem hiding this comment.
Kept @pytest.mark.asyncio() with parens to match the existing async tests in this file - changing only this one would be inconsistent. Happy to switch all of them in a separate cleanup if preferred.
Accessing .headers on the wrapper returned by chat.completions.with_raw_response.create(stream=True) raised AttributeError. parse() discards the LegacyAPIResponse that carries the headers, and ChatStreamWrapper kept no reference to them. The responses path already handles this, chat never did. Capture the headers before parse() and expose them via a property on the sync and async chat stream wrappers, with tests for both. Fixes open-telemetry#46
YuxiangJiangCT
force-pushed
the
fix/openai-chat-stream-headers
branch
from
3801e7e to
d7ccaee
Compare
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.
Description
chat.completions.with_raw_response.create(stream=True) returns a
ChatStreamWrapper, but accessing .headers on it raised AttributeError.
parse() drops the LegacyAPIResponse the headers come from, and the wrapper
didn't keep a reference. The Responses API wrapper already exposes headers
via .response; the chat path just never did.
This captures the headers before parse() and exposes them through a headers
property on the sync and async chat stream wrappers.
Fixes #46
Type of change
How has this been tested?
Added regression tests for the sync and async paths
(test_chat_completion_with_raw_response_streaming_exposes_headers) asserting
raw_response.headers is accessible and that streaming still works. They fail
on main with AttributeError and pass with the fix.
Checklist