fix(openai): suppress duplicate httpx spans during instrumented calls#4270
fix(openai): suppress duplicate httpx spans during instrumented calls#4270leorivastech wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAll five OpenAI instrumentation wrapper modules ( ChangesSuppress HTTPX double-tracing in OpenAI wrappers
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/opentelemetry-instrumentation-openai/tests/traces/test_suppress_http_instrumentation.py`:
- Line 112: Replace the hardcoded API key literal "test-key" passed to the
OpenAI client constructor with a reference to an environment variable. In the
OpenAI client instantiation, change the api_key parameter from a hardcoded
string to use os.getenv() or similar environment variable lookup (such as
os.environ.get()) to retrieve the API key from the test environment instead of
committing it as a literal in the code. This change is needed in multiple
locations where the OpenAI client is instantiated with a hardcoded api_key
value, following the guideline that API keys should never be committed as code
literals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 933f137d-01e1-415b-aa81-2baedf561f94
⛔ Files ignored due to path filters (1)
packages/opentelemetry-instrumentation-openai/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/embeddings_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/image_gen_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.pypackages/opentelemetry-instrumentation-openai/pyproject.tomlpackages/opentelemetry-instrumentation-openai/tests/traces/test_suppress_http_instrumentation.py
2c2a453 to
f9568f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/opentelemetry-instrumentation-openai/tests/traces/test_suppress_http_instrumentation.py`:
- Around line 88-90: The HTTPServer socket is not being properly released in the
finally block of the test fixture teardown. After the existing server.shutdown()
and thread.join() calls, add server.server_close() to explicitly release the
bound socket and prevent accumulated open descriptors that can cause test
flakiness on repeated runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfe5bd3a-76ea-412e-be01-76fe117826cb
⛔ Files ignored due to path filters (1)
packages/opentelemetry-instrumentation-openai/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/embeddings_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/image_gen_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.pypackages/opentelemetry-instrumentation-openai/pyproject.tomlpackages/opentelemetry-instrumentation-openai/tests/traces/test_suppress_http_instrumentation.py
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/opentelemetry-instrumentation-openai/pyproject.toml
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/image_gen_wrappers.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/embeddings_wrappers.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py
When the underlying HTTP client is also instrumented (e.g. opentelemetry-instrumentation-httpx), every OpenAI call was traced twice: once as the openai.* span and once as a raw HTTP POST span from the HTTP-client instrumentation. Wrap the underlying SDK call in suppress_http_instrumentation() so the HTTP-client span is suppressed while the OpenAI span is still recorded. Applies to the chat, completion, embeddings, responses and image generation wrappers (sync, async and streaming). Fixes traceloop#2845
f9568f5 to
fadcdca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-openai/tests/traces/test_suppress_http_instrumentation.py (1)
122-124: Consider adding an optionalConsoleSpanExporterfor span-hierarchy debugging.This regression test validates span hierarchy behavior but lacks a console debug path for local diagnosis if the test fails. Adding an opt-in debug fixture (e.g., env-gated) that wires
ConsoleSpanExporteralongside the existingInMemorySpanExporterwould aid future troubleshooting without affecting normal test runs.Per coding guidelines: "Use
ConsoleSpanExporterfromopentelemetry.sdk.trace.exportfor debugging OpenTelemetry spans and hierarchy issues."Also applies to: 142-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opentelemetry-instrumentation-openai/tests/traces/test_suppress_http_instrumentation.py` around lines 122 - 124, Add an optional environment-gated ConsoleSpanExporter to aid debugging of span hierarchy in this regression test. Import ConsoleSpanExporter from opentelemetry.sdk.trace.export and modify the test fixture (likely the setup/initialization code that creates the span exporter used by the test assertions at lines 122-124) to conditionally wire both the existing InMemorySpanExporter and the ConsoleSpanExporter when a debug environment variable (e.g., DEBUG_SPANS) is set. This enables developers to see console output of span hierarchies during local test failures without affecting normal test runs. Apply the same pattern to the other affected location at lines 142-144.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-openai/tests/traces/test_suppress_http_instrumentation.py`:
- Around line 122-124: Add an optional environment-gated ConsoleSpanExporter to
aid debugging of span hierarchy in this regression test. Import
ConsoleSpanExporter from opentelemetry.sdk.trace.export and modify the test
fixture (likely the setup/initialization code that creates the span exporter
used by the test assertions at lines 122-124) to conditionally wire both the
existing InMemorySpanExporter and the ConsoleSpanExporter when a debug
environment variable (e.g., DEBUG_SPANS) is set. This enables developers to see
console output of span hierarchies during local test failures without affecting
normal test runs. Apply the same pattern to the other affected location at lines
142-144.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4530dc29-84cd-408d-899d-75b7aab19472
⛔ Files ignored due to path filters (1)
packages/opentelemetry-instrumentation-openai/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/embeddings_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/image_gen_wrappers.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.pypackages/opentelemetry-instrumentation-openai/pyproject.tomlpackages/opentelemetry-instrumentation-openai/tests/traces/test_suppress_http_instrumentation.py
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/opentelemetry-instrumentation-openai/pyproject.toml
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/embeddings_wrappers.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/image_gen_wrappers.py
What
When the underlying HTTP client is instrumented in addition to OpenAI — e.g.
HTTPXClientInstrumentor().instrument()alongsideOpenAIInstrumentor().instrument(),a very common setup — every OpenAI call is traced twice:
openai.chat(this instrumentation, with prompts/model/tokens), andPOSTspan from the httpx instrumentation for the same request.This wraps the underlying SDK call in
suppress_http_instrumentation()so theHTTP-client span is suppressed for the duration of the request, while the OpenAI
span is still recorded as usual.
This is the approach suggested by @nirga in the issue thread ("set the suppress
flag within the OpenAI instrumentation"). It uses the HTTP-specific suppression
key (
_SUPPRESS_HTTP_INSTRUMENTATION_KEY), not the global one, so only HTTP-clientinstrumentation is suppressed.
Applied to the request-firing wrappers: chat, completion, embeddings, responses
and image generation (sync, async and streaming). Assistants (polling) and realtime
(websocket, not HTTP) are out of scope.
Fixes #2845
How it was verified
Tested against a local HTTP server over a real socket, so the httpx instrumentation
actually runs (VCR replay would bypass the transport it wraps):
openai.chat+POSTopenai.chatopenai.chat+POSTopenai.chatAdded
tests/traces/test_suppress_http_instrumentation.pycovering both thestreaming and non-streaming paths; the tests fail without the fix
(
['POST', 'openai.chat'] != ['openai.chat']) and pass with it. Full packagesuite stays green (270 passed, 1 skipped) and
ruffis clean.Checklist
fix(openai): ...Summary by CodeRabbit
Bug Fixes
Tests
Chores