feat(traceloop-sdk): add 'trace_content' flag and relevant test cases. Fixes #137 #3433
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Test
participant Env as Environment
participant SDK as Traceloop
Test->>Env: set TRACELOOP_TRACE_CONTENT="false"
Test->>SDK: init(trace_content=True)
SDK->>SDK: compute enable_content_tracing = trace_content or is_content_tracing_enabled()
SDK->>Env: (optional) is_content_tracing_enabled() -> false
SDK-->>Test: enable_content_tracing = True
Note right of SDK: Tracer initialized with content tracing enabled
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/traceloop-sdk/tests/conftest.py (2)
🔇 Additional comments (1)
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 |
visheshdvivedi
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 8d1f250 in 1 minute and 55 seconds. Click for details.
- Reviewed
59lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/traceloop/sdk/__init__.py:98
- Draft comment:
The expression 'trace_content or is_content_tracing_enabled()' may allow the environment variable’s value to override an explicit False. Confirm that an explicit False should not disable content tracing if the env var is truthy. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is technically correct about the behavior - iftrace_content=Falseand the environment variable is truthy, thenenable_content_tracingwill be True because of theoroperator. However, the comment is phrased as "Confirm that..." which is explicitly against the rules. The comment is asking the author to verify/confirm their intention rather than stating that there's definitely a problem. The default value isTrue, so the typical use case would be someone explicitly setting it toFalseto disable content tracing, but the env var could override that. This could be intentional (env var takes precedence) or a bug (explicit parameter should take precedence). Without more context about the intended behavior, this is speculative. The comment identifies a real behavioral quirk in the code logic, and it's about code that was actually changed in the diff. The concern is legitimate - typically when someone explicitly passes a parameter value, they expect it to be respected rather than overridden by an environment variable. This could be a genuine bug rather than just asking for confirmation. While the behavioral concern is real, the comment is phrased as "Confirm that..." which explicitly asks the author to verify their intention. This violates the rule that says "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended." The comment doesn't definitively state there's a problem - it's asking for confirmation, which makes it speculative rather than actionable. The comment should be deleted because it asks the PR author to "confirm" the intended behavior rather than stating a definitive issue. While the logic concern is valid, the phrasing violates the rule against asking authors to verify their intentions.
Workflow ID: wflow_oFaevp2D8YT18FLq
You can customize 
|
|
||
| @pytest.fixture | ||
| def exporter_with_trace_content_override(): | ||
| # Clear singleton if existed |
There was a problem hiding this comment.
Initialize _trace_wrapper_instance to None (e.g. before the if-check) so that the teardown code doesn’t raise a NameError if the attribute isn’t present.
| # Clear singleton if existed | |
| _trace_wrapper_instance = None |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/traceloop-sdk/tests/conftest.py(1 hunks)packages/traceloop-sdk/traceloop/sdk/__init__.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/traceloop-sdk/traceloop/sdk/__init__.pypackages/traceloop-sdk/tests/conftest.py
🧬 Code graph analysis (2)
packages/traceloop-sdk/traceloop/sdk/__init__.py (1)
packages/traceloop-sdk/traceloop/sdk/config/__init__.py (1)
is_content_tracing_enabled(8-9)
packages/traceloop-sdk/tests/conftest.py (2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
TracerWrapper(61-241)packages/traceloop-sdk/traceloop/sdk/__init__.py (2)
Traceloop(37-276)init(49-207)
| del TracerWrapper.instance | ||
|
|
||
| # Set environment variable to false | ||
| os.environ["TRACELOOP_TRACE_CONTENT"] = "false" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Preserve the original environment variable value.
The fixture unconditionally deletes TRACELOOP_TRACE_CONTENT in cleanup, which could raise KeyError if the test deletes it, or could lose a pre-existing value set outside the test.
Apply this diff:
+ _original_trace_content = os.environ.get("TRACELOOP_TRACE_CONTENT")
# Set environment variable to false
os.environ["TRACELOOP_TRACE_CONTENT"] = "false"
...
# Clean up environment
- del os.environ["TRACELOOP_TRACE_CONTENT"]
+ if _original_trace_content is not None:
+ os.environ["TRACELOOP_TRACE_CONTENT"] = _original_trace_content
+ else:
+ os.environ.pop("TRACELOOP_TRACE_CONTENT", None)Also applies to: 250-250
🤖 Prompt for AI Agents
In packages/traceloop-sdk/tests/conftest.py around lines 234 and 250, the
fixture unconditionally deletes TRACELOOP_TRACE_CONTENT in cleanup which can
raise KeyError or clobber an existing external value; capture the original
os.environ.get("TRACELOOP_TRACE_CONTENT") before setting it, and in teardown
restore it (if original is None remove safely using
os.environ.pop("TRACELOOP_TRACE_CONTENT", None) or if original had a value set
it back) so the environment is restored without raising errors or losing
pre-existing values.
| sampler: Optional[Sampler] = None, | ||
| traceloop_sync_enabled: bool = False, | ||
| should_enrich_metrics: bool = True, | ||
| trace_content: bool = True, |
There was a problem hiding this comment.
Breaking change: default trace_content=True overrides environment variable.
The default value of True breaks backward compatibility. Previously, is_content_tracing_enabled() controlled content tracing based on the TRACELOOP_TRACE_CONTENT environment variable. Now, the OR logic at line 98 means content tracing is always enabled when callers don't pass the parameter, ignoring TRACELOOP_TRACE_CONTENT=false.
Additionally, the OR semantics prevent true programmatic override: trace_content=False cannot disable content tracing if the environment variable is set to "true", contradicting the PR objective of "explicit programmatic control."
To maintain backward compatibility and enable true override, use None as the default:
- trace_content: bool = True,
+ trace_content: Optional[bool] = None,
resource_attributes: dict = {},And update the logic at line 98:
- enable_content_tracing = trace_content or is_content_tracing_enabled()
+ enable_content_tracing = (
+ trace_content if trace_content is not None else is_content_tracing_enabled()
+ )This ensures:
trace_content=True→ force enable (override env var)trace_content=False→ force disable (override env var)trace_content=None(default) → defer to env var (backward compatible)
Also applies to: 98-98
🤖 Prompt for AI Agents
In packages/traceloop-sdk/traceloop/sdk/__init__.py at line 67 and affecting
logic at line 98, the default trace_content=True both breaks backwards
compatibility and prevents programmatic overrides; change the parameter default
from True to None, and update the decision at line 98 to use the explicit value
when trace_content is not None, otherwise fall back to
is_content_tracing_enabled() (i.e., use trace_content if provided, else consult
the env-based helper), so True forces enable, False forces disable, and None
defers to the environment.
2 participants
feat(instrumentation): ...orfix(instrumentation): ....Important
Add
trace_contentflag toTraceloop.init()to overrideTRACELOOP_TRACE_CONTENTenv var, with tests inconftest.py.trace_contentflag toTraceloop.init()in__init__.pyto control content tracing, overridingTRACELOOP_TRACE_CONTENTenv var.enable_content_tracinglogic to usetrace_contentflag.exporter_with_trace_content_overridefixture inconftest.pyto testtrace_contentflag overriding env var.TRACELOOP_TRACE_CONTENTtofalseand initializesTraceloopwithtrace_content=True.This description was created by
for 8d1f250. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
trace_contentinitialization parameter to allow explicit control of trace content capture behavior.Tests