Move span methods to LaminarSpan, propagate association properties down to the context #225
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add user id and session id as params to start span for consistency with JS - Global metadata set for all traces at Laminar.initialize - Return LaminarSpan object from start trace - Expose most control methods on LaminarSpan and use them in Laminar - Add an ability to add tags
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 959a29c in 3 minutes and 14 seconds. Click for details.
- Reviewed
2561lines of code in31files - Skipped
0files when reviewing. - Skipped posting
7draft 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. tests/test_tracing.py:1
- Draft comment:
The tracing tests are very comprehensive, covering nested spans, exception events, async and synchronous contexts, and proper propagation of association properties (session_id, user_id, metadata, tags). Consider adding a few inline comments in the more complex nested scenarios (e.g. deep nesting of active spans) to further clarify intent. - 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. tests/test_utils.py:1
- Draft comment:
The utilities tests (especially for json_dumps and format_id) are thorough and cover a wide range of input types including pydantic models, dataclasses, edge cases (circular refs, failing str/repr) and mixed nested structures. They clearly validate the intended fallback behavior. Optionally, consider adding a performance test for extremely deep or large nested structures if performance is a concern. - 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. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/openhands_ai/__init__.py:10
- Draft comment:
There's a stray closing parenthesis on line 10 ():). It doesn't seem to serve any purpose. Please remove or clarify if intentional. - Reason this comment was not posted:
Comment was on unchanged code.
4. src/lmnr/opentelemetry_lib/tracing/span.py:2
- Draft comment:
Typo alert: The import on line 2 readsfrom inspect import Traceback. Typically, traceback-related functionality is imported from thetracebackmodule (all lowercase). Please verify if this is intentional or if it should be corrected. - Reason this comment was not posted:
Comment was on unchanged code.
5. src/lmnr/opentelemetry_lib/tracing/span.py:187
- Draft comment:
Typographical issue: The logger.debug message for 'laminar_association_properties' is split into two separate string literals due to a comma. Merging them into a single string literal would improve readability. - 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% Looking at the code on lines 187-193, the logger.debug() call has multiple string arguments. In Python's logging module, when you pass multiple arguments to logger.debug(), they are concatenated with spaces. However, looking at lines 188-190, there are three separate string literals: one ending with "span ", one saying "does not have attributes object. Perhaps, the span was created with a ", and one saying "custom OTel SDK. Returning an empty dictionary." Then lines 191-192 have another string starting with "Help:". This is indeed split across multiple string arguments. However, this is not necessarily wrong - it will work correctly. The comment is about code style/readability, not a functional bug. Looking at the similar pattern in thetagsproperty (lines 172-177), the same pattern is used there too. This appears to be an intentional style choice by the author, possibly for line length management. The comment is suggesting a minor style improvement, not fixing a bug. This is a style/readability suggestion, not a functional issue. The code works correctly as-is. The rules state "Do NOT make comments that are obvious or unimportant" and "Do NOT comment unless there is clearly a code change required." A minor formatting preference about string concatenation doesn't clearly require a code change - it's subjective and the current code is functional. While the comment is technically correct that the strings could be merged for readability, this is a very minor style issue that doesn't affect functionality. The same pattern appears elsewhere in the file (lines 172-177), suggesting this is an intentional style choice. This type of minor formatting suggestion doesn't meet the bar for "clearly a code change required." This comment should be deleted. It's a minor style suggestion about string formatting that doesn't represent a clear code issue requiring a change. The current code is functional and the pattern is used consistently in the file.
6. src/lmnr/sdk/laminar.py:614
- Draft comment:
Typographical note: There is an inconsistency in the parameter descriptions. The 'labels' parameter documentation uses "span" (i.e., "Labels to set for the span") while the new parameters ('user_id', 'session_id', 'metadata') refer to setting values for the "trace". Consider reviewing these descriptions for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This is a documentation/style comment about consistency in parameter descriptions. The comment is technically correct - there is an inconsistency wherelabelsrefers to "span" while the new parameters refer to "trace". However, I need to consider: 1) Is this actually incorrect or just a stylistic preference? 2) Looking at the semantics,user_id,session_id, andmetadataare indeed trace-level properties (they apply to the entire trace), whilelabels/tagsmight be span-level. So the inconsistency might actually be semantically correct. 3) The rules say "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." This is a minor documentation wording issue that doesn't affect functionality. 4) The comment says "Consider reviewing" which is vague and not actionable - it's asking the author to think about it rather than making a clear suggestion. The inconsistency might be intentional and semantically correct - tags/labels are span-level attributes while user_id/session_id/metadata are trace-level association properties. The comment doesn't provide strong evidence that this is wrong, and it's not actionable since it just asks to "consider reviewing" rather than making a specific change recommendation. Even if the distinction is semantically meaningful, the comment is too vague ("consider reviewing") and doesn't clearly indicate what should be changed. It's a minor documentation style issue that doesn't warrant a comment according to the rules about only commenting when there's a clear code change required. This comment should be deleted. It's a minor, vague documentation style suggestion that doesn't clearly indicate a required change. The inconsistency may even be intentional and semantically correct since tags are span-level while user_id/session_id/metadata are trace-level properties.
7. src/lmnr/sdk/laminar.py:910
- Draft comment:
Typo: There's an extraneous backslash at the end of the line in the metadata parameter description. Please remove it to ensure the docstring reads correctly. - 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% Looking at line 910 in the full file, I see: "metadata (dict[str, AttributeValue] | None, optional): metadata to". The backslash at the end is a line continuation character. Looking at line 911, it continues with "set for the trace. Defaults to None." This is a common Python docstring pattern where backslashes are used to continue long lines. However, in Python docstrings (triple-quoted strings), line continuation backslashes are not necessary - the string naturally continues to the next line. The backslash is actually redundant and could be removed. This is a valid code quality improvement, though minor. The comment is about a change that was made in the diff (the metadata parameter was added), so it is about changes. This is a very minor formatting issue. While technically correct that the backslash is unnecessary in a docstring, it doesn't affect functionality and similar patterns appear elsewhere in the codebase (e.g., lines 420, 618, 950). The comment might be considered too pedantic for a code review. While minor, removing unnecessary backslashes in docstrings is a legitimate code quality improvement that makes the code cleaner and more consistent with Python conventions. However, I notice this pattern appears in multiple places in the same file (lines 420, 618, 950 all have similar backslashes), so singling out just this one instance seems inconsistent. The comment would be more valuable if it addressed all instances or none. The comment is technically correct but very minor. Since similar backslashes appear throughout the file in other docstrings that weren't flagged, and this is a purely cosmetic issue that doesn't affect functionality, the comment is not particularly useful. It's the kind of nitpick that should either be applied consistently across the entire file or not at all.
Workflow ID: wflow_onko3msitS6cv2ZJ
You can customize 
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 8b738bf in 2 minutes and 28 seconds. Click for details.
- Reviewed
680lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9draft 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. tests/test_context_propagation.py:6
- Draft comment:
Typo fix: The function name has been corrected from '_assert_assoiation_properties' to '_assert_association_properties' for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative as it only points out a typo fix in the function name. It does not provide any actionable feedback or suggestions for improvement.
2. src/lmnr/sdk/laminar.py:560
- Draft comment:
The merged metadata is formed with dictionary unpacking (global < context < parent < explicit). While the order is logical, adding an inline comment to explicitly state this merge priority may help future maintainers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/lmnr/sdk/types.py:218
- Draft comment:
Docstring update: The references in the LaminarSpanContext docstring have been updated to use 'Laminar.serialize_span_context' and related methods. This accurately reflects current API usage and improves clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/lmnr/sdk/laminar.py:1180
- Draft comment:
Typographical note: There is an unnecessary backslash at the end of this line in the docstring (span\ from). Consider removing it for 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% The comment suggests removing a backslash fromspan\ fromon line 1180. However, looking at the actual code, the backslash is at the end of line 1180 after "span" and is used for line continuation in the docstring. This is a standard Python convention for breaking long lines in docstrings. The backslash is not creating "span\ from" as a single phrase - it's breaking the line between "span" and "from". This is intentional and correct Python syntax. The comment appears to misunderstand the purpose of the backslash. Additionally, this is part of a newly added docstring in the diff (lines 1176-1187 in the new file), so it is about changes made in the diff. Could the backslash actually be unnecessary or incorrect? In Python docstrings, backslashes at the end of lines are used for continuation, but they might not always be necessary depending on the formatting. I should verify if this is truly a typographical error or if it's intentional line continuation syntax. Looking more carefully at the docstring format, the backslash at the end of line 1180 is indeed used for line continuation. This is a common pattern in Python docstrings when describing parameters. The comment incorrectly identifies this as a typographical error when it's actually intentional syntax. The comment should be deleted as it suggests an incorrect change. The comment incorrectly identifies intentional line continuation syntax as a typographical error. The backslash is properly used to continue the docstring parameter description across multiple lines. This comment should be deleted.
5. src/lmnr/sdk/laminar.py:1185
- Draft comment:
Typographical note: The docstring contains an unnecessary backslash at the end of this line (active span.). Please remove it to avoid confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. tests/test_context_propagation.py:108
- Draft comment:
Typographical error fixed: the function name was corrected from '_assert_assoiation_properties' to '_assert_association_properties'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states that a typographical error was fixed in the function name. It does not provide any actionable feedback or suggestions for improvement.
7. tests/test_context_propagation.py:111
- Draft comment:
Typographical error fixed: the function name was corrected from '_assert_assoiation_properties' to '_assert_association_properties'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states that a typographical error was fixed in a function name. It does not provide any actionable feedback or suggestions for improvement.
8. tests/test_context_propagation.py:184
- Draft comment:
Typo corrected: changed '_assert_assoiation_properties' to '_assert_association_properties' for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states that a typo was corrected for consistency. It does not provide any actionable feedback or suggestions for improvement.
9. tests/test_context_propagation.py:187
- Draft comment:
Typo corrected: changed '_assert_assoiation_properties' to '_assert_association_properties' for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states that a typo was corrected for consistency. It does not provide any actionable feedback or suggestions for improvement.
Workflow ID: wflow_kaEBVtYiIKwvxBz2
You can customize
| try: | ||
| laminar_span_context = LaminarSpanContext.deserialize(parent_span_context) | ||
| except Exception: | ||
| logger.warning( |
Contributor
There was a problem hiding this comment.
In _parse_parent_span_context, when deserializing a parent span context from dict or str, the exception is caught and a warning is logged without including the exception details. Consider logging the exception (or its message) to improve debuggability.
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 08c348a in 1 minute and 9 seconds. Click for details.
- Reviewed
18lines 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. src/lmnr/opentelemetry_lib/tracing/span.py:110
- Draft comment:
Consider caching self.span.get_span_context() in a local variable to avoid redundant calls. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/lmnr/opentelemetry_lib/tracing/span.py:107
- Draft comment:
Ensure that returning a LaminarSpanContext without association properties (user_id, session_id, trace_type, metadata) when attributes are missing is the intended behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_MpS6tUXOtnbvmuSD
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed c957918 in 1 minute and 34 seconds. Click for details.
- Reviewed
29lines 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. src/lmnr/opentelemetry_lib/tracing/span.py:81
- Draft comment:
Initialize association property variables (user_id, session_id, trace_type, metadata) with defaults. Verify that using 'metadata' as a local variable here (shadowing its use elsewhere) is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/lmnr/opentelemetry_lib/tracing/span.py:107
- Draft comment:
Removed early return on missing 'attributes'. Now, even if attributes are not available, a full LaminarSpanContext is returned with default values. Ensure that this behavior (logging a warning and falling back to defaults) is consistent with design expectations. - 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_s7lJOsYKLF53ytjb
You can customize
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 3dca2c4 in 2 minutes and 26 seconds. Click for details.
- Reviewed
36lines 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. src/lmnr/sdk/laminar.py:1392
- Draft comment:
The removal of theglobal_metadataproperty appears intentional. Ensure that this does not break any public API relying on it. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_CDuGwMNhgeEFrSAb
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ed85a5d in 1 minute and 27 seconds. Click for details.
- Reviewed
686lines of code in4files - 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. src/lmnr/opentelemetry_lib/decorators/__init__.py:173
- Draft comment:
Good refactor: replacing the internal _set_association_props_in_context with the centralized set_association_props_in_context improves maintainability. Ensure the centralized function’s docstring reflects its expected behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment suggests replacing an internal function with a centralized one for maintainability, which is a valid code suggestion. However, it also asks to ensure the docstring reflects expected behavior, which is a request for confirmation. This part of the comment violates the rules.
2. src/lmnr/opentelemetry_lib/tracing/utils.py:20
- Draft comment:
The new utility function set_association_props_in_context neatly centralizes association property propagation. Verify that attach_context returns a detach token (or equivalent) so cleanup works as intended. - 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 the behavior of a function, which violates the rule against asking the author to confirm their intention or ensure behavior. It doesn't provide a specific suggestion or point out a clear issue.
3. src/lmnr/sdk/laminar.py:995
- Draft comment:
Consistently using set_association_props_in_context (and renaming current_span to get_current_span) improves code clarity. Confirm that context detachment (via detach_context) and token management remain correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to confirm the correctness of context detachment and token management after certain changes. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules.
4. tests/test_context_propagation.py:55
- Draft comment:
Test updates now expect 3 spans (with an added 'grandparent' span) instead of 2. Ensure that this new hierarchy is intentional and that documentation reflects the updated span structure. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the new hierarchy is intentional and that documentation reflects the changes. This is a request for confirmation and documentation update, which violates the rules.
Workflow ID: wflow_dT45VzlcBZRsFmT8
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ff9b8ed in 2 minutes and 54 seconds. Click for details.
- Reviewed
78lines 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. src/lmnr/sdk/laminar.py:573
- Draft comment:
Good improvement: moving the detach calls into a finally block in start_as_current_span guarantees cleanup even if an exception occurs within the span block. Consider logging detach failures (instead of silently passing) for better debugging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The first part of the comment ("Moving the detach calls into afinallyblock...guarantees cleanup even if an exception occurs") is just describing what the change does - it's not suggesting a change, it's acknowledging the improvement. This violates the rule about not making purely informative comments. The second part ("Consider logging detach failures...for better debugging") is a suggestion for improvement, but it's speculative and optional ("consider"). The current code already has exception handling (try-except that passes), and suggesting to add logging is a minor code quality suggestion that may not be critical. The rules state we should not make speculative comments or comments that aren't clearly required changes. This seems like a "nice to have" rather than a "must fix". The suggestion to log detach failures could be valuable for debugging production issues. Silent failures in cleanup code can make it harder to diagnose problems. This might be more than just a "nice to have" - it could be an actionable improvement that helps with observability. While logging failures could be helpful, the comment starts with an informative statement about what the change does (which violates the rules), and the logging suggestion is phrased as "consider" rather than as a definitive issue. The current implementation with silent exception handling may be intentional to prevent cleanup failures from masking the original exception. Without evidence that this is causing problems, this is more of a code quality suggestion than a required fix. The comment should be deleted. The first part is purely informative (acknowledging what the change does), and the second part is an optional suggestion ("consider") rather than a clear code issue. The rules explicitly state not to make purely informative comments or speculative suggestions.
2. src/lmnr/sdk/laminar.py:883
- Draft comment:
Similar cleanup in use_span: moving context detachment and pop_span_context into the finally block improves reliability. Again, consider logging any detach exceptions rather than silently suppressing them. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The first part of the comment ("moving context detachment and pop_span_context into the finally block improves reliability") is just acknowledging what was done - this is not actionable. The second part ("Consider logging any detach exceptions rather than silently suppressing them") is a suggestion for improvement. However, looking at the code in the finally block (lines 883-885), there's no try-except around these calls - they're just called directly. The comment seems to be confusing this with the start_as_current_span method (lines 595-599) where there IS a try-except that suppresses exceptions. In use_span's finally block, exceptions are NOT being suppressed - if detach or pop_span_context raise an exception, it would propagate. So the comment appears to be incorrect about the current state of the code. I might be missing something - perhaps the comment is suggesting to add exception handling around the detach calls in the finally block. But that would be a new suggestion, not about what was changed. Also, I should verify whether these detach operations can actually raise exceptions that need handling. Looking more carefully, the comment says "Consider logging any detach exceptions rather than silently suppressing them" - this could be interpreted as a suggestion to ADD exception handling with logging. However, this would be a suggestion for new code, not about the changes made in the diff. The diff simply moved these lines into the finally block without adding any exception suppression. The comment is either incorrect (if it's claiming exceptions are being suppressed when they're not) or it's suggesting new functionality (adding exception handling), which would make it not directly about the changes in the diff. The comment should be deleted. The first part is just acknowledging the change (not actionable), and the second part either mischaracterizes the code (no exception suppression is happening in use_span's finally block) or suggests adding new functionality that wasn't part of the diff changes.
Workflow ID: wflow_OoAlP5IoOKhNu8T0
You can customize
src/lmnr/sdk/laminar.py
Outdated
| finally: | ||
| context_api.detach(context_token) | ||
| detach_context(isolated_context_token) | ||
| wrapper.pop_span_context() |
There was a problem hiding this comment.
Bug: Missing error handling may cause UnboundLocalError in use_span
The use_span method's finally block accesses context_token and isolated_context_token without error handling. If an exception occurs in the try block before these variables are assigned (e.g., in set_association_props_in_context, push_span_context, or attach_context), the finally block will raise UnboundLocalError. Unlike start_as_current_span which wraps similar cleanup code in a try/except, use_span lacks this protection.
Additional Locations (1)
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 503ee39 in 57 seconds. Click for details.
- Reviewed
45lines 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. src/lmnr/opentelemetry_lib/decorators/__init__.py:82
- Draft comment:
Added JSON serialization for non-string inputs. Ensure that json_dumps handles all expected types. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/lmnr/opentelemetry_lib/decorators/__init__.py:121
- Draft comment:
Similarly, non-string output is serialized to JSON. Confirm that this behavior is intended for all output types. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/lmnr/sdk/laminar.py:887
- Draft comment:
Wrapping the detach and pop operations in a try/finally block ensures that span.end() is always called, even if detach operations throw. This improves robustness. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_VnShDcc3KJnpm0U8
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4d9a957 in 1 minute and 40 seconds. Click for details.
- Reviewed
22lines 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. src/lmnr/opentelemetry_lib/decorators/__init__.py:80
- Draft comment:
Removed JSON conversion of the input_formatter result. Ensure that if input_formatter returns a non-string, LaminarSpan.set_input now handles conversion appropriately. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/lmnr/opentelemetry_lib/decorators/__init__.py:116
- Draft comment:
Removed JSON conversion of the output_formatter result. Verify that non-string outputs are now correctly handled by LaminarSpan.set_output. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to verify that non-string outputs are correctly handled after a change. This falls under asking the author to ensure behavior is intended, which is against the rules.
Workflow ID: wflow_g1QQxBSuXmIdpPUO
You can customize
There was a problem hiding this comment.
Bug: Context tokens detached in wrong order causing potential leaks
Context tokens are detached in incorrect order during cleanup. Tokens are attached in order: _lmnr_assoc_props_token (T1), push_span_context token (T2), then isolated_ctx_token (T4). In the exception path, detachment order is T1, T2, T4 (same as attachment). In normal path, order is T4, T1, T2. The correct order is T4, T2, T1 (reverse). When ContextVar tokens are reset out-of-order, the final context reflects the last token's captured state. This leaves association properties (user_id, session_id, metadata) attached in the isolated context after function completion, potentially causing them to leak to subsequent decorated functions run in the same async context.
src/lmnr/opentelemetry_lib/decorators/__init__.py#L191-L199
lmnr-python/src/lmnr/opentelemetry_lib/decorators/__init__.py
Lines 191 to 199 in 4d9a957
src/lmnr/opentelemetry_lib/tracing/span.py#L374-L387
lmnr-python/src/lmnr/opentelemetry_lib/tracing/span.py
Lines 374 to 387 in 4d9a957
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note
Introduces LaminarSpan with span helpers, propagates user/session/trace_type/metadata through context and processor, centralizes json_dumps, updates SDK/instrumentations, and adds tests for context/metadata.
LaminarSpanwrapper with helpers (set_input/output, tags, association props, context cleanup) and export in__all__.user_id,session_id,trace_type, andmetadatavia isolated context (context.py,utils.set_association_props_in_context) and set on spans inLaminarSpanProcessor.LaminarSpanand manage context stack properly.Laminar.initializewith globalmetadataand merge it instart_span,start_as_current_span,start_active_span(also acceptuser_id,session_id,metadata).Laminar.get_current_span,set_trace_user_id/session_id/metadata,set_span_tags/add_span_tags, and span-context (de)serialization helpers.observe(sync/async) to acceptmetadata, useLaminarSpanfor input/output, and propagate association props.lmnr.sdk.utils.json_dumps; update usages across instrumentations and SDK.json_dumpsand minor formatting/attribute adjustments across OpenAI, Google GenAI, Kernel, CUA, Skyvern, OpenHands, LiteLLM, Browser Use, Claude Agent.test_context_propagation.pyandtest_global_metadata.py; expand existing tests for tags, session/user IDs, metadata merging, and span APIs.claude-agent-sdkextra tolmnr-claude-code-proxy>=0.1.3.Written by Cursor Bugbot for commit 4d9a957. This will update automatically on new commits. Configure here.
Important
Introduce
LaminarSpanwith context-propagated properties, refactor JSON utils, update APIs, and add tests.LaminarSpanwith context-propagated association properties and global metadata.user_id,session_id,trace_type,metadatavia context and populate inLaminarSpanProcessor.LaminarTracerto manage isolated contexts and span push/pop.metadata, set association props, and returnLaminarSpan.metadataininitializeand expose helpers for span context operations.observeacceptsmetadataand usesLaminarSpanmethods; refactor instrumentations to use centralized JSON utils.json_dumpsand serialization helpers insdk.utils.set_association_props_in_context.test_context_propagation.py,test_global_metadata.py; expand tests for tags, session/user IDs, metadata, serialization.lmnr-claude-code-proxyto>=0.1.3and include inallextras.This description was created by
for 4d9a957. You can customize this summary. It will automatically update as commits are pushed.