feat(sdk): expose use_legacy_attributes via Traceloop.init()#4133
Conversation
dvirski
requested review from
doronkopit5,
galzilber,
max-deygin-traceloop,
netanel-tl and
nina-kollman
📝 WalkthroughWalkthroughAdds a ChangesAttribute vs events configuration propagation
Sequence DiagramsequenceDiagram
participant Client
participant TraceloopInit
participant TracerWrapper
participant InitInstrumentations
participant OpenAIInstrumentor
participant AnthropicInstrumentor
Client->>TraceloopInit: Traceloop.init(use_attributes=...)
TraceloopInit->>TracerWrapper: TracerWrapper(..., use_attributes=...)
TracerWrapper->>InitInstrumentations: init_instrumentations(use_attributes)
InitInstrumentations->>OpenAIInstrumentor: OpenAIInstrumentor(use_attributes=...)
InitInstrumentations->>AnthropicInstrumentor: AnthropicInstrumentor(use_attributes=...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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 |
|
|
![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.
dvirski
force-pushed
the
dr/feat(sdk)-expose-use_legacy_attributes-via-Traceloop.init()
branch
from
7541daa to
b7c4747
Compare
|
@doronkopit5 Your suggestions were on point. What has changed:
Default behavior unchanged; existing callers using use_legacy_attributes keep working (with a warning) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/traceloop-sdk/traceloop/sdk/__init__.py (1)
57-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid mutable defaults in
Traceloop.init.Line 57 and Line 70 use
{}defaults. Those dicts are shared across calls and can retain mutated state unexpectedly.Suggested fix
- headers: Dict[str, str] = {}, + headers: Optional[Dict[str, str]] = None, ... - resource_attributes: dict = {}, + resource_attributes: Optional[dict] = None, ... - headers = os.getenv("TRACELOOP_HEADERS") or headers + headers = os.getenv("TRACELOOP_HEADERS") or headers or {} ... - resource_attributes.update({SERVICE_NAME: app_name}) + resource_attributes = resource_attributes or {} + resource_attributes.update({SERVICE_NAME: app_name})Also applies to: 70-70
🤖 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/traceloop/sdk/__init__.py` at line 57, The init method uses mutable default dicts (headers: Dict[str, str] = {} and the other {} at line 70) which can retain state across calls; change those parameter defaults to None (e.g., headers: Optional[Dict[str,str]] = None) and inside Traceloop.init initialize them with an empty dict if None (headers = headers or {}) so each call gets a fresh dict; update any docstrings and type hints accordingly and ensure callers are unaffected.
🤖 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-watsonx/opentelemetry/instrumentation/watsonx/__init__.py`:
- Around line 648-669: The _emit_response_events function currently has a
mismatched signature vs its call sites, causing a TypeError when callers pass an
event_logger; update the _emit_response_events definition to accept an optional
second parameter (e.g., def _emit_response_events(responses, event_logger=None))
and make its body robust to both single-response dicts and lists of responses
and to a None event_logger, so both callers (the dont_throw-decorated call that
does _emit_response_events(responses, event_logger) and the other call that
passes only responses) succeed without raising; ensure the function emits events
using event_logger when provided and safely no-ops or logs appropriately when it
is None.
---
Outside diff comments:
In `@packages/traceloop-sdk/traceloop/sdk/__init__.py`:
- Line 57: The init method uses mutable default dicts (headers: Dict[str, str] =
{} and the other {} at line 70) which can retain state across calls; change
those parameter defaults to None (e.g., headers: Optional[Dict[str,str]] = None)
and inside Traceloop.init initialize them with an empty dict if None (headers =
headers or {}) so each call gets a fresh dict; update any docstrings and type
hints accordingly and ensure callers are unaffected.
🪄 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: 5a0e3f6f-4554-45f6-9761-037b35fb8bfd
📒 Files selected for processing (11)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.pypackages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/__init__.pypackages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.pypackages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/__init__.pypackages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/__init__.pypackages/opentelemetry-instrumentation-together/opentelemetry/instrumentation/together/__init__.pypackages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pypackages/traceloop-sdk/tests/test_sdk_initialization.pypackages/traceloop-sdk/traceloop/sdk/__init__.pypackages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
| def __init__( | ||
| self, | ||
| exception_logger=None, | ||
| use_attributes: bool = True, | ||
| use_legacy_attributes=_USE_ATTRIBUTES_UNSET, | ||
| ): | ||
| super().__init__() | ||
| if use_legacy_attributes is not _USE_ATTRIBUTES_UNSET: | ||
| warnings.warn( | ||
| "`use_legacy_attributes` is deprecated and will be removed in a " | ||
| "future release; use `use_attributes` instead. The current OTel " | ||
| "GenAI spec emits prompts/completions as span attributes " | ||
| "(`gen_ai.input.messages` / `gen_ai.output.messages`), which is " | ||
| "what `use_attributes=True` (the default) does. " | ||
| "`use_attributes=False` opts into the events path instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| use_attributes = use_legacy_attributes | ||
| Config.exception_logger = exception_logger | ||
| Config.use_legacy_attributes = use_legacy_attributes | ||
| Config.use_legacy_attributes = use_attributes | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify definition/call mismatch for _emit_response_events
rg -n -C2 'def _emit_response_events\(|_emit_response_events\(' \
packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 525
🏁 Script executed:
# Get full context around function definition (line 365)
sed -n '360,375p' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 537
🏁 Script executed:
# Get full context around the first call site (line 520)
sed -n '515,535p' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 616
🏁 Script executed:
# Get full context around the second call site (line 539)
sed -n '535,555p' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 585
🏁 Script executed:
# Search for emit_event function to see what parameters it expects
rg -n -A 5 'def emit_event' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 47
🏁 Script executed:
# Search for `@dont_throw` decorator usage around the calls
rg -n -B 3 '_handle_response|_emit_response_events' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py | head -40Repository: traceloop/openllmetry
Length of output: 668
🏁 Script executed:
# Get the full _emit_response_events function to see all emit_event calls
sed -n '365,375p' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 424
🏁 Script executed:
# Verify that the second call site at line 539 also shows the mismatch
sed -n '538,547p' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 386
🏁 Script executed:
# Check if there are more lines after 547 for the second call site
sed -n '538,550p' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pyRepository: traceloop/openllmetry
Length of output: 469
🏁 Script executed:
# Verify the emit_event signature to confirm it expects event_logger
rg -n -B 2 -A 3 'def emit_event' packages/opentelemetry-instrumentation-watsonx/Repository: traceloop/openllmetry
Length of output: 811
🏁 Script executed:
# Check if there's a second argument after the comma at line 546
sed -n '539,550p' packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py | cat -ARepository: traceloop/openllmetry
Length of output: 435
use_attributes=False now exposes a broken Watsonx events path for non-stream responses.
With this constructor change, events mode is easier to reach, but Line 520 calls _emit_response_events(responses, event_logger) while the function at line 365 is defined with a single parameter. Line 539 similarly calls it with a response dict but omits the event_logger. Both calls occur within @dont_throw-decorated functions, so the resulting TypeError is silently swallowed, causing completion events to never be emitted.
💡 Proposed fix
-def _emit_response_events(response: dict):
+def _emit_response_events(response: dict, event_logger):
for i, message in enumerate(response.get("results", [])):
emit_event(
ChoiceEvent(
index=i,
message={"content": message.get("generated_text"), "role": "assistant"},
finish_reason=message.get("stop_reason", "unknown"),
- )
+ ),
+ event_logger,
)- _emit_response_events(
+ _emit_response_events(
{
"results": [
{
"stop_reason": stream_stop_reason,
"generated_text": stream_generated_text,
}
]
},
+ event_logger,
)🤖 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-watsonx/opentelemetry/instrumentation/watsonx/__init__.py`
around lines 648 - 669, The _emit_response_events function currently has a
mismatched signature vs its call sites, causing a TypeError when callers pass an
event_logger; update the _emit_response_events definition to accept an optional
second parameter (e.g., def _emit_response_events(responses, event_logger=None))
and make its body robust to both single-response dicts and lists of responses
and to a None event_logger, so both callers (the dont_throw-decorated call that
does _emit_response_events(responses, event_logger) and the other call that
passes only responses) succeed without raising; ensure the function emits events
using event_logger when provided and safely no-ops or logs appropriately when it
is None.
There was a problem hiding this comment.
Valid finding but pre-existing — not introduced by this PR. _emit_response_events had the broken signature on main before this branch was cut, and the bug only fires when use_attributes=False (events path) + a 2-arg call site is hit; even then it's silently swallowed by the surrounding
@dont_throw. This PR is scoped to the kwarg rename, so I'll leave the watsonx event-emission bug for a follow-up that can also fix the matching issue inside _emit_response_events itself (the emit_event(ChoiceEvent(...)) call also passes only one arg and would TypeError on its own).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Thanks @dvirski — the rename, the deprecation shim, the docstring, the fixture-based tests are all on point. Two concrete issues with the implementation though: 1. CI lint will fail —
|
| File | Ruff errors |
|---|---|
opentelemetry-instrumentation-anthropic/__init__.py |
19 |
opentelemetry-instrumentation-bedrock/__init__.py |
19 |
opentelemetry-instrumentation-groq/__init__.py |
17 |
opentelemetry-instrumentation-langchain/__init__.py |
15 |
opentelemetry-instrumentation-sagemaker/__init__.py |
14 |
opentelemetry-instrumentation-together/__init__.py |
14 |
opentelemetry-instrumentation-watsonx/__init__.py |
15 |
traceloop-sdk/sdk/__init__.py + tracing.py |
41 combined |
opentelemetry-instrumentation-openai/__init__.py |
0 (sentinel placed correctly below imports) |
2. The whole sentinel machinery is overengineering — Optional[bool] = None is the idiomatic fix
None isn't a valid value for a bool flag, so it's a natural sentinel. Drops 9 module-level objects, sidesteps the import-ordering trap entirely, and reads cleaner:
def __init__(
self,
...
use_attributes: bool = True,
use_legacy_attributes: Optional[bool] = None,
):
if use_legacy_attributes is not None:
warnings.warn(...)
use_attributes = use_legacy_attributesThis also addresses a subtle behavior issue with the current shim: if a caller passes both kwargs — Traceloop.init(use_attributes=False, use_legacy_attributes=True) — the deprecated one silently clobbers the explicit new one. Worth deciding which should win (I'd vote: the new kwarg wins, or raise TypeError), and either way add a one-line test.
Nit
The same ~12-line warnings.warn(...) block is copy-pasted 9 times. A small helper (e.g. traceloop.sdk._deprecation.warn_use_legacy_attributes()) would keep the message in one place if it ever needs to change.
Everything else looks good to merge once these are addressed.
dvirski
force-pushed
the
dr/feat(sdk)-expose-use_legacy_attributes-via-Traceloop.init()
branch
from
b7c4747 to
4f9b300
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/traceloop-sdk/traceloop/sdk/__init__.py`:
- Around line 86-88: The docs incorrectly reference an EventLoggerProvider and
opentelemetry._events.set_event_logger_provider; update the text to say
Traceloop emits through the OpenTelemetry logging API
(opentelemetry._logs.get_logger(...)) and instruct users to configure a
LoggerProvider via opentelemetry._logs.set_logger_provider (or their
OpenTelemetry logging SDK) so log-backed events and prompt/completion data are
recorded when use_attributes=False; change the mention of
EventLoggerProvider/set_event_logger_provider to
LoggerProvider/set_logger_provider and call out opentelemetry._logs.get_logger
as the emission point.
🪄 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: 4c5a168d-51ba-4c0a-9493-235880cf2ac5
📒 Files selected for processing (11)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.pypackages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/__init__.pypackages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.pypackages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/__init__.pypackages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/__init__.pypackages/opentelemetry-instrumentation-together/opentelemetry/instrumentation/together/__init__.pypackages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pypackages/traceloop-sdk/tests/test_sdk_initialization.pypackages/traceloop-sdk/traceloop/sdk/__init__.pypackages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/traceloop-sdk/tests/test_sdk_initialization.py
|
Fixed last comments: summary of what was fixed according to your comment numbers:
On the both-kwargs question: went with raise TypeError rather than "new wins". Passing both is a programmer error, and a loud failure beats either kwarg silently winning. Added a test (test_passing_both_kwargs_raises_type_error) asserting pytest.raises(TypeError, match="Cannot pass both"). Nit — duplicated 12-line warning across 9 sites
|
| stacklevel=2, | ||
| ) | ||
| use_attributes = use_legacy_attributes | ||
| if use_attributes is None: |
There was a problem hiding this comment.
This means the use_attributes should have a default value of True - it should not be optional
There was a problem hiding this comment.
▎ Good catch on the signature — fair to question why use_attributes is typed Optional[bool] when conceptually it's a bool defaulting to True.
▎
▎ The None default isn't because None is a meaningful runtime value — it's a marker for "caller didn't pass this kwarg." I need that marker to detect when a caller passes both use_attributes and the deprecated use_legacy_attributes in the same call, and raise TypeError (per Doron's earlier ask —
▎ silent clobber of an explicit kwarg is sneaky). The check is:
▎
▎ if use_attributes is not None and use_legacy_attributes is not None:
▎ raise TypeError("Cannot pass both ...")
▎
▎ If I default use_attributes to True, I lose the ability to tell apart init(use_legacy_attributes=False) from init(use_attributes=True, use_legacy_attributes=False) — both look the same inside the method body, and the both-passed TypeError becomes impossible to detect cleanly.
▎
▎ The alternatives I considered:
▎ 1. Module-level _UNSET = object() sentinel — what I originally had; Doron flagged it as overengineering (and it triggered E402 in 8 of 9 files).
▎ 2. **kwargs to absorb the deprecated alias — kills IDE autocomplete + discoverability of use_legacy_attributes.
▎ 3. Lie in the annotation (bool = True but actually default to a sentinel) — breaks inspect.signature and mypy.
▎
▎ So Optional[bool] = None ended up being the least-bad option: it's a known Python idiom for "unset", costs no extra symbols, and matches what Optional semantically means here ("either a bool, or the absence of one"). Once we remove the use_legacy_attributes alias in the next major, this can
▎ collapse to use_attributes: bool = True.
▎
▎ Happy to add an inline code comment explaining this if you think it'd help future readers.
There was a problem hiding this comment.
I get the argument. But still I think you are aming to the final situation after the deprecation that param looks like:
use_attributes: bool = True
If this is the case then the logic should be to check if the legacy is defined, warn about it and deal with it..
Your choice
dvirski
force-pushed
the
dr/feat(sdk)-expose-use_legacy_attributes-via-Traceloop.init()
branch
from
4f9b300 to
3da01e5
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/traceloop-sdk/tests/test_sdk_initialization.py`:
- Around line 249-259: The test
test_use_legacy_attributes_false_propagates_to_instrumentors should ensure
TracerWrapper singleton is reset before calling Traceloop.init to avoid
order-dependent flakes; wrap the initialization/assertions using the existing
isolated_tracer_wrapper fixture (or explicitly reset TracerWrapper.instance) so
Traceloop.init executes its init path, then call
Traceloop.init(exporter=InMemorySpanExporter(), disable_batch=True) and assert
OpenAIConfig.use_legacy_attributes and AnthropicConfig.use_legacy_attributes are
as expected.
🪄 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: 4e9e9601-d694-495b-9ea3-baf290f8bce8
⛔ Files ignored due to path filters (2)
packages/opentelemetry-instrumentation-langchain/uv.lockis excluded by!**/*.lockpackages/traceloop-sdk/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.pypackages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/__init__.pypackages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.pypackages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.pypackages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/__init__.pypackages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/__init__.pypackages/opentelemetry-instrumentation-together/opentelemetry/instrumentation/together/__init__.pypackages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.pypackages/traceloop-sdk/tests/test_sdk_initialization.pypackages/traceloop-sdk/traceloop/sdk/__init__.pypackages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/init.py
- packages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/init.py
- packages/traceloop-sdk/traceloop/sdk/init.py
- packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/init.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/init.py
- packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/init.py
- packages/opentelemetry-instrumentation-together/opentelemetry/instrumentation/together/init.py
- packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/init.py
- packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/init.py
- packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
| def test_use_legacy_attributes_false_propagates_to_instrumentors(): | ||
| """use_legacy_attributes=False passed to Traceloop.init() must reach each | ||
| instrumentor's Config — otherwise users have no way to opt into the new | ||
| event-based format through the SDK.""" | ||
| from opentelemetry.instrumentation.openai.shared.config import Config as OpenAIConfig | ||
| from opentelemetry.instrumentation.anthropic.config import Config as AnthropicConfig | ||
|
|
||
| Traceloop.init(exporter=InMemorySpanExporter(), disable_batch=True) | ||
|
|
||
| assert OpenAIConfig.use_legacy_attributes is True | ||
| assert AnthropicConfig.use_legacy_attributes is True |
There was a problem hiding this comment.
Add singleton isolation to this init test to avoid order-dependent flakes.
This test calls Traceloop.init() but does not use isolated_tracer_wrapper, unlike the other new init-flag tests. If TracerWrapper.instance is already set by a prior test, this can silently bypass the init path and make assertions non-deterministic.
Suggested fix
-def test_use_legacy_attributes_false_propagates_to_instrumentors():
+def test_use_legacy_attributes_false_propagates_to_instrumentors(isolated_tracer_wrapper):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_use_legacy_attributes_false_propagates_to_instrumentors(): | |
| """use_legacy_attributes=False passed to Traceloop.init() must reach each | |
| instrumentor's Config — otherwise users have no way to opt into the new | |
| event-based format through the SDK.""" | |
| from opentelemetry.instrumentation.openai.shared.config import Config as OpenAIConfig | |
| from opentelemetry.instrumentation.anthropic.config import Config as AnthropicConfig | |
| Traceloop.init(exporter=InMemorySpanExporter(), disable_batch=True) | |
| assert OpenAIConfig.use_legacy_attributes is True | |
| assert AnthropicConfig.use_legacy_attributes is True | |
| def test_use_legacy_attributes_false_propagates_to_instrumentors(isolated_tracer_wrapper): | |
| """use_legacy_attributes=False passed to Traceloop.init() must reach each | |
| instrumentor's Config — otherwise users have no way to opt into the new | |
| event-based format through the SDK.""" | |
| from opentelemetry.instrumentation.openai.shared.config import Config as OpenAIConfig | |
| from opentelemetry.instrumentation.anthropic.config import Config as AnthropicConfig | |
| Traceloop.init(exporter=InMemorySpanExporter(), disable_batch=True) | |
| assert OpenAIConfig.use_legacy_attributes is True | |
| assert AnthropicConfig.use_legacy_attributes is True |
🤖 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 249 -
259, The test test_use_legacy_attributes_false_propagates_to_instrumentors
should ensure TracerWrapper singleton is reset before calling Traceloop.init to
avoid order-dependent flakes; wrap the initialization/assertions using the
existing isolated_tracer_wrapper fixture (or explicitly reset
TracerWrapper.instance) so Traceloop.init executes its init path, then call
Traceloop.init(exporter=InMemorySpanExporter(), disable_batch=True) and assert
OpenAIConfig.use_legacy_attributes and AnthropicConfig.use_legacy_attributes are
as expected.
4 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
New Features
Bug Fixes
Tests