-
Notifications
You must be signed in to change notification settings - Fork 9
Kernel #206
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
Kernel #206
Conversation
src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/__init__.py
Outdated
Show resolved
Hide resolved
src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/__init__.py
Outdated
Show resolved
Hide resolved
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 0faeabd in 2 minutes and 12 seconds. Click for details.
- Reviewed
509lines of code in6files - 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:144
- Draft comment:
Ensure the updated uv_build version (>=0.9.7,<0.10) is compatible with your build and tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure compatibility of a dependency version with their build and tests. This falls under the rule of not asking the author to ensure or verify things, especially related to dependencies.
2. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/__init__.py:342
- Draft comment:
Consider making the 'to_wrap' parameter in _wrap_app_action optional to avoid having to pass an empty dict. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_OqvRT7XmS2XijO2I
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/__init__.py
Outdated
Show resolved
Hide resolved
src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/utils.py
Outdated
Show resolved
Hide resolved
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 9d4399c in 1 minute and 22 seconds. Click for details.
- Reviewed
34lines 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. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/__init__.py:222
- Draft comment:
Use 'await' when calling the wrapped function in async wrapper, and pull the span_type from to_wrap. This makes the async instrumentation consistent with the sync version. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/utils.py:27
- Draft comment:
Removed debug print to avoid unintended logging in production. This cleanup prevents leaking extra output. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining why a debug print was removed. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. It violates the rule against making purely informative comments.
Workflow ID: wflow_adosyxyeNBjEUdSz
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/utils.py
Show resolved
Hide resolved
src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/kernel/__init__.py
Show resolved
Hide resolved
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 98668f0 in 1 minute and 42 seconds. Click for details.
- Reviewed
20lines 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/opentelemetry_lib/opentelemetry/instrumentation/kernel/utils.py:31
- Draft comment:
Using deepcopy here prevents modifying the original dict, which is good practice. However, if the dict only contains shallow (immutable) values, consider using a shallow copy (e.g., output.copy()) for better performance. - 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% The comment is about the change fromoutputtodeepcopy(output), which is indeed part of the diff. However, the comment is speculative ("If the dict only contains shallow values...") and suggests a conditional optimization without knowing the actual structure of the data. The author clearly chosedeepcopyfor a reason - likely because they need to avoid mutating the original dict (lines 32-35 show the dict is being modified). The comment doesn't provide strong evidence that shallow copy would be sufficient, and using shallow copy could introduce bugs if the dict contains nested structures. This is a speculative performance suggestion without clear evidence it's safe or necessary. The author added deepcopy for a reason - to avoid mutating the original dict. Without knowing the actual structure of the data being passed in, suggesting a shallow copy could introduce bugs. The performance concern is speculative and may not be significant in practice. The comment is speculative ("If the dict only contains...") and doesn't provide strong evidence that the change would be safe or beneficial. The author likely chose deepcopy deliberately to ensure no mutation of the original object, and without knowing the actual data structures involved, this suggestion could introduce subtle bugs. This comment should be deleted. It's a speculative performance optimization that starts with "If" and doesn't provide clear evidence that the change would be safe or necessary. The author chose deepcopy deliberately, and suggesting shallow copy without understanding the data structures could introduce bugs.
Workflow ID: wflow_XkYehWtcgXdQ7UjE
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Note
Adds Kernel OpenTelemetry instrumentation with output formatters, registers it in tracing, and updates packaging/version to 0.7.21.
opentelemetry/instrumentation/kernel/__init__.pywrappingBrowsersResource,ComputerResource(tool spans),ProcessResource(tool spans),PlaywrightResource.execute, andKernelApp.action(observed handlers with flush).screenshot_tool_output_formatterandprocess_tool_output_formatterinkernel/utils.py.KernelInstrumentorInitializerin_instrument_initializers.pyand registerInstruments.KERNELintracing/instruments.py.0.7.21(pyproject.toml,version.py), add Python3.14classifier, include dev depkernel>=0.18.0, and update build-system touv_build>=0.9.7,<0.10.Written by Cursor Bugbot for commit 98668f0. This will update automatically on new commits. Configure here.
Important
Add Kernel OpenTelemetry instrumentation, update version to 0.7.21, and add new dependency.
KernelInstrumentorinopentelemetry/instrumentation/kernel/__init__.pyto traceBrowsersResource,ComputerResource,PlaywrightResource,ProcessResource, andKernelApp.actionwith async wrappers and error handling.kernel/utils.py:screenshot_tool_output_formatterandprocess_tool_output_formatterfor base64 decoding.KernelInstrumentorInitializerin_instrument_initializers.pyand addInstruments.KERNELininstruments.py.0.7.21inpyproject.tomlandversion.py.kernel>=0.18.0.uv_build>=0.9.7,<0.10and add Python3.14classifier.This description was created by
for 98668f0. You can customize this summary. It will automatically update as commits are pushed.