start active span

[dinmukhamedm]

LAM-815

---

Important

Introduces start_active_span in laminar.py for span management with context, adding comprehensive tests for various concurrency models.

• Behavior:
  • Adds start_active_span() and end_active_span() methods to Laminar class in laminar.py for starting and ending spans with context management.
  • Supports manual instrumentation with options for span_type, context, parent_span_context, and tags.
  • Returns a span and a context token for detaching the context.
• Tests:
  • Adds tests in test_observe.py, test_observe_concurrency.py, test_tracing.py, and test_tracing_concurrency.py to validate start_active_span() functionality.
  • Covers scenarios including nested spans, async and threading concurrency, and context isolation.
  • Ensures spans have correct parent-child relationships and trace IDs.
• Misc:
  • Updates get_span_context() usage to get_span_context().span_id in multiple test files for consistency.

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

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 55de7d5 in 3 minutes and 33 seconds. Click for details.

• Reviewed 1336 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 7 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:87

• Draft comment:
  Several tests mix accessing the span’s context via both 'span.context.span_id' and 'span.get_span_context().span_id'. For clarity and maintainability, consider standardizing on one method (preferably get_span_context()) across 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.

2. tests/test_tracing.py:536

• Draft comment:
  Clearing the span processor (via TracerWrapper.instance._span_processor.clear()) to simulate span context passing is useful for testing but may leak global state. Ensure tests are isolated or reset the processor between 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:
  Patching internal attributes (e.g. openai_client._client.send) to simulate auto‐instrumentation works for tests but relies on internal APIs. Consider adding a comment explaining this dependency and its potential fragility if the library internals change.
• 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_observe.py:175

• Draft comment:
  The observe decorator tests are very comprehensive. For clarity, consider adding inline comments in nested observe cases to explain the expected parent–child span relationships.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50% None

5. tests/test_tracing_concurrency.py:956

• Draft comment:
  The mixed concurrency test that runs async, thread, and threadpool tasks simultaneously checks for proper trace isolation. The coverage is excellent—just ensure that shared state in the underlying tracing library is isolated across these models.
• Reason this comment was not posted:
  Confidence changes required: 10% <= threshold 50% None

6. tests/test_tracing.py:437

• Draft comment:
  The tests for span context serialization/deserialization (and verifying span paths and ids_path) are thorough. Ensure that any edge cases, such as passing an already-ended span or non-dict/string contexts, are also handled in production.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50% None

7. tests/test_tracing_concurrency.py:1158

• Draft comment:
  The tests for start_active_span with nested async and threaded environments (using observe decorator) are well-organized and comprehensive. No issues found here.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_1gGfLuA0xjAjeEkp

^{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 53afe3f in 1 minute and 38 seconds. Click for details.

• Reviewed 326 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. tests/test_observe_concurrency.py:92

• Draft comment:
  Good update: replacing '.context.span_id' with 'get_span_context().span_id' ensures tests use the newer API consistently. Verify that get_span_context() returns the expected span context.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment starts with a purely informative statement about the update, which is not allowed. The second part of the comment asks the PR author to verify the behavior, which is also not allowed. Therefore, this comment should be removed.

2. tests/test_tracing_concurrency.py:98

• Draft comment:
  Consistently using get_span_context() instead of direct .context access aligns with the updated API. Ensure that all child-parent span assertions accurately reflect this change.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment suggests using a specific method consistently, which is a valid code suggestion. However, it also asks to ensure that assertions reflect this change, which is a request for confirmation. This part of the comment violates the rules.

Workflow ID: wflow_oRxsmrH3KKxtG4rz

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