fix(sdk): warn when both exporter and processor are passed to Traceloop.init()#4137
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEmits a UserWarning when both ChangesExporter and Processor Warning
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
235-237: ⚡ Quick winPrefer a more robust pattern for instance cleanup.
The pattern
if "_instance" in dir():works but is fragile and non-idiomatic. A more explicit approach improves clarity and reliability.♻️ Proposed refactor using try-except
- if hasattr(TracerWrapper, "instance"): - _instance = TracerWrapper.instance + try: + _instance = TracerWrapper.instance del TracerWrapper.instance + except AttributeError: + _instance = None exporter = InMemorySpanExporter() processor = SimpleSpanProcessor(exporter) @@ -245,5 +246,5 @@ processor=processor, ) - if "_instance" in dir(): + if _instance is not None: TracerWrapper.instance = _instanceAlso applies to: 248-249
🤖 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 - 237, Replace the fragile instance cleanup with a robust try/except/finally pattern: use getattr(TracerWrapper, "instance", None) to grab the current instance, then if it is not None call delattr(TracerWrapper, "instance") inside a try block and in finally restore the saved instance with setattr(TracerWrapper, "instance", saved) so the test cannot leak state; apply the same change for the other cleanup at the second occurrence (lines referencing TracerWrapper, _instance, instance).
🤖 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/traceloop-sdk/tests/test_sdk_initialization.py`:
- Around line 235-237: Replace the fragile instance cleanup with a robust
try/except/finally pattern: use getattr(TracerWrapper, "instance", None) to grab
the current instance, then if it is not None call delattr(TracerWrapper,
"instance") inside a try block and in finally restore the saved instance with
setattr(TracerWrapper, "instance", saved) so the test cannot leak state; apply
the same change for the other cleanup at the second occurrence (lines
referencing TracerWrapper, _instance, instance).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3721c9b8-fc97-4609-a09c-6e92f952dffa
⛔ Files ignored due to path filters (1)
packages/traceloop-sdk/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/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
0668e0c to
ccd0832
Compare
|
Fixed both your comments (in resolved state now) |
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 UserWarning pointing at the caller's line. Also removes the redundant
exporter= from the exporter_with_custom_span_processor test fixture which had the same bug.
Summary by CodeRabbit
New Features
Tests