feat(sdk): expose use_legacy_attributes via Traceloop.init()#4133
feat(sdk): expose use_legacy_attributes via Traceloop.init()#4133dvirski wants to merge 1 commit into
Conversation
dvirski
requested review from
doronkopit5,
galzilber,
max-deygin-traceloop,
netanel-tl and
nina-kollman
📝 WalkthroughWalkthroughThe PR adds a ChangesLegacy Attributes Parameter Threading
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.
🧹 Nitpick comments (1)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
232-253: 💤 Low valueTest correctly verifies propagation to OpenAI instrumentor.
The test properly isolates itself by saving/restoring the
TracerWrapperinstance and verifies thatuse_legacy_attributes=Falsereaches the OpenAI instrumentor configuration.Consider adding complementary test coverage:
Optional test improvements
Verify the default case: Test that when
use_legacy_attributesis not specified (or set toTrue), the config defaults toTrue.Test multiple instrumentors: While testing OpenAI is representative, you could verify that the flag propagates to at least one other instrumentor (e.g., Anthropic or Groq) to ensure the pattern holds across different code paths.
Example for default case:
def test_use_legacy_attributes_defaults_to_true(): """Verify use_legacy_attributes defaults to True for backward compatibility.""" from opentelemetry.instrumentation.openai.shared.config import Config as OpenAIConfig _instance = None if hasattr(TracerWrapper, "instance"): _instance = TracerWrapper.instance del TracerWrapper.instance exporter = InMemorySpanExporter() Traceloop.init( exporter=exporter, disable_batch=True, # use_legacy_attributes not specified, should default to True ) assert OpenAIConfig.use_legacy_attributes is True if _instance is not None: TracerWrapper.instance = _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 232 - 253, Add complementary tests: create test_use_legacy_attributes_defaults_to_true() that mirrors test_use_legacy_attributes_false_propagates_to_instrumentors but omits the use_legacy_attributes argument when calling Traceloop.init(exporter=InMemorySpanExporter(), disable_batch=True) and assert OpenAIConfig.use_legacy_attributes is True; also add an additional test (e.g., test_use_legacy_attributes_propagates_to_other_instrumentor) that initializes the SDK with use_legacy_attributes=False and asserts the same flag reached another instrumentor's Config (e.g., AnthropicConfig or GroqConfig) to confirm propagation; keep the same TracerWrapper instance save/restore pattern used in the existing test to avoid global state leaks.
🤖 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 232-253: Add complementary tests: create
test_use_legacy_attributes_defaults_to_true() that mirrors
test_use_legacy_attributes_false_propagates_to_instrumentors but omits the
use_legacy_attributes argument when calling
Traceloop.init(exporter=InMemorySpanExporter(), disable_batch=True) and assert
OpenAIConfig.use_legacy_attributes is True; also add an additional test (e.g.,
test_use_legacy_attributes_propagates_to_other_instrumentor) that initializes
the SDK with use_legacy_attributes=False and asserts the same flag reached
another instrumentor's Config (e.g., AnthropicConfig or GroqConfig) to confirm
propagation; keep the same TracerWrapper instance save/restore pattern used in
the existing test to avoid global state leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc9db1b3-3297-4a1c-bd8b-32022889f21c
⛔ Files ignored due to path filters (2)
packages/sample-app/uv.lockis excluded by!**/*.lockpackages/traceloop-sdk/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/traceloop-sdk/tests/test_sdk_initialization.pypackages/traceloop-sdk/traceloop/sdk/__init__.pypackages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
|
The PR cleanly threads the flag through the SDK, and the mechanical scope is right (all 8 instrumentors that accept the kwarg are covered). Before merging though, I want to flag that the world shifted under issue #3236 in a way that affects what this flag should be named and what it should do. The OTel semconv decision reversedWhen #3236 was filed, the assumption was: "legacy attributes ( OpenLLMetry has already migratedSpot-checking the instrumentors, they all emit the new attributes today by default:
So What
|
| Flag value | What gets emitted | Status |
|---|---|---|
use_legacy_attributes=True (default) |
gen_ai.input.messages on span |
Current spec |
use_legacy_attributes=False (this PR exposes) |
OTel log events instead | Abandoned direction |
The name is now actively misleading: a user reading the SDK signature would reasonably assume False means "non-legacy / modern", but it actually opts them off the current spec and onto the events path.
Proposal: rename to use_attributes
This matches @nirga's original suggestion in #3236 ("use_attributes (or use_events)") and reflects reality without flipping defaults:
| Old | New |
|---|---|
use_legacy_attributes: bool = True |
use_attributes: bool = True |
Default stays True (no behavior change for existing users). The "legacy" word — which is the inaccurate part — goes away. False continues to mean "emit as events instead."
To keep the SDK and instrumentor-level APIs consistent, I'd suggest renaming at both layers in this PR:
- Rename the kwarg on the 8 instrumentor
__init__s touse_attributes, acceptinguse_legacy_attributesas a deprecated alias that emits aDeprecationWarningand forwards to the new name. - Same on
Traceloop.init(). - Remove the alias in the next major version.
This adds maybe ~15 lines of deprecation-shim code but gives us a clean, accurate public API without breaking anyone.
Two related items worth deciding before merge
-
EventLoggerProviderplumbing. The original issue also asked how to configure the event logger when opting into events. Today,use_legacy_attributes=Falsesilently relies on the global logger provider being configured. Worth documenting clearly that users must configure one themselves — otherwise setting the flag silently does nothing useful. -
Test robustness. The new test in
tests/test_sdk_initialization.py:232-253manually mutatesTracerWrapper.instanceinstead of using the existing fixture pattern fromconftest.py. It also only verifies one instrumentor (OpenAI). Worth parametrizing across 2-3 instrumentors and using the fixture pattern to avoid global-state leakage ifTraceloop.init()ever raises mid-call.
3 participants
Fixes #3236.
Problem: use_legacy_attributes existed on every instrumentation but was
unreachable through the SDK — users were locked to legacy gen_ai.prompt/
gen_ai.completion span attributes with no way to opt into the new
event-based format.
Fix: thread use_legacy_attributes=True (default, no behaviour change) from
Traceloop.init() through TracerWrapper, init_instrumentations(), and all
8 instrumentations that support the flag: openai, anthropic, bedrock,
sagemaker, groq, langchain, together, watsonx.
Summary by CodeRabbit
use_legacy_attributesparameter to SDK initialization, enabling control over tracing attribute formatting. Defaults toTruefor backward compatibility. This setting propagates across all supported AI service instrumentors for consistent behavior.