Initial implementation of the agent duplicated and adapted from the i… #2

mirelap-amazon · 2021-01-05T12:47:06Z

Issue #, if available:

Description of changes:
Initial implementation of the agent duplicated and adapted from the internal repository.

Testing:

Run all tests locally with pytest (including integration with local AWS credentials).
When pushed on my personal fork, the commit triggered a GitHub Workflow action that run the unit and acceptance tests, see here the results.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…nternal repository.

gimki

I have left some comments. I would not be bothered to have those changes in this PR. I am happy for you to push this as initial commits but I would suggest to do a clean up before we making it open source. We may possibly open source this when we release v1.0.3 onto PyPI with new features + cleanup. My comments are minor really but also something that I think we should clean up before making it visible to the community. Feel free to merge it; the comments I left are for future use really. Thanks

gimki · 2021-01-05T19:33:08Z

codeguru_profiler_agent/sampling_utils.py

+
+def _maybe_append_time_sleep(result, frame, line_no):
+    line = linecache.getline(frame.f_code.co_filename, line_no).strip()
+    if "sleep(" in line:


Do we mind exposing this hardcoded hack here?

The behaviour is already clear for a customer with this use-case, so I don't see any reason to not include this.

gimki · 2021-01-05T19:40:22Z

codeguru_profiler_agent/aws_lambda/lambda_handler.py

+            customer_handler_function = bootstrap_module._get_handler(original_handler_name)
+        else:
+            # Support for python 3.6 bootstrap which is different. It is starting to be quite hacky at this point.
+            # We will need to discuss if it is worth handling this case and if we can do it in a better way.


Should we rephrase comments like these(?)

Make it like a TODO or FIXME instead (?)

I'll add a TODO FIXME.

gimki · 2021-01-06T11:15:43Z

.github/workflows/python-package.yml

+  push:
+    branches: [ main ]
+  pull_request:
+    branches: [ main ]


I am not sure I can link this back to any part of the doc on https://docs.github.com/en/free-pro-team@latest/actions/guides/building-and-testing-python (?)

The first link from https://docs.github.com/en/free-pro-team@latest/actions/guides/building-and-testing-python#starting-with-the-python-workflow-template for "Python workflow template" redirects to a sample that shows this configuration. I generated the initial file by using the default wizard from GitHub.

gimki · 2021-01-06T11:47:34Z

codeguru_profiler_agent/model/sample.py

@@ -0,0 +1,12 @@
+class Sample:


(Not this PR) TODO: Make use of slot for this class

Will do, interesting.

gimki · 2021-01-07T11:32:45Z

codeguru_profiler_agent/profiler.py

+# [B108:hardcoded_tmp_directory] Probable insecure usage of temp file/directory.
+# https://bandit.readthedocs.io/en/latest/plugins/b108_hardcoded_tmp_directory.html
+# This file can be used by the customer as kill switch for the Profiler.
+KILLSWITCH_FILEPATH = "/var/tmp/killProfiler" #nosec


Note: This would not work for Windows

Will add a TODO FIXME.

gimki · 2021-01-07T12:34:22Z

test/conftest.py

+import pytest
+
+# Enable pytestutils @focus decoration
+pytest_plugins = "test.pytestutils_focus"


Similar question: Do we need to keep this?

The file is needed to allow other decorations (like @before) from pytestutils, but I will remove @Focus we can use pytest to run a specific folder or file and considering I haven't found a big utility of @Focus yet.

gimki · 2021-01-07T12:35:08Z

test/integration/test_live_backend_reporting.py

+
+
+@pytest.mark.skipif(
+    # TODO FIXME Remove the conditions for skipping this on Amazonian fleets when we move fully to GitHub.


We may need to address that in cleanup commit next

Yes, will update, thanks.

gimki · 2021-01-07T12:39:24Z

test/unit/aws_lambda/test_lambda_handler.py

+                                                      context="expected_context") == "expected result"
+
+    class TestLoadModuleFunction:
+        class TestWhenPython38LambdaBootstrapCalls:


Question: Do we need to control that it is run for python38 only (?) I know it doesnt matter but it is a bit weird to run that for all version of python given that it said python38 explicitly.

I think here is more about the Python38 implementation, not necessary the version we are using. I don't see it worth it to update it considering it passes for all versions.

gimki · 2021-01-07T12:46:30Z

test/unit/test_profiler_runner.py

+        self.mock_collector.add.assert_not_called()
+
+
+def wait_until(predicate, max_wait_seconds=1, period_seconds=0.1):


I think we refactored this out to utils.

Yes, I'll update to use the existing wait_for from help_utils, thanks.

gimki · 2021-01-07T12:48:03Z

test/unit/reporter/test_agent_configuration_merger.py

+            assert AgentConfiguration.get().sampling_interval == timedelta(milliseconds=1)
+            assert AgentConfiguration.get().minimum_time_reporting == timedelta(seconds=1)
+            assert AgentConfiguration.get().reporting_interval == timedelta(minutes=1)
+            assert AgentConfiguration.get().max_stack_depth == 998


mirelap-amazon · 2021-01-08T11:41:15Z

As discussed, none of this was considered blocking, so I merged like this and handle the comments in the next PR, here. Thanks a lot for your review! @gimki

Initial implementation of the agent duplicated and adapted from the i…

c3974eb

…nternal repository.

mirelap-amazon self-assigned this Jan 5, 2021

mirelap-amazon marked this pull request as ready for review January 5, 2021 12:48

mirelap-amazon requested review from a team and gimki and removed request for gimki January 5, 2021 12:54

mirelap-amazon added 2 commits January 5, 2021 14:48

Updated documentation to include the other released agent we have.

2e6d1f4

Remove file for releasing the lambda layer, not needed here.

1962ae1

gimki approved these changes Jan 7, 2021

View reviewed changes

mirelap-amazon merged commit 5acbd7d into aws:main Jan 7, 2021

mirelap-amazon mentioned this pull request Jan 8, 2021

Multiple minor clean-ups from previous PR. #3

Merged

mirelap-amazon assigned gimki Jan 8, 2021

mirelap-amazon requested a review from a team January 8, 2021 12:13



		@pytest.mark.skipif(
		# TODO FIXME Remove the conditions for skipping this on Amazonian fleets when we move fully to GitHub.

		self.mock_collector.add.assert_not_called()


		def wait_until(predicate, max_wait_seconds=1, period_seconds=0.1):

Initial implementation of the agent duplicated and adapted from the i… #2

Initial implementation of the agent duplicated and adapted from the i… #2

Uh oh!

Conversation

mirelap-amazon commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gimki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirelap-amazon commented Jan 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mirelap-amazon commented Jan 5, 2021 •

edited

Loading