feat(sdk): support multiple span processors #3207
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughSupport for multiple span processors was added to the Traceloop SDK. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SampleApp
participant TraceloopSDK
participant OpenTelemetry
participant OpenAI
User->>SampleApp: Run main()
SampleApp->>TraceloopSDK: Traceloop.init(processors=[...])
TraceloopSDK->>OpenTelemetry: Register multiple span processors
SampleApp->>SampleApp: create_joke() (decorated task)
SampleApp->>OpenAI: openai.chat.completions.create(...)
OpenAI-->>SampleApp: Returns joke
SampleApp->>OpenTelemetry: Spans processed by all registered processors
OpenTelemetry->>TraceloopSDK: Export span
OpenTelemetry->>ConsoleExporter: Export span
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 ffb34b0 in 1 minute and 57 seconds. Click for details.
- Reviewed
291lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft 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/tracing/tracing.py:250
- Draft comment:
Using a shared _span_processor_on_start for multiple processors results in calling all original on_start functions for every span event, potentially duplicating calls. Consider wrapping each processor’s on_start individually (e.g., via a closure capturing its original on_start). - Reason this comment was not posted:
Comment was on unchanged code.
2. packages/sample-app/sample_app/multiple_span_proccesors.py:1
- Draft comment:
Typo alert: The filename 'multiple_span_proccesors.py' appears to have a misspelling. Consider renaming it to 'multiple_span_processors.py' for consistency and clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% According to the rules, we should not comment on file renames or anything that would be obviously caught by the build. While this is a new file with a typo, fixing typos in filenames could cause import issues that would be caught by the build system. Additionally, renaming files is generally a trivial change that doesn't affect functionality. The typo could cause confusion for other developers reading the code. It might be worth fixing for maintainability and professionalism. While the typo is real, fixing it would be a trivial change that would be better caught by code review standards or linting tools, not as a PR comment. Delete the comment as it violates the rule about not commenting on file renames or issues that would be caught by the build.
Workflow ID: wflow_RcKbWX5OOa430UQ0
You can customize 
![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
🧹 Nitpick comments (1)
packages/sample-app/sample_app/multiple_span_proccesors.py (1)
1-37: Fix filename typo.The filename contains a typo: "proccesors" should be "processors". This affects consistency and may cause confusion.
Consider renaming the file from
multiple_span_proccesors.pytomultiple_span_processors.py.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/sample-app/sample_app/multiple_span_proccesors.py(1 hunks)packages/traceloop-sdk/traceloop/sdk/__init__.py(4 hunks)packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (4)
packages/traceloop-sdk/traceloop/sdk/telemetry.py (2)
Telemetry(12-94)capture(66-73)packages/traceloop-sdk/traceloop/sdk/__init__.py (1)
get_default_span_processor(213-253)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
force_flush(60-61)packages/traceloop-sdk/traceloop/sdk/utils/__init__.py (1)
is_notebook(17-26)
packages/traceloop-sdk/traceloop/sdk/__init__.py (2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
get_default_span_processor(370-396)packages/traceloop-sdk/tests/conftest.py (1)
exporter(18-26)
🪛 Ruff (0.12.2)
packages/sample-app/sample_app/multiple_span_proccesors.py
4-4: traceloop.sdk.tracing.context_manager.get_tracer imported but unused
Remove unused import: traceloop.sdk.tracing.context_manager.get_tracer
(F401)
🪛 Flake8 (7.2.0)
packages/sample-app/sample_app/multiple_span_proccesors.py
[error] 4-4: 'traceloop.sdk.tracing.context_manager.get_tracer' imported but unused
(F401)
🔇 Additional comments (7)
packages/sample-app/sample_app/multiple_span_proccesors.py (1)
12-15: LGTM! Good demonstration of the new feature.The code correctly demonstrates how to use multiple span processors by:
- Getting the default Traceloop processor with batching disabled
- Creating a console processor for debugging
- Initializing Traceloop with both processors
This is an excellent example of the new functionality.
packages/traceloop-sdk/traceloop/sdk/__init__.py (2)
63-63: LGTM! Well-designed API extension.The addition of the
processorsparameter provides a clean way to support multiple span processors while maintaining backward compatibility with the existingprocessorparameter.
212-254: Excellent implementation with comprehensive documentation.The
get_default_span_processormethod is well-designed with:
- Clear parameter documentation
- Proper handling of environment variables
- Helpful usage examples
- Consistent API patterns with the rest of the SDK
The method correctly delegates to the implementation in
tracing.pywhile handling the SDK-level concerns like API key resolution.packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (4)
90-93: LGTM! Proper parameter validation.The mutual exclusivity check between
processorandprocessorsparameters is essential to prevent configuration errors and provides clear error messaging.
370-397: LGTM! Well-designed helper function.The
get_default_span_processorfunction properly encapsulates the processor creation logic with:
- Clear parameter handling
- Proper fallback to TracerWrapper static attributes
- Correct processor type selection based on environment
- Good documentation
292-298: LGTM! Proper flush handling for multiple processors.The flush method correctly handles both single and multiple processor scenarios using proper attribute checking.
250-257: LGTM! Flexible on_start method handling.The logic properly handles both single callable and list of callables for the original
on_startmethods, providing good flexibility for the multiple processors feature.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed acddebc in 47 seconds. Click for details.
- Reviewed
24lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft 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/sample-app/sample_app/multiple_span_proccesors.py:4
- Draft comment:
Removed the unused import 'get_tracer'. Consider also renaming the file to fix the 'proccesors' typo. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:149
- Draft comment:
Initialize '__spans_processor_original_on_start' to None for the default processor branch. Consider adding a short inline comment for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_bbijP5aMh4VKTJFT
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3664304 in 1 minute and 16 seconds. Click for details.
- Reviewed
181lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft 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/sample-app/sample_app/multiple_span_processors.py:11
- Draft comment:
Good rename and cleanup; no issues observed in this sample app file. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/traceloop-sdk/traceloop/sdk/__init__.py:244
- Draft comment:
Switching to explicit None checks (e.g. 'if headers is None') is clearer. Note that passing an empty dict {} will bypass setting the Authorization header – confirm that this behavior is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:98
- Draft comment:
In the multiple processors branch, only processors with the '_traceloop_processor' attribute get their on_start overridden. Verify that this selective behavior (vs. replacing on_start for all processors) is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:388
- Draft comment:
Setting the '_traceloop_processor' attribute on the default processor helps detect it later. Ensure that custom processors intended for special handling are created with this attribute if needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is informative and suggests ensuring a certain behavior, which violates the rules. It doesn't provide a specific code suggestion or ask for a specific test to be written.
Workflow ID: wflow_Dj1l0hrVyn6fD9Yk
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
94-107: Improve processor identification and fix on_start method override issue.The current implementation has two issues:
- The processor identification logic is fragile and may incorrectly identify custom processors as Traceloop processors
- The
on_startmethod is only overridden for processors with_traceloop_processorattribute, but the original methods are stored for all processors# Handle multiple processors case if processors is not None: obj.__spans_processors = [] obj.__spans_processor_original_on_start = [] for proc in processors: obj.__spans_processors.append(proc) obj.__spans_processor_original_on_start.append(proc.on_start) - if hasattr(proc, "_traceloop_processor"): - proc.on_start = obj._span_processor_on_start + # Override on_start for Traceloop processors only + if hasattr(proc, "_traceloop_processor") and getattr(proc, "_traceloop_processor", False): + proc.on_start = obj._span_processor_on_start obj.__tracer_provider.add_span_processor(proc)This ensures that only verified Traceloop processors get the special
on_starttreatment while maintaining proper original method storage.
🧹 Nitpick comments (2)
packages/sample-app/sample_app/multiple_span_processors.py (1)
17-26: Consider adding error handling for the OpenAI API call.The function lacks error handling for potential API failures, which could cause the application to crash in production scenarios.
@task(name="joke_creation", version=1) def create_joke(): + try: completion = client.chat.completions.create( model="gpt-3.5-turbo", messages=[{"role": "user", "content": "Tell me a joke about opentelemetry multiple span processors"}], ) result = completion.choices[0].message.content print(result) return result + except Exception as e: + print(f"Error creating joke: {e}") + return "Sorry, couldn't generate a joke right now!"packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
361-391: Address static analysis hint and improve function design.The function is well-designed and documented, but there's a static analysis warning about using
setattrwith a constant value.- setattr(processor, "_traceloop_processor", True) + processor._traceloop_processor = TrueThe direct attribute assignment is cleaner and addresses the static analysis hint while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/sample-app/sample_app/multiple_span_processors.py(1 hunks)packages/traceloop-sdk/traceloop/sdk/__init__.py(4 hunks)packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/traceloop-sdk/traceloop/sdk/init.py
🧰 Additional context used
🪛 Ruff (0.12.2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
389-389: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🔇 Additional comments (5)
packages/sample-app/sample_app/multiple_span_processors.py (1)
11-14: LGTM! Excellent demonstration of the new multi-processor feature.The code effectively demonstrates the new multiple span processors capability by combining:
- A Traceloop default processor with batching disabled for immediate processing
- A console processor for local debugging output
This showcases the flexibility of the enhanced SDK.
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (4)
90-92: LGTM! Proper parameter validation.The mutual exclusivity check between
processorandprocessorsparameters is well-implemented and provides clear error messaging.
135-138: LGTM! Clean refactoring to use the new helper function.The refactoring to use
get_default_span_processor()improves code maintainability and provides better separation of concerns.
241-247: LGTM! Robust handling of multiple original on_start methods.The implementation correctly handles both single callable and list of callables for the original
on_startmethods, ensuring backward compatibility while supporting the new multi-processor feature.
283-288: LGTM! Proper flush handling for both single and multiple processors.The flush method correctly handles both the legacy single processor case and the new multiple processors case using appropriate attribute checks.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 28e2226 in 1 minute and 57 seconds. Click for details.
- Reviewed
251lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3draft 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/tests/conftest.py:161
- Draft comment:
Potential NameError: initialize _trace_wrapper_instance (e.g. _trace_wrapper_instance = None) before checking if TracerWrapper.instance exists, so that the final restoration block does not crash if the instance wasn’t set. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment suggests a theoretical issue, but examining the code more carefully: 1) _trace_wrapper_instance is only used in the restoration block if it was previously set in the hasattr block 2) This pattern is used successfully in multiple other fixtures in the same file 3) If hasattr() is false, _trace_wrapper_instance never gets referenced 4) The code follows a common Python pattern for singleton management in tests I could be overlooking an edge case where the variable somehow gets referenced without being set. The pattern appears in multiple places which could indicate it's problematic. The code structure ensures _trace_wrapper_instance is only referenced after it's defined. The multiple occurrences actually demonstrate it's a proven pattern rather than a problem. The comment should be deleted as it raises a theoretical issue that isn't actually possible given the code structure, and the pattern is proven to work throughout the codebase.
2. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:98
- Draft comment:
Good use of default arguments in the chained_on_start closure to capture the original on_start. This pattern avoids the common loop variable capture issue. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/traceloop-sdk/tests/test_sdk_initialization.py:148
- Draft comment:
The tests validate multiple span processors and processor parameter conflict well. Ensure that the singleton restoration logic (used in multiple fixtures) is consistent across tests to avoid unexpected state issues. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
Workflow ID: wflow_xUMTYwyFkw2H2XTW
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
130-146: Solid test for default span processor creation.The test properly verifies processor types and the marker attribute for both batch configurations.
Consider replacing
getattrwith direct attribute access for better readability:- assert getattr(processor, "_traceloop_processor") is True + assert processor._traceloop_processor is TrueApply the same change on line 145.
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
362-391: Well-designed helper function with comprehensive configuration.The function provides excellent flexibility and documentation. The marker attribute helps with processor identification.
Consider replacing
setattrwith direct attribute assignment for better readability:- setattr(processor, "_traceloop_processor", True) + processor._traceloop_processor = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/sample-app/sample_app/multiple_span_processors.py(1 hunks)packages/traceloop-sdk/tests/conftest.py(1 hunks)packages/traceloop-sdk/tests/test_sdk_initialization.py(1 hunks)packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sample-app/sample_app/multiple_span_processors.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (4)
packages/traceloop-sdk/traceloop/sdk/telemetry.py (2)
Telemetry(12-94)capture(66-73)packages/traceloop-sdk/traceloop/sdk/__init__.py (1)
get_default_span_processor(213-253)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
force_flush(60-61)packages/traceloop-sdk/traceloop/sdk/utils/__init__.py (1)
is_notebook(17-26)
🪛 Ruff (0.12.2)
packages/traceloop-sdk/tests/test_sdk_initialization.py
139-139: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
145-145: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
390-390: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (7)
packages/traceloop-sdk/tests/conftest.py (1)
160-217: Well-structured test fixture for multiple span processors.The fixture properly demonstrates the new multi-processor functionality with three distinct processors, each with isolated exporters. The singleton management follows existing patterns, and the custom processors provide meaningful test scenarios for attribute setting and span counting.
packages/traceloop-sdk/tests/test_sdk_initialization.py (2)
73-128: Comprehensive test for multiple span processor functionality.The test effectively verifies that all processors receive spans, add their specific attributes, and execute in the correct order. The span counting verification is particularly valuable for ensuring proper processor chaining.
148-175: Essential validation test for parameter mutual exclusivity.This test ensures users receive clear error messages when attempting to use both
processorandprocessorsparameters simultaneously. The singleton cleanup is properly handled.packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (4)
70-71: Clean parameter addition and validation.The new
processorsparameter and mutual exclusivity validation provide a clear API contract with helpful error messages.Also applies to: 90-92
94-111: Well-implemented multiple processor support.The closure-based approach for chaining
on_startmethods is clean and correctly captures the original methods. Each processor is properly registered with the tracer provider.
113-125: Excellent backward compatibility and code reuse.The single processor handling maintains full backward compatibility while using consistent chaining logic. The refactoring to use
get_default_span_processorpromotes code reuse and maintainability.Also applies to: 145-163
284-289: Proper flush implementation for both single and multiple processors.The method correctly handles both scenarios using appropriate attribute checks and ensures all processors are flushed.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3bb8969 in 2 minutes and 0 seconds. Click for details.
- Reviewed
114lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft 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/tracing/tracing.py:107
- Draft comment:
When a processor has the _traceloop_processor attribute, its on_start is overridden with obj._span_processor_on_start, which bypasses the original chained callback. Ensure this behavior is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:125
- Draft comment:
Similarly, in the single processor branch the on_start chaining is replaced when _traceloop_processor is present. Confirm that skipping the original on_start call is the intended design. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:358
- Draft comment:
The new default_span_processor_on_start function largely duplicates the logic in _span_processor_on_start. Consider refactoring common logic to reduce duplication. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:358
- Draft comment:
Consider adding a docstring to default_span_processor_on_start to clarify its purpose and differences from the instance method _span_processor_on_start. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:368
- Draft comment:
default_span_processor_on_start converts attribute values to strings (e.g. using str(workflow_name)) while _span_processor_on_start does not. Verify that this conversion is intended and that the behavior is consistent. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment points out a real inconsistency in string conversion between two similar functions. However, the comment asks to "verify" the behavior rather than making a clear recommendation. According to the rules, we should not ask the PR author to verify or confirm things. The comment should either definitively say this is a bug that needs fixing, or be removed. I may be too focused on the technical correctness of the observation and not enough on whether this comment is actionable and clear. Also, I don't know if this string conversion difference actually matters in practice. While the inconsistency is real, the comment as written violates the rule about not asking authors to verify things. A better comment would either definitively state this needs to be fixed or explain why the inconsistency is problematic. The comment should be deleted because it asks for verification rather than making a clear recommendation. If the string conversion inconsistency is actually problematic, a new comment should be written that clearly states the issue and proposed fix.
Workflow ID: wflow_Q3rE9Ld2MiC0vLgL
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
152-169: Inconsistent processor handling for default case.The default processor path directly assigns
_span_processor_on_startwithout using the chaining logic used in other paths, creating inconsistency.For consistency, the default processor should use the same chaining approach:
- obj.__spans_processor.on_start = obj._span_processor_on_start + obj.__spans_processor.on_start = obj._create_chained_on_start( + obj.__spans_processor.on_start, True # Default processor is always Traceloop + )This ensures all processor paths follow the same pattern and makes the code more maintainable.
🧹 Nitpick comments (3)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (3)
96-116: LGTM! Multiple processor logic addresses past review concerns.The implementation correctly addresses the previous issue with on_start method overrides by:
- Capturing the original method before any modifications
- Creating proper chaining logic that calls both the wrapper and original methods
- Using the
_traceloop_processormarker for special handlingConsider extracting the on_start chaining logic into a helper method to reduce duplication between single and multiple processor paths:
+ def _create_chained_on_start(self, original_on_start, is_traceloop_processor=False): + if is_traceloop_processor: + return self._span_processor_on_start + + def chained_on_start(span, parent_context=None, orig=original_on_start): + self._span_processor_on_start(span, parent_context) + if orig: + orig(span, parent_context) + return chained_on_start
367-423: Code duplication between standalone and method versions.The
default_span_processor_on_startfunction duplicates most logic from the_span_processor_on_startmethod, with added type safety checks.Consider extracting the common logic into a shared helper function that both can use:
+def _process_span_attributes(span, parent_context=None): + """Common span attribute processing logic.""" + workflow_name = get_value("workflow_name") + if workflow_name is not None: + span.set_attribute(SpanAttributes.TRACELOOP_WORKFLOW_NAME, str(workflow_name)) + # ... rest of common logic + def default_span_processor_on_start(span: Span, parent_context: Context | None = None): - workflow_name = get_value("workflow_name") - # ... duplicate logic + _process_span_attributes(span, parent_context)
425-455: Well-implemented function with comprehensive documentation.The
get_default_span_processorfunction is well-documented and properly handles processor creation with appropriate configuration.Regarding the static analysis hint on Line 453: The
setattrusage is intentional for dynamically marking processors with the_traceloop_processorattribute. This is not a safety issue but a design pattern for processor identification.The static analysis hint can be safely ignored, but if preferred, you could use direct assignment:
- setattr(processor, "_traceloop_processor", True) + processor._traceloop_processor = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
453-453: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (4)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (4)
24-26: LGTM! Import additions support new functionality.The new imports for
Context,Span, and type annotations (Callable,List,Optional) are appropriately added to support the multiple span processor feature.Also applies to: 37-37
72-73: LGTM! Parameter design supports backward compatibility.The addition of the
processorsparameter alongside the existingprocessorparameter maintains backward compatibility while enabling multiple processor support.
92-94: LGTM! Clear parameter validation prevents ambiguous usage.The mutual exclusivity validation between
processorandprocessorsparameters is well-implemented with a clear error message.
291-295: LGTM! Flush method correctly handles both single and multiple processors.The implementation properly checks for both processor configurations and ensures all processors are flushed appropriately.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 8551147 in 1 minute and 3 seconds. Click for details.
- Reviewed
17lines of code in1files - 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/tracing/tracing.py:368
- Draft comment:
Nice improvement adding a proper docstring. Consider expanding it with brief parameter descriptions (e.g., forspanandparent_context) to further clarify their roles in standalone usage. - 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% While adding parameter descriptions could be helpful, this seems like a minor documentation suggestion rather than a critical code issue. The parameters are fairly self-explanatory from their types (Span and Context). The function's purpose is already clearly stated. The comment doesn't point out any actual problems with the code. The parameters might not be as self-explanatory as I assume - someone unfamiliar with OpenTelemetry might not know what a Span or Context represents. However, this is an internal SDK function and anyone using it would need to understand OpenTelemetry concepts. The docstring already explains the key difference from the original method. The comment should be deleted as it suggests a minor documentation enhancement rather than pointing out a substantive code issue that needs to be fixed.
Workflow ID: wflow_wrT9Dp0hWNI4SD4E
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
118-132: Same closure issue and code duplicationThe single processor logic has the same closure issue as the multiple processor case, and as noted in previous reviews, duplicates the chaining logic.
Apply the same closure fix as suggested for the multiple processor case:
- original_on_start = obj.__spans_processor.on_start - - def chained_on_start(span, parent_context=None, orig=original_on_start): - obj._span_processor_on_start(span, parent_context) - if orig: - orig(span, parent_context) - - obj.__spans_processor.on_start = chained_on_start + original_on_start = obj.__spans_processor.on_start + + def create_chained_on_start(orig): + def chained_on_start(span, parent_context=None): + obj._span_processor_on_start(span, parent_context) + if orig: + orig(span, parent_context) + return chained_on_start + + obj.__spans_processor.on_start = create_chained_on_start(original_on_start)
🧹 Nitpick comments (2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (2)
368-426: Code duplication concern with defensive improvementsThis function duplicates logic from
_span_processor_on_startbut adds defensivestr()conversions and type checking. While the standalone nature serves a specific purpose, consider extracting common logic to reduce maintenance overhead.The defensive improvements (str() conversions and type checking) are valuable and could be back-ported to the instance method for consistency.
428-459: Well-designed helper function with minor style improvementThe function is well-structured with proper documentation and logic. However, the static analysis tool correctly suggests replacing
setattrwith direct assignment for better readability.Apply this minor style improvement:
- setattr(processor, "_traceloop_processor", True) + processor._traceloop_processor = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (5)
packages/traceloop-sdk/tests/conftest.py (1)
exporter(18-26)packages/traceloop-sdk/traceloop/sdk/telemetry.py (2)
Telemetry(12-94)capture(66-73)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
force_flush(60-61)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-257)packages/traceloop-sdk/traceloop/sdk/utils/__init__.py (1)
is_notebook(17-26)
🪛 Ruff (0.12.2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
456-456: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (6)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (6)
24-25: LGTM - Import additions support new functionalityThe new imports for
ContextandListare necessary to support the multiple span processors feature and proper type annotations.Also applies to: 37-37
72-73: LGTM - Constructor parameters properly designedThe addition of the optional
processorsparameter alongside the existingprocessorparameter maintains backward compatibility while enabling multiple processor support.
92-94: LGTM - Proper parameter validationThe mutual exclusivity validation between
processorandprocessorsparameters prevents configuration conflicts and provides a clear error message.
152-155: LGTM - Proper use of default processor helperThe refactoring to use
get_default_span_processor()improves code organization and the direct assignment ofon_startis appropriate for the default Traceloop processor.Also applies to: 168-169
290-295: LGTM - Flush method properly handles both scenariosThe flush method correctly handles both single and multiple processor cases using appropriate attribute checking and iteration.
96-115: Critical closure bug in multiple processor handlingThe
chained_on_startfunction closure captures the loop variableoriginal_on_start, causing all processors to use the same (final)on_startmethod reference.Fix the closure issue by capturing the variable properly:
- if processors is not None: - obj.__spans_processors = [] - for proc in processors: - original_on_start = proc.on_start - - def chained_on_start(span, parent_context=None, orig=original_on_start): - obj._span_processor_on_start(span, parent_context) - if orig: - orig(span, parent_context) - - proc.on_start = chained_on_start - if hasattr(proc, "_traceloop_processor"): - proc.on_start = obj._span_processor_on_start - - obj.__spans_processors.append(proc) - - obj.__tracer_provider.add_span_processor(proc) + if processors is not None: + obj.__spans_processors = [] + for proc in processors: + original_on_start = proc.on_start + + def create_chained_on_start(orig): + def chained_on_start(span, parent_context=None): + obj._span_processor_on_start(span, parent_context) + if orig: + orig(span, parent_context) + return chained_on_start + + proc.on_start = create_chained_on_start(original_on_start) + if hasattr(proc, "_traceloop_processor"): + proc.on_start = obj._span_processor_on_start + + obj.__spans_processors.append(proc) + + obj.__tracer_provider.add_span_processor(proc)Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed ac436a3 in 1 minute and 40 seconds. Click for details.
- Reviewed
42lines of code in1files - 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/tracing/tracing.py:122
- Draft comment:
Closure Issue: The variable 'is_traceloop_processor' is used inside the chained_on_start function without being captured as a default argument, risking late binding problems. Consider passing it as a default parameter. - 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% Late binding in Python closures can be a real issue when the outer variable might change. However, in this case,is_traceloop_processoris only set once when initializing the processor and never changes. The function is immediately assigned toon_startand the outer scope is not retained. The same pattern is used in two places in the diff with identical structure. I could be wrong about the lifecycle of these variables - perhaps there are edge cases where the processor initialization happens multiple times or the outer scope is retained longer than I think. While late binding can be an issue in Python, this is a common pattern for processor initialization where the closed-over variable is effectively constant. Making it a default parameter would add complexity without clear benefit. The comment raises a theoretically valid concern but in this specific case the late binding is not problematic since the variable is effectively constant. The comment should be removed.
Workflow ID: wflow_FQCY0YUz8KIeCfVw
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
366-424: Code duplication with _span_processor_on_start method.The
default_span_processor_on_startfunction duplicates most of the logic from_span_processor_on_start, creating maintenance overhead. The functions are nearly identical except for some explicitstr()conversions and type checks.This is the same code duplication issue identified in past reviews. The duplication creates maintenance overhead as changes need to be applied in two places.
Consider extracting the common logic into a shared helper function:
+def _set_span_attributes_from_context(span: Span) -> None: + """Common logic for setting span attributes from context values.""" + workflow_name = get_value("workflow_name") + if workflow_name is not None: + span.set_attribute(SpanAttributes.TRACELOOP_WORKFLOW_NAME, str(workflow_name)) + + entity_path = get_value("entity_path") + if entity_path is not None: + span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_PATH, str(entity_path)) + + # ... rest of the common logic + def default_span_processor_on_start(span: Span, parent_context: Context | None = None): - # ... duplicated logic + _set_span_attributes_from_context(span)
🧹 Nitpick comments (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
426-457: Helper function design is solid but has minor style issue.The function provides good modularity and proper documentation. However, the use of
setattrwith a constant string is flagged by static analysis as unnecessary.Replace the
setattrcall with direct attribute assignment as suggested by the linter:- setattr(processor, "_traceloop_processor", True) + processor._traceloop_processor = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (5)
packages/traceloop-sdk/traceloop/sdk/telemetry.py (2)
Telemetry(12-94)capture(66-73)packages/traceloop-sdk/traceloop/sdk/__init__.py (1)
get_default_span_processor(213-253)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
force_flush(60-61)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-257)packages/traceloop-sdk/traceloop/sdk/utils/__init__.py (1)
is_notebook(17-26)
🪛 Ruff (0.12.2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
105-105: Function definition does not bind loop variable is_traceloop_processor
(B023)
454-454: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (6)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (6)
24-26: New imports look appropriate for the multiple processors feature.The additions of
ContextandSpantypes support the new standalone function, while these imports are correctly placed and necessary for the implementation.
37-37: Import addition for List type is correct.The
Listtype is properly imported to support the newprocessorsparameter typing.
72-94: Good parameter validation for mutual exclusivity.The validation correctly prevents both
processorandprocessorsfrom being specified simultaneously, which prevents ambiguous configuration.
150-167: Refactoring to use helper function improves modularity.The extraction of default processor creation into
get_default_span_processoris a good design improvement that enables reusability.
289-294: Flush method correctly handles both single and multiple processors.The implementation properly checks for the existence of either single or multiple processors and flushes them accordingly.
115-130: Same closure bug exists in single processor case.The same variable capture issue exists here as well, though it's less critical since there's only one iteration.
Apply the same fix pattern:
# Handle single processor case (backward compatibility) elif processor is not None: Telemetry().capture("tracer:init", {"processor": "custom"}) obj.__spans_processor: SpanProcessor = processor original_on_start = obj.__spans_processor.on_start is_traceloop_processor = hasattr(obj.__spans_processor, "_traceloop_processor") - def chained_on_start(span, parent_context=None, orig=original_on_start): + def chained_on_start(span, parent_context=None, orig=original_on_start, is_traceloop=is_traceloop_processor): obj._span_processor_on_start(span, parent_context) - if orig and not is_traceloop_processor: + if orig and not is_traceloop: orig(span, parent_context) obj.__spans_processor.on_start = chained_on_startLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2144fb4 in 37 seconds. Click for details.
- Reviewed
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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/tracing/tracing.py:103
- Draft comment:
Good use of default argument capture for 'is_traceloop'. Consider renaming the parameter to 'is_traceloop_processor' for clearer consistency with the outer variable. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:122
- Draft comment:
Same pattern here: capturing 'is_traceloop_processor' as 'is_traceloop'. For clarity, consider aligning the naming with the outer variable. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:109
- Draft comment:
Typo Alert: The attribute name__spans_processorson line 109 appears inconsistent with the naming used elsewhere (e.g.,obj._span_processor_on_start). Consider renaming it to__span_processorsfor clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_L4e2BAW2IR0TtvsX
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (3)
115-129: LGTM! Single processor handling is correct.The single processor logic properly handles the chained
on_startmethod with correct closure capture. The implementation maintains backward compatibility while integrating with the new multiple processor architecture.For future maintenance, consider extracting the common chaining logic into a helper method to reduce duplication between single and multiple processor cases.
366-424: LGTM! Standalone on_start function implemented correctly.The
default_span_processor_on_startfunction properly replicates the span attribute enrichment logic for standalone usage. The explicitstr()conversions and type checking add robustness.Consider extracting the common span attribute logic into a shared helper function to reduce duplication between this function and
_span_processor_on_start, improving maintainability.
454-454: Use direct attribute assignment instead of setattr.The static analysis tool correctly identifies that
setattrwith a constant attribute name is unnecessary.- setattr(processor, "_traceloop_processor", True) + processor._traceloop_processor = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (6)
packages/traceloop-sdk/tests/conftest.py (4)
on_start(60-61)on_start(168-170)on_start(177-180)exporter(18-26)packages/traceloop-sdk/traceloop/sdk/telemetry.py (2)
Telemetry(12-94)capture(66-73)packages/traceloop-sdk/traceloop/sdk/__init__.py (1)
get_default_span_processor(213-253)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
force_flush(60-61)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-257)packages/traceloop-sdk/traceloop/sdk/utils/__init__.py (1)
is_notebook(17-26)
🪛 Ruff (0.12.2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
454-454: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (6)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (6)
24-26: LGTM! Import changes support new functionality.The new imports for
Context,Span, andListare all properly used in the implementation to support multiple span processors and the new helper functions.Also applies to: 37-37
72-73: LGTM! Proper parameter validation.The mutual exclusivity validation between
processorandprocessorsparameters prevents configuration errors and provides a clear error message for invalid usage.Also applies to: 92-94
96-113: LGTM! Closure bug has been fixed.The multiple processor handling logic is now correct. The closure bug identified in previous reviews has been properly addressed by capturing
is_traceloop_processoras a default parameteris_traceloop=is_traceloop_processorin thechained_on_startfunction, preventing all processors from sharing the last loop value.
426-457: LGTM! Well-designed helper function.The
get_default_span_processorfunction is well-implemented with proper parameter handling, fallback logic, and processor configuration. The docstring clearly explains the usage and parameters.
289-293: LGTM! Flush method correctly handles both processor scenarios.The updated flush logic properly handles both single and multiple processor cases using appropriate attribute checks and calls
force_flush()on all relevant processors.
150-167: Verify on_start assignment necessityThe use of
get_default_span_processoris correct, but the helper already doesprocessor.on_start = default_span_processor_on_startso immediately overwriting it with
obj.__spans_processor.on_start = obj._span_processor_on_startmay be redundant. Please confirm:
- Does
obj._span_processor_on_startprovide additional or different behavior compared todefault_span_processor_on_start?- If it’s identical, consider removing one of the assignments for clarity and consistency.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6df8242 in 1 minute and 53 seconds. Click for details.
- Reviewed
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft 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/tracing/tracing.py:102
- Draft comment:
In the multiple processors block, the chained_on_start now calls the original on_start before _span_processor_on_start (reversing the previous order) and omits the 'not is_traceloop' check. Confirm that this reordering and removal of the guard is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:121
- Draft comment:
In the single processor block, the 'not is_traceloop' condition has been removed while retaining the original call order (_span_processor_on_start before original). This behavior contrasts with the multiple processors block. Verify that both ordering and condition removal are as intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_jG3xeukDlPOo4FH4
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 360c57e in 1 minute and 48 seconds. Click for details.
- Reviewed
97lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft 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/tests/test_workflows.py:52
- Draft comment:
Changed the expected type for the prompt version from an integer (5) to a string ("5"). Ensure this type conversion is intentional and consistent across the SDK usage. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:102
- Draft comment:
In the multiple processors branch, the chained on_start calls the original on_start before calling _span_processor_on_start, whereas in the single processor branch the order is reversed. Consider unifying the order unless this difference is intentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:190
- Draft comment:
Refactored _span_processor_on_start now delegates LLM-related attribute setting to default_span_processor_on_start. Confirm that handling association properties separately (after the default call) meets all intended use cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_fQfYJe9BadBWDXwC
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
315-373: Code duplication for reusability.The
default_span_processor_on_startfunction duplicates span attribute enrichment logic from_span_processor_on_startto enable standalone usage. While this creates maintenance overhead, it's a reasonable trade-off for the reusability requirements.Consider documenting that these functions should be kept in sync when making changes to span attribute logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/traceloop-sdk/tests/test_workflows.py(2 hunks)packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/traceloop-sdk/tests/test_workflows.py
🧰 Additional context used
🪛 Ruff (0.12.2)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
403-403: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (9)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (9)
24-26: LGTM!The new imports are correctly placed and necessary for the multiple span processors functionality.
Also applies to: 37-37
72-73: LGTM!The parameter type hints are correctly specified and follow the existing pattern.
92-94: LGTM!Good defensive programming to prevent users from specifying both parameters simultaneously. The error message is clear and helpful.
96-112: LGTM!The multiple processors handling correctly captures the original
on_startmethod as a default parameter, avoiding closure bugs. The chaining logic properly calls the original processor first, then the wrapper's method.
114-127: LGTM!The single processor chaining logic is consistent with the multiple processor approach and properly handles closures by capturing the original method as a default parameter.
148-151: LGTM!Good refactoring to use the
get_default_span_processorhelper function for code reuse. The direct assignment ofon_startis appropriate for the default processor case.Also applies to: 164-165
237-242: LGTM!The flush method correctly handles both single and multiple processor cases using appropriate attribute checks.
191-191: LGTM!Good refactoring to delegate common span attribute enrichment to the standalone function.
375-405: LGTM!The
get_default_span_processorhelper function properly centralizes default processor creation logic and correctly marks the processor with the_traceloop_processorattribute for identification.Note: The static analysis hint about
setattris a false positive - usingsetattrto add a marker attribute is an appropriate pattern here.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5ba3d5a in 1 minute and 26 seconds. Click for details.
- Reviewed
89lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft 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/tracing/tracing.py:92
- Draft comment:
The chaining of on_start is inconsistent: in the multiple processor branch (lines 92–105) the chained function calls the original on_start before _span_processor_on_start, but in the single processor branch (lines 115–119) the order is reversed. Please confirm if this ordering is intentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. packages/traceloop-sdk/traceloop/sdk/__init__.py:238
- Draft comment:
The docstring example still refers to the 'processors' parameter. Since the API now uses 'processor' (which can be a single processor or a list), please update the example and documentation to reflect the new parameter name. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:107
- Draft comment:
It looks like there might be a typo here: you're capturing the count withlen(processor)when the context suggests it should be a collection (as indicated by the key "processor": "multiple"). Should this belen(processors)instead oflen(processor)? - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_WUSba6Eirm48dahR
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4b5bd9f in 1 minute and 49 seconds. Click for details.
- Reviewed
73lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft 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/sample-app/sample_app/multiple_span_processors.py:15
- Draft comment:
Changed parameter from 'processors' to 'processor'. Ensure docs and type hints reflect that 'processor' now accepts a list of processors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment is asking the author to verify documentation and type hints, which falls under the "do not ask author to verify/ensure" rule. It's also speculative since we don't have access to the docs or type hints. The comment isn't pointing out a clear code issue that needs to be fixed. Maybe this is actually a breaking API change that needs documentation updates? Maybe the type hints really are wrong now? Even if docs/type hints need updates, this comment is still asking for verification rather than pointing out a specific issue. We don't have enough context to know if there's really a problem. Delete this comment as it violates the rule about not asking authors to verify/ensure things, and we don't have strong evidence of an actual issue.
2. packages/traceloop-sdk/tests/conftest.py:200
- Draft comment:
Updated init call to use 'processor' instead of 'processors'. Verify that this change is reflected consistently in all documentation and usage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that the change is reflected consistently in all documentation and usage. This falls under the rule of not asking the author to double-check things or ensure behavior is intended. Therefore, this comment should be removed.
3. packages/traceloop-sdk/tests/conftest.py:209
- Draft comment:
Changed dictionary key from 'processors' to 'processor'. Confirm that downstream tests use the updated key if needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%The comment is asking the PR author to confirm that downstream tests use the updated key, which violates the rule against asking the author to ensure that the change is tested. However, the comment does point out a specific change in the code, which is useful information. The comment could be rephrased to focus on the change itself without asking for confirmation about tests.
4. packages/traceloop-sdk/tests/test_sdk_initialization.py:143
- Draft comment:
Removal of test_processors_parameter_validation is noted. Ensure that documentation clearly states the new API (using only 'processor') and that backward compatibility concerns are addressed if needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure documentation and backward compatibility, which falls under the rule of not asking the author to ensure things. It doesn't provide a specific code suggestion or ask for a specific test to be written.
Workflow ID: wflow_inRRICbzdvPZJfH8
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
130-146: Good test for default processor creation with minor improvement needed.The test properly validates both processor types and the Traceloop marker attribute. However, the static analysis tool correctly identifies an improvement opportunity.
Replace
getattrcalls with direct attribute access since you're already verifying the attribute exists withhasattr:assert isinstance(processor, SimpleSpanProcessor) assert hasattr(processor, "_traceloop_processor") - assert getattr(processor, "_traceloop_processor") is True + assert processor._traceloop_processor is True # Test with batch enabled processor = Traceloop.get_default_span_processor(disable_batch=False) assert isinstance(processor, BatchSpanProcessor) assert hasattr(processor, "_traceloop_processor") - assert getattr(processor, "_traceloop_processor") is True + assert processor._traceloop_processor is True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/sample-app/sample_app/multiple_span_processors.py(1 hunks)packages/traceloop-sdk/tests/conftest.py(1 hunks)packages/traceloop-sdk/tests/test_sdk_initialization.py(1 hunks)packages/traceloop-sdk/traceloop/sdk/__init__.py(3 hunks)packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sample-app/sample_app/multiple_span_processors.py
- packages/traceloop-sdk/traceloop/sdk/init.py
- packages/traceloop-sdk/tests/conftest.py
🧰 Additional context used
🪛 Ruff (0.12.2)
packages/traceloop-sdk/tests/test_sdk_initialization.py
139-139: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
145-145: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
398-398: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🔇 Additional comments (7)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
73-128: Excellent comprehensive test for multiple span processors.This test thoroughly validates the multiple span processor functionality by:
- Testing that all processors receive the expected spans
- Verifying processor-specific attributes are correctly added
- Ensuring span counting works properly across processors
- Using appropriate fixtures and decorators
The test structure is clear and the assertions are comprehensive.
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (6)
91-108: Multiple processor handling looks correct.The implementation properly handles multiple processors by:
- Preserving each processor's original
on_startmethod- Creating a chained function that calls both the original and Traceloop's processing
- Adding all processors to the tracer provider
- Recording telemetry with processor count
The closure handling appears correct since
original_on_startis captured as a default parameter in the function definition.
109-123: Single processor handling maintains good backward compatibility.The implementation correctly:
- Preserves the original processor's
on_startmethod- Chains the calls appropriately (Traceloop processing first, then original)
- Maintains the same telemetry patterns
The backward compatibility approach is well-designed.
124-161: Good refactoring with default processor helper function.The use of
get_default_span_processorhelper function:
- Centralizes default processor creation logic
- Maintains existing telemetry and callback handling
- Enables reuse across the codebase
- Preserves all existing functionality
The refactoring improves maintainability without changing behavior.
232-237: Clean flush implementation for both single and multiple processors.The flush method correctly:
- Checks for both single and multiple processor scenarios using
hasattr- Calls
force_flushappropriately for each case- Maintains the existing flush behavior
The implementation is straightforward and robust.
310-368: Excellent refactoring of span processing logic.The
default_span_processor_on_startfunction:
- Extracts reusable span attribute enrichment logic
- Handles all the necessary context values (workflow, entity, prompts)
- Is well-documented with clear purpose
- Enables standalone usage without class dependencies
- Maintains all existing span processing behavior
This refactoring improves modularity and reusability.
370-401: Well-designed helper function for default processor creation.The
get_default_span_processorfunction:
- Centralizes default processor creation logic
- Properly handles configuration parameters and fallbacks
- Correctly chooses between Simple and Batch processors
- Sets up the marker attribute for processor identification
- Assigns the default span processing function
- Is well-documented with clear parameters and return types
The use of
setattrfor the marker attribute is appropriate here since we're adding a runtime attribute to an existing processor object.
feat(instrumentation): ...orfix(instrumentation): ....Important
Support for multiple span processors added to Traceloop SDK with updated initialization and new tests.
TracerWrapperintracing.py.Traceloop.init()in__init__.pynow accepts a list ofSpanProcessor.get_default_span_processor()in__init__.pyandtracing.pyto obtain default processor.test_multiple_span_processorsintest_sdk_initialization.pyto validate multiple processor handling.test_get_default_span_processorintest_sdk_initialization.pyto validate default processor creation.multiple_span_processors.pyadded to demonstrate usage with console and OpenAI processors.This description was created by
for 4b5bd9f. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Documentation
Tests