You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/10/25 02:47:47 UTC

[GitHub] [skywalking-python] westarest opened a new pull request, #246: add GreenletProfiler

westarest opened a new pull request, #246:
URL: https://github.com/apache/skywalking-python/pull/246

   <!-- Uncomment the following checklist WHEN AND ONLY WHEN you're adding a new plugin -->
   <!--
   - [ ] Add a test case for the new plugin
   - [ ] Add a CHANGELOG entry for the new plugin
   - [ ] Add a component id in [the main repo](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
   - [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-booster-ui/tree/main/src/assets/img/technologies)
   - [ ] Rebuild the `requirements.txt` by running `tools/env/build_requirements_(linux|windows).sh`
   - [ ] Rebuild the `Plugins.md` documentation by running `make doc-gen`
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] sonatype-lift[bot] commented on a diff in pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#discussion_r1008225278


##########
tests/e2e/base/provider/flask_provider.py:
##########
@@ -0,0 +1,32 @@
+# from gevent import monkey
+# monkey.patch_all()
+# import grpc.experimental.gevent as grpc_gevent     # key point
+# grpc_gevent.init_gevent()   # key point
+# from skywalking import config, agent  
+# config.logging_level = 'DEBUG'
+# config.init()
+# agent.start()
+
+import time
+import random
+from flask import Flask, request
+
+app = Flask(__name__)
+   
+@app.route('/artist', methods=['POST'])
+def artist():
+    try:
+
+        time.sleep(random.random())
+        payload = request.get_json()
+        print(f" args:{payload}")
+        return {"artist": "song"}
+        
+    except Exception as e:  # noqa
+        print(f"error: {e}")
+        return {'message': e}
+
+
+if __name__ == '__main__':
+    # noinspection PyTypeChecker
+    app.run(host='0.0.0.0', port=9090)

Review Comment:
   *hardcoded_bind_all_interfaces:*  Possible binding to all interfaces.
   
   ---
   
   <details><summary><b>ℹī¸ Learn about @sonatype-lift commands</b></summary>
   
   You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
   | **Command** | **Usage** |
   | ------------- | ------------- |
   | `@sonatype-lift ignore` | Leave out the above finding from this PR |
   | `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
   | `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
   
   **Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
   <sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
   
   
   
   ---
   
   Was this a good recommendation?
   [ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=349074067&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=349074067&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349074067&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349074067&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=349074067&lift_comment_rating=5) ]



##########
tests/e2e/case/profiling/greenlet/start_gevent.sh:
##########
@@ -0,0 +1,3 @@
+set -ex

Review Comment:
   *[SC2148](https://github.com/koalaman/shellcheck/wiki/SC2148):*  Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
   
   ---
   
   <details><summary><b>ℹī¸ Learn about @sonatype-lift commands</b></summary>
   
   You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
   | **Command** | **Usage** |
   | ------------- | ------------- |
   | `@sonatype-lift ignore` | Leave out the above finding from this PR |
   | `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
   | `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
   
   **Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
   <sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
   
   
   
   ---
   
   Was this a good recommendation?
   [ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=349074181&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=349074181&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349074181&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349074181&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=349074181&lift_comment_rating=5) ]



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] westarest commented on pull request #246: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
westarest commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1289932710

   > thank you, it is the docs what I need


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] Superskyyy commented on pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
Superskyyy commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1300746616

   Generally looks good to me. @kezhenxu94 can you take a look? I'm not very familiar with the profiling feature.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] westarest commented on pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
westarest commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1304391643

   > We will need documentation. The original documentation for threading-profiling was posted to here https://skywalking.apache.org/blog/2021-09-12-skywalking-python-profiling/, now with the addition of Greenlet Profiler, it will be better to move it back to our repository and add the corresponding part for greenlet. @westarest, can you work on the docs in the next PR?
   
   no problem. I can post a document about the difference between the Threading profiler  and the Greenlet profiler that I committed. 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] wu-sheng commented on pull request #246: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1289927745

   Verifying through e2e profiling case would be a good place to start.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] westarest commented on pull request #246: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
westarest commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1289906717

   it is a draft, but it can work with threading model and greenlet model automatically now. it needs more things:
   test cases
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] westarest commented on pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
westarest commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1295203701

   there is something wrong when profiling start:
   ```
   skywalking [Thread-127] [DEBUG] profile task [1666974131787_ZTJlLXNlcnZpY2UtcHJvdmlkZXI=.1] for endpoint [/artist] started
   Traceback (most recent call last):
     File "src/gevent/_waiter.py", line 122, in gevent._gevent_c_waiter.Waiter.switch
     File "/skywalking-python/venv/lib/python3.6/site-packages/apache_skywalking-0.8.0-py3.6.egg/skywalking/profile/profile_context.py", line 329, in callback
       agent.add_profiling_snapshot(snapshot)
     File "/skywalking-python/venv/lib/python3.6/site-packages/apache_skywalking-0.8.0-py3.6.egg/skywalking/agent/__init__.py", line 288, in add_profiling_snapshot
       __snapshot_queue.put(snapshot)
     File "/usr/local/lib/python3.6/queue.py", line 126, in put
       with self.not_full:
     File "/usr/local/lib/python3.6/threading.py", line 240, in __enter__
       return self._lock.__enter__()
     File "src/gevent/_semaphore.py", line 278, in gevent._gevent_c_semaphore.Semaphore.__enter__
     File "src/gevent/_semaphore.py", line 279, in gevent._gevent_c_semaphore.Semaphore.__enter__
     File "src/gevent/_semaphore.py", line 180, in gevent._gevent_c_semaphore.Semaphore.acquire
     File "/skywalking-python/venv/lib/python3.6/site-packages/gevent/thread.py", line 121, in acquire
       acquired = BoundedSemaphore.acquire(self, blocking, timeout)
     File "src/gevent/_semaphore.py", line 180, in gevent._gevent_c_semaphore.Semaphore.acquire
     File "src/gevent/_semaphore.py", line 249, in gevent._gevent_c_semaphore.Semaphore.acquire
     File "src/gevent/_abstract_linkable.py", line 521, in gevent._gevent_c_abstract_linkable.AbstractLinkable._wait
     File "src/gevent/_abstract_linkable.py", line 487, in gevent._gevent_c_abstract_linkable.AbstractLinkable._wait_core
     File "src/gevent/_abstract_linkable.py", line 490, in gevent._gevent_c_abstract_linkable.AbstractLinkable._wait_core
     File "src/gevent/_abstract_linkable.py", line 442, in gevent._gevent_c_abstract_linkable.AbstractLinkable._AbstractLinkable__wait_to_be_notified
     File "src/gevent/_abstract_linkable.py", line 451, in gevent._gevent_c_abstract_linkable.AbstractLinkable._switch_to_hub
     File "src/gevent/_greenlet_primitives.py", line 61, in gevent._gevent_c_greenlet_primitives.SwitchOutGreenletWithLoop.switch
     File "src/gevent/_greenlet_primitives.py", line 64, in gevent._gevent_c_greenlet_primitives.SwitchOutGreenletWithLoop.switch
     File "src/gevent/_greenlet_primitives.py", line 67, in gevent._gevent_c_greenlet_primitives.SwitchOutGreenletWithLoop.switch_out
     File "src/gevent/_greenlet_primitives.py", line 68, in gevent._gevent_c_greenlet_primitives.SwitchOutGreenletWithLoop.switch_out
   gevent.exceptions.BlockingSwitchOutError: Impossible to call blocking function in the event loop callback
   2022-10-28T16:22:34Z <built-in method switch of gevent._gevent_cgreenlet.Greenlet object at 0x7ff213196a48> failed with BlockingSwitchOutError
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] kezhenxu94 merged pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged PR #246:
URL: https://github.com/apache/skywalking-python/pull/246


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] Superskyyy commented on a diff in pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
Superskyyy commented on code in PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#discussion_r1010607918


##########
.github/workflows/CI.yaml:
##########
@@ -223,6 +223,10 @@ jobs:
             path: tests/e2e/case/http/e2e.yaml
           - name: Kafka
             path: tests/e2e/case/kafka/e2e.yaml
+          - name: proflinig_threading

Review Comment:
   ```suggestion
             - name: profiling_threading
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] wu-sheng commented on pull request #246: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1289930500

   Profiling e2e has been included in the main repo, https://github.com/apache/skywalking/blob/317d539371c588c4754029134ad76087ecb6d0cd/.github/workflows/skywalking.yaml#L435-L454 powered by our e2e-infra tool, https://skywalking.apache.org/docs/skywalking-infra-e2e/next/readme/
   
   You could learn how we run tests using the latest agent with real OAP/SkyWalking backend, and verify the result through SkyWalking CLI through this tool.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] sonatype-lift[bot] commented on a diff in pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#discussion_r1008674975


##########
tests/e2e/case/profiling/provider/provider.py:
##########
@@ -0,0 +1,40 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+import time
+import random
+from flask import Flask, request
+
+app = Flask(__name__)
+   
+@app.route('/artist', methods=['POST'])
+def artist():
+    try:
+
+        time.sleep(random.random())
+        payload = request.get_json()
+        print(f" args:{payload}")
+        return {"artist": "song"}
+        
+    except Exception as e:  # noqa
+        print(f"error: {e}")
+        return {'message': e}
+
+
+if __name__ == '__main__':
+    # noinspection PyTypeChecker
+    app.run(host='0.0.0.0', port=9090)

Review Comment:
   *hardcoded_bind_all_interfaces:*  Possible binding to all interfaces.
   
   ---
   
   <details><summary><b>ℹī¸ Learn about @sonatype-lift commands</b></summary>
   
   You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
   | **Command** | **Usage** |
   | ------------- | ------------- |
   | `@sonatype-lift ignore` | Leave out the above finding from this PR |
   | `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
   | `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
   
   **Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
   <sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
   
   
   
   ---
   
   Was this a good recommendation?
   [ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=349164446&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=349164446&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349164446&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349164446&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=349164446&lift_comment_rating=5) ]



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] Superskyyy commented on pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
Superskyyy commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1303905192

   We need to update `changes.md` to reflect the new feature. And also we will need documentation. The original documentation for threading-profiling was posted to here https://skywalking.apache.org/blog/2021-09-12-skywalking-python-profiling/, now with the addition of Greenlet Profiler, it will be better to move it back to our repository and add the corresponding part for greenlet. @westarest, can you add the docs in the next PR?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] Superskyyy commented on pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
Superskyyy commented on PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#issuecomment-1303899718

   The sonatype bug can be ignored, since it's testing code. LGTM.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-python] kezhenxu94 commented on a diff in pull request #246: feat: add GreenletProfiler

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #246:
URL: https://github.com/apache/skywalking-python/pull/246#discussion_r1014540376


##########
skywalking/profile/profile_context.py:
##########
@@ -34,6 +34,24 @@
 from skywalking.utils.time import current_milli_time
 
 
+THREAD_MODEL = 'thread'
+try:
+    from gevent import monkey
+    import greenlet
+    from gevent.exceptions import BlockingSwitchOutError
+
+    if monkey.is_module_patched('threading'):
+        if greenlet.__version__ < '2.0.0':

Review Comment:
   This looks error prone. Can you use the `packaging` module to parse and compare the version?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org