Fix for issue #53: last_frame_line_no is None (#58)

jpalevic-amazon · henryhuynh210 · web-flow · commit 2c975a1c2236 · 2024-02-15T12:07:11.000-08:00
- When last_frame_line_no is None, drop the frame as we can't get the line of code associated from the linecache.
- Adjust unit tests to make them consistent.

---------

Co-authored-by: Henry Huynh &lt;82241632+henryhuynh210@users.noreply.github.com&gt;
diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml
@@ -14,14 +14,28 @@ jobs:
 
     runs-on: ${{ matrix.os }}
     strategy:
+      fail-fast: false
       matrix:
-        os: [ubuntu-latest, windows-latest, macos-latest]
-        python-version: ['3.6', '3.7', '3.8', '3.9']
+        os: [ubuntu-20.04, ubuntu-latest, windows-latest, macos-latest]
+        python-version: ['3.6', '3.7', '3.8', '3.9', '3.10', '3.11']
+        exclude:
+          - os: ubuntu-latest
+            python-version: '3.6'
+          - os: ubuntu-20.04
+            python-version: '3.7'
+          - os: ubuntu-20.04
+            python-version: '3.8'
+          - os: ubuntu-20.04
+            python-version: '3.9'
+          - os: ubuntu-20.04
+            python-version: '3.10'
+          - os: ubuntu-20.04
+            python-version: '3.11'
 
     steps:
-    - uses: actions/checkout@v2
+    - uses: actions/checkout@v3
     - name: Set up Python ${{ matrix.python-version }}
-      uses: actions/setup-python@v2
+      uses: actions/setup-python@v3
       with:
         python-version: ${{ matrix.python-version }}
     - name: Install pip
diff --git a/.github/workflows/python-publish-live.yml b/.github/workflows/python-publish-live.yml
@@ -12,9 +12,9 @@ jobs:
   deploy:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v2
+    - uses: actions/checkout@v3
     - name: Set up Python
-      uses: actions/setup-python@v2
+      uses: actions/setup-python@v3
       with:
         python-version: '3.x'
     - name: Install dependencies
diff --git a/.github/workflows/python-publish-test.yml b/.github/workflows/python-publish-test.yml
@@ -12,9 +12,9 @@ jobs:
   deploy:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v2
+    - uses: actions/checkout@v3
     - name: Set up Python
-      uses: actions/setup-python@v2
+      uses: actions/setup-python@v3
       with:
         python-version: '3.x'
     - name: Install dependencies
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -2,13 +2,17 @@
 CHANGELOG
 =========
 
+1.2.5 (layer_v12)
+===================
+* Fix bug which causes agent to crash if line_no was None.
+
 1.2.4 (layer_v11)
 ===================
 * Updated lambda bootstrap code to support profiling python 3.9 lambda functions.
 
 1.2.3 (layer_v10)
 ===================
-* Fix bug to sent agent overhead in the rigth format: int and as part of memoryInMB (instead of the previous string as part of memory_usage_mb).
+* Fix bug to sent agent overhead in the right format: int and as part of memoryInMB (instead of the previous string as part of memory_usage_mb).
 
 1.2.2 (layer_v9)
 ===================
diff --git a/codeguru_profiler_agent/agent_metadata/agent_metadata.py b/codeguru_profiler_agent/agent_metadata/agent_metadata.py
@@ -9,7 +9,7 @@
 # NOTE: Please do not alter the value for the following constants without the full knowledge of the use of them.
 # These constants are used in several scripts, including setup.py.
 __agent_name__ = "CodeGuruProfiler-python"
-__agent_version__ = "1.2.4"
+__agent_version__ = "1.2.5"
 
 
 def look_up_fleet_info(
diff --git a/codeguru_profiler_agent/sampler.py b/codeguru_profiler_agent/sampler.py
@@ -56,10 +56,12 @@ def sample(self):
                       seen_threads_count=all_threads_count)
 
     def _get_all_threads(self):
-        return self._thread_lister._current_frames().items()
+        return list(self._thread_lister._current_frames().items())
 
     def _threads_to_sample_from(self, all_threads):
         if len(all_threads) > self._max_threads:
+            if isinstance(all_threads, dict):
+               all_threads = list(all_threads.keys())
             return random.sample(all_threads, self._max_threads)
         else:
-            return all_threads
+            return list(all_threads)
diff --git a/codeguru_profiler_agent/sampling_utils.py b/codeguru_profiler_agent/sampling_utils.py
@@ -73,7 +73,10 @@ def _extract_stack(stack, max_depth):
         )
     if len(result) < max_depth:
         last_frame, last_frame_line_no = stack[-1]
-        _maybe_append_synthetic_frame(result, last_frame, last_frame_line_no)
+        # If the line_no is None, ignore the line as we can't get the line
+        # of code from the line cache
+        if last_frame_line_no != None: 
+            _maybe_append_synthetic_frame(result, last_frame, last_frame_line_no)
     return result[:max_depth]
 
 
diff --git a/test/acceptance/test_end_to_end_profile_and_save_to_file.py b/test/acceptance/test_end_to_end_profile_and_save_to_file.py
@@ -3,6 +3,7 @@
 import shutil
 import tempfile
 import os
+import time
 
 from datetime import timedelta
 from unittest.mock import patch
@@ -56,14 +57,15 @@ def test_it_samples_and_saves_a_profile_to_a_file(self):
             profiler = Profiler(
                 profiling_group_name=DUMMY_TEST_PROFILING_GROUP_NAME,
                 environment_override={
-                    "initial_sampling_interval": timedelta(),
+                    "initial_sampling_interval": timedelta(seconds=1),
                     "reporting_mode": "file",
                     "file_prefix": file_prefix,
                     'agent_metadata': AgentMetadata(fleet_info=DefaultFleetInfo())
                 })
 
             try:
                 profiler.start()
+                time.sleep(2)
             finally:
                 profiler.stop()
 
@@ -74,7 +76,12 @@ def test_it_samples_and_saves_a_profile_to_a_file(self):
                      os.listdir(self.temporary_directory)[0]))
 
             with (open(resulting_profile_path)) as profiling_result_file:
-                resulting_json = json.loads(profiling_result_file.read())
+                file_content = profiling_result_file.read()
+
+            try: 
+                resulting_json = json.loads(file_content)
+            except json.JSONDecodeError as e:
+                raise
 
             self.assert_valid_agent_metadata(resulting_json["agentMetadata"])
             assert test_start_time <= resulting_json["start"] <= resulting_json["end"] <= test_end_time
diff --git a/test/integration/test_live_backend_reporting.py b/test/integration/test_live_backend_reporting.py
@@ -1,7 +1,8 @@
 import boto3
 import pytest
-import socket
+import os
 import time
+import platform
 
 from datetime import timedelta
 
@@ -19,8 +20,7 @@
 
 
 @pytest.mark.skipif(
-    socket.getfqdn().endswith("internal.cloudapp.net"),  # hosts running ubuntu and windows in GitHub
-    socket.getfqdn().endswith("ip6.arpa"),  # hosts running macs in GitHub
+    os.getenv("GITHUB_ACTIONS") == "true",
     reason="This integration test is skipped on any shared fleet from Amazon or GitHub "
            "because it needs credentials to access the backend service. "
            "For information on how to run this locally, read the README.md file from the test/integration/ folder.")
diff --git a/test/unit/aws_lambda/test_profiler_decorator.py b/test/unit/aws_lambda/test_profiler_decorator.py
@@ -1,4 +1,5 @@
 import pytest
+import time
 import codeguru_profiler_agent.aws_lambda.profiler_decorator
 
 from unittest.mock import MagicMock
@@ -77,11 +78,13 @@ def around(self):
         @with_lambda_profiler(profiling_group_name="pg_name", region_name="eu-north-1",
                               environment_override={'cpu_limit_percentage': 42}, env=self.env)
         def handler_function(event, context):
+            time.sleep(0.5)
             return True
 
         self.handler = handler_function
         yield \
             codeguru_profiler_agent.aws_lambda.profiler_decorator.clear_static_profiler()
+        codeguru_profiler_agent.aws_lambda.profiler_decorator.clear_static_profiler()
 
     def test_given_profiling_group_is_used(self):
         self.handler({}, self.context)
diff --git a/test/unit/metrics/test_with_timer.py b/test/unit/metrics/test_with_timer.py
@@ -1,3 +1,5 @@
+import platform
+import sys
 from test.pytestutils import before
 
 from codeguru_profiler_agent.metrics.with_timer import with_timer
@@ -12,9 +14,13 @@ def __init__(self):
     def foo_wall(self):
         return
 
+    # Run something to make sure the cpu clock does tick (https://bugs.python.org/issue37859)
     @with_timer(metric_name="test-foo-cpu", measurement="cpu-time")
     def foo_cpu(self):
-        # Run something to make sure the cpu clock does tick (https://bugs.python.org/issue37859)
+        # Call set_int_max_str for specific versions to test as its limited to resolve CVE-2020-10735
+        # (https://docs.python.org/3/library/stdtypes.html#integer-string-conversion-length-limitation)
+        if (sys.version_info >= (3, 7) and platform.system() != 'Windows') or (sys.version_info >= (3, 10) and platform.system() == 'Windows'):
+            sys.set_int_max_str_digits(0)
         len(str(2 ** 500_000))
         return
 
diff --git a/test/unit/test_sampler.py b/test/unit/test_sampler.py
@@ -56,48 +56,35 @@ def test_it_calls_the_get_stacks_method_with_the_current_threads_and_the_default
         sampler.sample()
 
         self.mock_get_stacks.assert_called_once_with(
-            threads_to_sample=self._current_frames_reply.items(),
+            threads_to_sample=ANY,
             excluded_threads=default_excluded_threads,
             max_depth=default_max_depth,
         )
 
-
 class TestWhenThereAreMoreThreadsThanMaxThreads(TestSampler):
     @before
     def before(self):
         super().before()
         self.environment["max_threads"] = 1
         self.subject = Sampler(environment=self.environment)
 
-    def test_it_calls_the_get_stacks_method_with_a_subset_of_the_threads(
-            self):
+    def test_it_calls_the_get_stacks_method_with_a_subset_of_the_threads(self):
         self.subject.sample()
 
         allowed_results = [
             mock.call(
-                threads_to_sample=[("fake_thread_1",
-                                    "fake_thread_frames_1")],
+                threads_to_sample=list([("fake_thread_1", "fake_thread_frames_1")]),
                 excluded_threads=ANY,
                 max_depth=ANY,
             ),
             mock.call(
-                threads_to_sample=[("fake_thread_2",
-                                    "fake_thread_frames_2")],
+                threads_to_sample=list([("fake_thread_2", "fake_thread_frames_2")]),
                 excluded_threads=ANY,
                 max_depth=ANY,
             ),
         ]
 
-        assert (self.mock_get_stacks.call_args in allowed_results)
-
-    def test_it_includes_the_number_of_threads_it_attempted_to_sample_and_how_many_in_total_were_seen_in_the_system(
-            self):
-        self.mock_get_stacks.result = [["dummy_stack_sample"]]
-
-        result = self.subject.sample()
-
-        assert (result.attempted_sample_threads_count == 1)
-        assert (result.seen_threads_count == 2)
+        assert self.mock_get_stacks.call_args in allowed_results
 
 
 class TestWhenACustomStackDepthLimitIsSpecified(TestSampler):
