-
Notifications
You must be signed in to change notification settings - Fork 9
Cdp use fixes #184
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
Cdp use fixes #184
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.
Caution
Changes requested ❌
Reviewed everything up to ec9e390 in 2 minutes and 7 seconds. Click for details.
- Reviewed
862lines of code in9files - Skipped
0files when reviewing. - Skipped posting
11draft 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.11 is applied. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/lmnr/opentelemetry_lib/tracing/_instrument_initializers.py:80
- Draft comment:
New BrowserUseSession and Bubus instrumentor initializers are added. Confirm that the async_client parameter is correctly passed to BrowserUseInstrumentor. - 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/tracing/instruments.py:20
- Draft comment:
Enum values for BROWSER_USE_SESSION and BUBUS added; verify mapping in INSTRUMENTATION_INITIALIZERS. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/lmnr/sdk/browser/bubus_otel.py:42
- Draft comment:
New Bubus Instrumentor wraps EventBus methods; consider logging or handling errors in the wrapper if needed. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
5. src/lmnr/sdk/browser/cdp_utils.py:518
- Draft comment:
Enhanced CDP functions now use a timeout and isolate context; verify that returning None in error cases (e.g., in inject_session_recorder) is handled appropriately by callers. - 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 returning None in error cases is handled appropriately by callers. This falls under asking the author to ensure behavior is intended or to double-check things, which is against the rules. Therefore, this comment should be removed.
6. src/lmnr/sdk/evaluations.py:115
- Draft comment:
Type hints for 'instruments' and 'disabled_instruments' now accept set, list, or tuple. Confirm that downstream usage supports all types. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
7. src/lmnr/sdk/laminar.py:421
- Draft comment:
Documentation and usage examples updated to use Laminar.use_span; changes look consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/lmnr/version.py:6
- Draft comment:
Version updated to 0.7.11; version fetching from PyPI looks handled with fallbacks. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. src/lmnr/opentelemetry_lib/tracing/_instrument_initializers.py:62
- Draft comment:
Typo detected: In the docstring, "BubusInstrumentor" appears on line 62. Please confirm if this should be "BrowserUseInstrumentor" or another intended name. - Reason this comment was not posted:
Comment looked like it was already resolved.
10. src/lmnr/opentelemetry_lib/tracing/instruments.py:21
- Draft comment:
Typo noticed: The enum value 'BUBUS' doesn’t appear to match expected naming conventions. Please confirm if 'BUBUS' is correct or if it should be something else. - 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 name BUBUS seems intentional since it's consistently used in both the enum and initializer. The code follows a clear pattern where each enum value has a matching initializer. While BUBUS is an unusual name, there's no evidence it's incorrect - it could be a valid product/library name. The comment is essentially asking the author to double-check their intention, which violates our review rules. I could be wrong if BUBUS is actually a typo of something else. There might be external context about what this instrumentation is supposed to be for. Even if BUBUS is unusual, the consistent usage and matching initializer suggests it's intentional. We're instructed not to ask authors to confirm their intentions or double-check things. Delete the comment. It violates the rule against asking authors to confirm their intentions, and there's no strong evidence that BUBUS is actually incorrect.
11. src/lmnr/sdk/evaluations.py:180
- Draft comment:
Typographical suggestion: The phrase "auto-instrumentations" seems off. For consistency with the docs link (which uses "automatic-instrumentation") and common usage (singular "instrumentation"), consider changing it to "auto-instrumentation". - 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 - the docs link on line 178 uses "automatic-instrumentation" (singular) while this docstring uses "auto-instrumentations" (plural). The suggestion would improve consistency. However, this is a very minor docstring wording change that doesn't affect functionality. The rules say not to make purely informative comments. The comment is technically correct and would improve consistency, but it's a very minor docstring change. The rules explicitly say not to make purely informative comments. While the change would be an improvement, this type of minor docstring wording suggestion doesn't meet the bar for required code changes according to the rules. Delete this comment as it is purely informative and doesn't point out a required code change.
Workflow ID: wflow_sGuNaN9T5nsZDtvG
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
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 1fdc895 in 1 minute and 40 seconds. Click for details.
- Reviewed
30lines 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/browser/cdp_utils.py:601
- Draft comment:
Returning True on TimeoutError and Exception (instead of False) is intentional for safety, but this may skip valid pages on transient errors. Confirm that this behavior aligns with overall error handling strategy. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/lmnr/sdk/browser/cdp_utils.py:520
- Draft comment:
Typo: 'Thius' should be 'This'. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_AgjhksvDFFS8iArK
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Draft: there are still intermittent
ERROR [cdp_use.client] CDP Error for request 131: {'code': -32000, 'message': 'Cannot find context with specified id'}Important
Enhance instrumentation for
browser-useand addbubussupport, improve session recording, and increment version to 0.7.11.BubusInstrumentorInitializerandBrowserUseSessionInstrumentorInitializerin_instrument_initializers.py.BUBUSandBROWSER_USE_SESSIONtoInstrumentsenum ininstruments.py.INSTRUMENTATION_INITIALIZERSininstruments.pyto include new initializers.get_isolated_context_id()andshould_skip_page()incdp_utils.pyfor better session management.inject_session_recorder()incdp_utils.pyto use isolated context IDs.inject_session_recorder()andtake_full_snapshot()incdp_utils.py.BubusInstrumentorinbubus_otel.pyto handlebubusevents.wrap_dispatch()andwrap_process_event()inbubus_otel.pyfor event handling.pyproject.tomlandversion.py.browser_use_cdp_otel.pyandcdp_utils.py.This description was created by
for 1fdc895. You can customize this summary. It will automatically update as commits are pushed.