-
Notifications
You must be signed in to change notification settings - Fork 9
init laminar span context from env var #221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to a76ddb0 in 1 minute and 48 seconds. Click for details.
- Reviewed
73lines 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. src/lmnr/sdk/laminar.py:63
- Draft comment:
New attribute __default_parent_span_context is added. Consider adding a brief inline comment explaining its role and potential thread-safety considerations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/lmnr/sdk/laminar.py:211
- Draft comment:
In _initialize_context_from_env, a broad exception is caught when deserializing LMNR_SPAN_CONTEXT. Consider catching more specific exceptions to avoid masking unexpected issues. - 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% This is a code quality suggestion about exception handling. The comment suggests being more specific with exception catching. However, looking at the broader context, I can see that elsewhere in the same file (lines 384, 410, 580, 672), the code also uses broad exception catching withexcept Exception. This appears to be a deliberate pattern in this codebase for handling deserialization and other operations that might fail in various ways. The comment is suggesting a refactor, but it's not pointing to a clear bug or issue. The broad exception catching here seems intentional - they want to gracefully handle any deserialization failure and just log a warning, which is reasonable for initialization code reading from an environment variable. The pylint disable comment also shows this was a conscious decision. The comment is technically correct that catching specific exceptions is generally better practice. However, this appears to be an intentional pattern used throughout the codebase for similar scenarios. The code is handling an optional environment variable during initialization, and gracefully degrading if it can't be parsed seems like reasonable behavior. While the suggestion has merit in general, it's not clearly actionable without knowing what specific exceptionsLaminarSpanContext.deserializemight raise. The broad exception catching appears intentional and consistent with the codebase's error handling patterns. This is more of a stylistic preference than a clear issue. This comment should be deleted. It's a code quality suggestion that isn't clearly actionable, and the broad exception catching appears to be an intentional pattern in this codebase. The code is handling optional initialization from an environment variable and gracefully degrading, which is reasonable behavior.
3. src/lmnr/sdk/laminar.py:373
- Draft comment:
Defaulting parent_span_context to __default_parent_span_context is applied here (and similarly in the later block). Consider refactoring this duplicate logic into a helper to ensure consistency across span creation functions. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_83M8P7IUsVsQuBQQ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed a283ee5 in 2 minutes and 33 seconds. Click for details.
- Reviewed
228lines 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. tests/test_tracing.py:468
- Draft comment:
In test_span_context_str, the helper function ‘foo’ expects a string (as indicated by its parameter type) but is passed a dict (via get_laminar_span_context_dict). Consider using get_laminar_span_context_str (or updating the parameter type) to avoid ambiguity. - 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:267
- Draft comment:
A commented-out assertion in test_use_span_with_auto_instrumentation_langchain should be reviewed. Remove or update the commented assertion to clarify expected span count. - 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.py:15
- Draft comment:
Several tests include redundant 'pass' statements inside context managers. While not harmful, they could be removed to improve code clarity. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
Workflow ID: wflow_6OSzuc5CjvffT5Lv
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 2d90abd in 1 minute and 10 seconds. Click for details.
- Reviewed
23lines 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. tests/test_observe.py:123
- Draft comment:
Using os.environ.pop('LMNR_SPAN_CONTEXT', None) instead of os.unsetenv() is a good, cross‐platform approach to remove the env var. - 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:675
- Draft comment:
The new test 'test_span_context_from_env_variables' correctly sets, initializes, and then verifies propagation of the parent span context from the LMNR_SPAN_CONTEXT env var. The cleanup logic (restoring or removing the env var) is also properly handled. - 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_Fy1ulCzq7QkQ4jvC
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 4bde579 in 23 seconds. Click for details.
- Reviewed
26lines 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. pyproject.toml:9
- Draft comment:
Version bump to 0.7.24 is reflected here. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/lmnr/version.py:6
- Draft comment:
Updated version to 0.7.24 to match new release. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_2Qknty9MDvWpe6V5
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Note
Initialize span context from
LMNR_SPAN_CONTEXTand propagate parent path/ids via the processor, with tests and version bump.Laminar._initialize_context_from_env()to readLMNR_SPAN_CONTEXT, deserialize toLaminarSpanContext, set aNonRecordingSpanin context, push it, and preseed parent path/ids viaLaminarSpanProcessor.set_parent_path_infoduringLaminar.initialize().LaminarSpanProcessor.set_parent_path_info(parent_span_id, span_path, span_ids_path)and use path caches when settinglmnr.span.path/lmnr.span.ids_pathon span start.tests/test_tracing.pyandtests/test_observe.pyensuring path/ids and parent/trace consistency.0.7.24.Written by Cursor Bugbot for commit 4bde579. This will update automatically on new commits. Configure here.
Important
Initialize Laminar span context from
LMNR_SPAN_CONTEXTenvironment variable with processor support and tests._initialize_context_from_env()inlaminar.pyto readLMNR_SPAN_CONTEXT, deserialize toLaminarSpanContext, set OTEL parent, and push context; invoked duringLaminar.initialize().Laminarto coordinate withTracerWrapper/LaminarSpanProcessorfor inheritinglmnr.span.pathandlmnr.span.ids_pathfrom upstream context.set_parent_path_info()toLaminarSpanProcessorto seed parent span path/id-path caches.test_tracing.pyandtest_observe.pyverifying parent linkage, trace IDs, andlmnr.span.path/lmnr.span.ids_path.This description was created by
for 4bde579. You can customize this summary. It will automatically update as commits are pushed.