update the interface behind start_active_span

[dinmukhamedm]

Note

Simplifies span activation to return a manually-ended span, introduces LaminarSpan/LaminarTracer with auto context cleanup, centralizes push/pop in context, updates all tests, and bumps version to 0.7.20.

• API:
  • Laminar.start_active_span(...) now returns a Span that must be ended via span.end(); removes token handling and end_active_span usage; updates docstrings.
• Tracing Internals:
  • Adds LaminarSpan wrapper that pops/detaches context on end().
  • Adds LaminarTracer so start_span/start_as_current_span yield LaminarSpan.
  • Moves push_span_context/pop_span_context to tracing/context.py with isolated token stack helpers; TracerWrapper delegates to these.
• Tests:
  • Refactors observe/tracing (incl. async/threading/threadpool) tests to drop context tokens and call span.end(); adjusts nesting/concurrency cases accordingly.
• Version:
  • Bumps version to 0.7.20.

^{Written by Cursor Bugbot for commit dfbe740. This will update automatically on new commits. Configure here.}

---

Important

Refactor Laminar SDK to simplify span lifecycle management and context handling, introducing LaminarSpan and LaminarTracer, and updating tests accordingly.

• Behavior:
  • Laminar.start_active_span() now returns a Span object, removing token handling and end_active_span().
  • Introduces LaminarSpan and LaminarTracer to wrap SDK spans.
  • Adds push_span_context() and pop_span_context() for context management.
• Context Management:
  • Updates TracerWrapper.push_span_context() and pop_span_context() to use new context methods.
  • Simplifies context handling by removing token stack management.
• Tests:
  • Updates tests in test_observe.py, test_tracing.py, and test_tracing_concurrency.py to use span.end() directly.
  • Adjusts tests to reflect changes in span lifecycle and context management.

^{This description was created by for dfbe740. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 595a889 in 1 minute and 47 seconds. Click for details.

• Reviewed 868 lines of code in 9 files
• Skipped 0 files when reviewing.
• Skipped posting 10 draft 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_concurrency.py:22

• Draft comment:
  Add a brief inline comment describing the expected behavior of each async task in this test to aid future maintainers.
• 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_tracing_concurrency.py:174

• Draft comment:
  Consider reducing or parameterizing the sleep duration to prevent potential flakiness and speed up tests.
• 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. tests/test_tracing_concurrency.py:370

• Draft comment:
  Avoid patching the private '_client' attribute of the OpenAI client; if possible, use the public API to reduce brittleness.
• 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. tests/test_tracing_concurrency.py:800

• Draft comment:
  Good use of assertions comparing trace IDs. Consider adding debug logging for trace IDs on failure to assist with troubleshooting.
• 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. tests/test_tracing_concurrency.py:998

• Draft comment:
  This mixed concurrency test is comprehensive. Ensure that any new concurrency model added in the future also verifies independent trace contexts.
• 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.

6. tests/test_tracing_concurrency.py:1240

• Draft comment:
  Add a clarifying comment explaining why worker spans in this ThreadPoolExecutor test are expected to be in separate traces, to make the intent explicit.
• 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.

7. tests/test_tracing_concurrency.py:1128

• Draft comment:
  Document the expected hierarchy in nested active spans (async or threading) to clarify parent-child relationships for future maintainers.
• 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.

8. tests/test_tracing_concurrency.py:1240

• Draft comment:
  In the ThreadPoolExecutor context isolation test, consider logging or asserting the worker span parent IDs explicitly to ensure proper context propagation.
• 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.

9. tests/test_tracing.py:27

• Draft comment:
  In test_start_as_current_span_exception, consider asserting the complete set of exception attributes for more robust validation.
• 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.

10. tests/test_tracing.py:71

• Draft comment:
  Ensure that the mapping of attribute keys (e.g. from Attributes.PROVIDER to 'gen_ai.system') remains consistent; consider adding a comment linking these transformations to the documentation.
• 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_WOpwv2JSxlTwWrfr

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed d3b3209 in 44 seconds. Click for details.

• Reviewed 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft 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:713

• Draft comment:
  The return value has been updated to only return span (removing the context token). Ensure that the documentation and tests have been updated to reflect this change.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to ensure that documentation and tests are updated, which violates the rule against asking the author to ensure things are tested or documented. It does not provide a specific suggestion or ask for a specific test to be written.

Workflow ID: wflow_oSJThb8GnyABJ2Mt

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 82e273c in 2 minutes and 22 seconds. Click for details.

• Reviewed 67 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 4 draft 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:29

• Draft comment:
  Good early-return check using is_recording(). Ensure that silently skipping end() when not recording fits all debug and lifecycle expectations.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. src/lmnr/opentelemetry_lib/tracing/span.py:32

• Draft comment:
  The try/except around detach() was removed. Ensure that detach() internally handles context detachment issues (like mismatched async contexts) since errors aren’t explicitly caught here.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. src/lmnr/opentelemetry_lib/tracing/span.py:115

• Draft comment:
  Updated property accessors to delegate directly to the underlying span (self.span.dropped_attributes, etc.) which is correct, replacing use of super().
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. src/lmnr/sdk/laminar.py:713

• Draft comment:
  In start_active_span, the method now returns only a span after attaching a context token (_lmnr_ctx_token). Consider using a more unique internal attribute name to help avoid accidental collisions in user code.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_snYXWJIX8hBpSztO

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 684d7de in 2 minutes and 9 seconds. Click for details.

• Reviewed 33 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft 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:30

• Draft comment:
  Removed the is_recording() check; ensure that always calling self.span.end() is the intended behavior when the span may not be recording.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. src/lmnr/opentelemetry_lib/tracing/span.py:32

• Draft comment:
  The try/except block silently swallows all exceptions. Consider logging or handling specific exceptions to aid debugging if pop_span_context() or detach() fails.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 35% vs. threshold = 50% The comment is about a code change in the diff (the new try/except block). It's suggesting a code quality improvement - to log exceptions rather than silently swallow them. However, I need to consider: 1) The existing comment says "Internally handles and logs the error" which suggests the author is aware of error handling, 2) The bare except with pass might be intentional to ensure cleanup never fails the end() method, 3) This is a cleanup/teardown operation where failures might be expected and acceptable to ignore. The comment is somewhat speculative about whether logging is needed - it says "consider" and "to aid debugging", which suggests it's not a definite issue. The author may have intentionally chosen to silently ignore errors here. The comment might be valid - silently swallowing exceptions is generally a code smell. However, the existing inline comment suggests the author is aware of error handling concerns. The comment is also somewhat speculative ("consider") rather than pointing to a definite bug. It's possible the author intentionally wants to ignore all errors during cleanup to ensure end() never fails. While the comment is about changed code and suggests a reasonable improvement, it's not pointing to a clear bug or issue. The use of "consider" makes it more of a suggestion than a required fix. Given the context of cleanup code and the existing comment about error handling, the author likely made a deliberate choice here. This falls into the category of "not clearly necessary" and could be seen as second-guessing the author's intentional design choice. This comment is a code quality suggestion rather than identifying a clear bug. While logging exceptions is generally good practice, the bare except might be intentional for cleanup code. The comment is speculative ("consider") and the author appears to have thought about error handling based on the inline comment. I'll give this a moderate-low score as it's not clearly wrong but also not clearly necessary.

Workflow ID: wflow_gMUQfEk5YChYA99G

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed a0a1f96 in 1 minute and 58 seconds. Click for details.

• Reviewed 51 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft 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:36

• Draft comment:
  Reordered setting of _popped after calling detach(). Ensure this ordering is intentional, so that if detach() raises an exception, the span isn’t marked as popped.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. src/lmnr/sdk/laminar.py:430

• Draft comment:
  Docstring updated for start_span and start_active_span clarifies that spans must be ended manually in LIFO order. Verify the added instructions align with intended API usage.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_c5iALkF0tpNPLvNS

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed a71921a in 1 minute and 28 seconds. Click for details.

• Reviewed 11 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft 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:170

• Draft comment:
  to_json is now a regular method instead of a property. Ensure that all call sites are updated to use it as a callable (e.g., span.to_json()) if they previously relied on property access.
• 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 PR author to "ensure" or "verify" that call sites are updated, which violates the rule: "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended, to make sure their change is tested, or similar." The comment starts with "Ensure that..." which is explicitly mentioned as a red flag. Additionally, if there were call sites that weren't updated, this would be caught at runtime or by tests - it's not providing a specific code change. The comment is speculative and asks for verification rather than pointing out a concrete issue in the code being reviewed. However, this is a breaking API change that could cause runtime errors if call sites aren't updated. The comment might be valuable as it highlights a potential issue that could slip through if tests don't have full coverage. It's pointing out a real consequence of the change. While the API change is real, the comment doesn't point to any specific problematic call site in the code being reviewed. It's asking the author to verify/ensure something rather than identifying a concrete issue. If there were actual call sites that needed updating and weren't updated, that would be caught by the build or tests. The rules explicitly state not to make "ensure that" or "verify that" comments. This comment should be deleted because it asks the PR author to "ensure" something is done correctly, which violates the explicit rule against verification requests. It doesn't point to a specific code issue in the diff and would be caught by tests or runtime if there were problems.

Workflow ID: wflow_zxOBVEJilK7ihXki

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 46c044b in 1 minute and 13 seconds. Click for details.

• Reviewed 42 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft 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:24

• Draft comment:
  Good addition: The comment clearly explains that _popped prevents double popping when end() is called multiple times.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. src/lmnr/sdk/laminar.py:663

• Draft comment:
  Updated docstring and usage example: The added note about child spans and the switch from start_active_span to start_as_current_span (with @observe decorators) clarify the new API behavior. Ensure consistency across documentation.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_8Krt6HHWJGes0mFL

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed dfbe740 in 1 minute and 40 seconds. Click for details.

• Reviewed 26 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft 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. pyproject.toml:9

• Draft comment:
  Version bump updated to 0.7.20. Ensure consistency with release notes.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. src/lmnr/version.py:6

• Draft comment:
  Updated version to 0.7.20. Confirm alignment with pyproject.toml.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_IhCrxfea0wdjxSNa

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}