fix(sdk): warn when both exporter and processor are passed to Traceloop.init()#4132
Open
dvirski wants to merge 2 commits into
Open
Conversation
added 2 commits
May 12, 2026 16:57
dvirski
requested review from
doronkopit5,
galzilber,
max-deygin-traceloop,
netanel-tl and
nina-kollman
📝 WalkthroughWalkthroughThe PR adds response instruction prepending to traced OpenAI agent input messages when ChangesResponse Instructions Prepending in Traced Input
Exporter and Processor Conflict Warning
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 docstrings
🧪 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 |
|
Dvir Rezenman seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
242-246: ⚡ Quick winAssert warning source location to cover the
stacklevel=2behavior.The test currently checks only message content; validating filename/lineno would lock in the caller-line contract from
Traceloop.init.Suggested assertion enhancement
+import inspect @@ - with pytest.warns(UserWarning, match="exporter.*ignored"): - Traceloop.init( - exporter=exporter, - processor=processor, - ) + with pytest.warns(UserWarning, match="exporter.*ignored") as caught: + call_line = inspect.currentframe().f_lineno + 1 + Traceloop.init( + exporter=exporter, + processor=processor, + ) + warning = caught.list[0] + assert warning.filename == __file__ + assert warning.lineno == call_line🤖 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/traceloop-sdk/tests/test_sdk_initialization.py` around lines 242 - 246, The test should capture the warnings recorder from the pytest.warns context and assert the warning's source file and line to verify stacklevel=2 behavior: wrap the Traceloop.init call with "with pytest.warns(UserWarning, match='exporter.*ignored') as rec:" then inspect rec.list[0] (or rec.records[0]) and assert record.filename endswith "test_sdk_initialization.py" and that record.lineno equals the line where Traceloop.init was invoked; update the assertion locations around the Traceloop.init call to use these checks so the test enforces the stacklevel caller location contract.
🤖 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/traceloop-sdk/tests/test_sdk_initialization.py`:
- Around line 235-249: The test mutates TracerWrapper.instance and only restores
it after the pytest.warns block, which can leak state if the assertion fails;
wrap the call to Traceloop.init (the pytest.warns(...) context and its body) in
a try/finally and move the restoration of TracerWrapper.instance (using the
saved _instance) into the finally so TracerWrapper.instance is always restored
even if the warning assertion fails; locate the code around
TracerWrapper.instance, _instance, and the Traceloop.init(...) call and
implement the try/finally around that block.
---
Nitpick comments:
In `@packages/traceloop-sdk/tests/test_sdk_initialization.py`:
- Around line 242-246: The test should capture the warnings recorder from the
pytest.warns context and assert the warning's source file and line to verify
stacklevel=2 behavior: wrap the Traceloop.init call with "with
pytest.warns(UserWarning, match='exporter.*ignored') as rec:" then inspect
rec.list[0] (or rec.records[0]) and assert record.filename endswith
"test_sdk_initialization.py" and that record.lineno equals the line where
Traceloop.init was invoked; update the assertion locations around the
Traceloop.init call to use these checks so the test enforces the stacklevel
caller location contract.
🪄 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: d5ba66d7-e16c-4114-8fa4-5c8d2011e920
⛔ Files ignored due to path filters (3)
packages/opentelemetry-instrumentation-openai-agents/uv.lockis excluded by!**/*.lockpackages/sample-app/uv.lockis excluded by!**/*.lockpackages/traceloop-sdk/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/_hooks.pypackages/opentelemetry-instrumentation-openai-agents/tests/test_openai_agents.pypackages/opentelemetry-instrumentation-openai-agents/tests/test_tracing_processor.pypackages/traceloop-sdk/tests/conftest.pypackages/traceloop-sdk/tests/test_sdk_initialization.pypackages/traceloop-sdk/traceloop/sdk/__init__.py
💤 Files with no reviewable changes (1)
- packages/traceloop-sdk/tests/conftest.py
Comment on lines
+235
to
+249
| if hasattr(TracerWrapper, "instance"): | ||
| _instance = TracerWrapper.instance | ||
| del TracerWrapper.instance | ||
|
|
||
| exporter = InMemorySpanExporter() | ||
| processor = SimpleSpanProcessor(exporter) | ||
|
|
||
| with pytest.warns(UserWarning, match="exporter.*ignored"): | ||
| Traceloop.init( | ||
| exporter=exporter, | ||
| processor=processor, | ||
| ) | ||
|
|
||
| if "_instance" in dir(): | ||
| TracerWrapper.instance = _instance |
There was a problem hiding this comment.
Restore TracerWrapper.instance in a finally block to prevent cross-test state leakage.
If the warning assertion fails before cleanup, this test can leave mutated global state and cause downstream flaky failures.
Proposed fix
def test_both_exporter_and_processor_warns():
@@
- if hasattr(TracerWrapper, "instance"):
- _instance = TracerWrapper.instance
- del TracerWrapper.instance
+ had_instance = hasattr(TracerWrapper, "instance")
+ previous_instance = getattr(TracerWrapper, "instance", None)
+ if had_instance:
+ del TracerWrapper.instance
@@
- with pytest.warns(UserWarning, match="exporter.*ignored"):
- Traceloop.init(
- exporter=exporter,
- processor=processor,
- )
-
- if "_instance" in dir():
- TracerWrapper.instance = _instance
+ try:
+ with pytest.warns(UserWarning, match="exporter.*ignored"):
+ Traceloop.init(
+ exporter=exporter,
+ processor=processor,
+ )
+ finally:
+ if had_instance:
+ TracerWrapper.instance = previous_instance🤖 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/traceloop-sdk/tests/test_sdk_initialization.py` around lines 235 -
249, The test mutates TracerWrapper.instance and only restores it after the
pytest.warns block, which can leak state if the assertion fails; wrap the call
to Traceloop.init (the pytest.warns(...) context and its body) in a try/finally
and move the restoration of TracerWrapper.instance (using the saved _instance)
into the finally so TracerWrapper.instance is always restored even if the
warning assertion fails; locate the code around TracerWrapper.instance,
_instance, and the Traceloop.init(...) call and implement the try/finally around
that block.
doronkopit5
reviewed
May 12, 2026
Comment on lines
+944
to
+946
| response = getattr(span_data, "response", None) | ||
| if trace_content and response and getattr(response, "instructions", None): | ||
| input_data = [{"role": "system", "content": response.instructions}] + (input_data if input_data else []) |
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
3 participants
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 #3046
Problem: passing both exporter and processor is a mistake — the processor already wraps the exporter
internally. The exporter was silently ignored with no feedback to the user.
Fix: emit a
UserWarningpointing at the caller's line. Also removes the redundantexporter=from theexporter_with_custom_span_processortest fixture which had the same bug.Summary by CodeRabbit
New Features
Improvements