fix(openai): materialize generator messages/tools before wrapped call (#825) - #826
Niranjan-png wants to merge 3 commits into
Conversation
Fixes open-telemetry#825. Prevents one-shot iterables from being consumed during content capture, which left the SDK request empty. - Materialize messages and tools to list before consumption - Update kwargs so the wrapped call sees the same materialized inputs
|
Pull request dashboard statusWaiting on the author · refreshed 2026-10-02 17:47 UTC Investigate required status check failures. Status above doesn't look right?
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Rebinding local kwargs leaves the caller’s dictionary unchanged, so generators remain exhausted, and regression tests are absent.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR attempts to prevent OpenAI message/tool iterators from being consumed during content capture.
Changes:
- Materializes non-list/tuple message and tool iterables.
- Attempts to pass materialized values to the wrapped SDK call.
| File | Description |
|---|---|
instrumentation/opentelemetry-instrumentation-genai-openai/src/opentelemetry/instrumentation/genai/openai/utils.py |
Adds iterable materialization before telemetry extraction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Materialize one-shot iterables so they aren't consumed before | ||
| # the wrapped API call (fixes #825). | ||
| messages_raw = kwargs.get("messages", []) |
…telemetry#825) - Materialize generator/map-backed messages and tools in patch.py traced_method before passing to create_chat_invocation and wrapped. - Reverts incorrect local-only kwargs fix in utils.py. - Adds regression tests for message and tool generators. Fixes open-telemetry#825
| def test_materialization_preserves_generator(): | ||
| MSGS = [{"role":"system","content":"x"},{"role":"user","content":"hi"}] | ||
| generator = (m for m in MSGS) | ||
| materialized = list(generator) if not isinstance(generator, (list, tuple)) else generator |
There was a problem hiding this comment.
This only verifies that Python’s list() consumes a generator; it doesn’t exercise the instrumented wrapper or confirm the SDK receives the materialized messages. The test would still pass if the production fix were removed. Could we test through a mock OpenAI transport and assert the request contains all messages? Please cover tools as well, including the async wrapper.
messagesandtoolsare consumed and the request is sent empty #825