feat(debugging): Support debugging Golang functions. #565

jfuss · 2018-07-19T19:17:27Z

Golang Debugging was initally implemented in #495, but reverted in #526
due to functional and integration tests failures. This commit reverts #526
and adds in the testing gaps. The specific differences are:

_get_debug_context method was replaces with the bool/__nonzero (py2)
Added additional unit tests for lambda_container to cover added methods
Updated Functional tests
Added debug_context support in start-lambda

Issue #, if available:
#281

Description of changes:

Run debugger locally with a go function

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Golang Debugging was initally implemented in aws#495, but reverted in aws#526 due to functional and integration tests failures. This commit reverts aws#526 and adds in the testing gaps. The specific differences are: * _get_debug_context method was replaces with the __bool__/__nonzero (py2) * Added additional unit tests for lambda_container to cover added methods * Updated Functional tests * Added debug_context support in start-lambda

* Added the command for building a Go binary for debugging * Added notes being explict about <output path> for dlv and --debugger-path

jfuss · 2018-07-20T20:22:34Z

I have successfully debugged my first Go application. I update the README.rst for the Golang Debugging section to include extra steps I needed to do to get debugging working in VSCode.

I am considering updating the README in the Golang example we generate from sam init --runtime go, to include some of this information on debugging. What is your thought on that @sanathkr and @austinlparker?

Also, shout out to @austinlparker for helping me get debugging working and test everything :)

austinlparker · 2018-07-20T20:32:58Z

README.rst

+as the debugger, and wrap your function with it at runtime. The debugger
+is run in headless mode, listening on the debug port.
+
+When debugging, you must compile you function in debug mode:


Small typo; Second you should be your.

austinlparker · 2018-07-20T20:35:57Z

I think some updates to the Golang example might be useful, yeah. I wouldn't say it's blocking a release though.

sanathkr

Looks good overall. Two questions: why get_debug_context still exists, and why requests library is version pinned?

sanathkr · 2018-07-20T21:24:22Z

README.rst

+You must compile `delve` to run in the container and provide its local path
+via the `--debugger-path` argument. Build delve locally as follows:
+
+`GOARCH=amd64 GOOS=linux go build -o <output path>/dlv github.com/derekparker/delve/cmd/dlv`


Can you change <output path> to something like <delve folder path> to be more descriptive?

I can. I still want to keep the /dlv at the end since it needs to be that name (right now).

yeah /dlv is fine..

sanathkr · 2018-07-20T21:24:50Z

README.rst

+The following is an example launch configuration for Visual Studio Code to
+attach to a debug session.
+
+.. code:: bash


json language not bash

sanathkr · 2018-07-20T21:26:52Z

requirements/dev.txt

@@ -8,7 +8,7 @@ pylint==1.7.2
 pytest==3.0.7
 py==1.4.33
 mock==2.0.0
-requests>=2.19.0
+requests==2.19.0


These are test requirements and without pinning we fail some tests because the request version is in the user-agent headers: https://github.com/awslabs/aws-sam-cli/blob/develop/tests/functional/local/apigw/test_local_apigw_service.py#L629

"headers": {"Host": "0.0.0.0:33651", "User-Agent": "python-requests/2.19.0", ...

Requests library adds these headers. I had different versions of requests in my local Py3 vs Py2.

I noticed this too. We should fix the tests to ignore these headers and not pin the versions here. Pip installation will break if we pin

I would rather handle this outside of this PR. Pip should not break for pinning (we already do this for pytest, py, mock, etc) in the dev.txt requirements. For developing with the cli, we recommend pyenv to allow multiple python versions for testing. Devs should not be installing this into an existing pyenv. I think the concern here is valid but very low for 'break if we pin'.

Ahh my bad, I thought this was the base requirements.txt. Dev requirements are okay to be pinned

sanathkr · 2018-07-20T21:27:47Z

samcli/commands/local/invoke/cli.py

+    :param string debugger_path: Path to debugger on host
+    :return DebugContext:
+    """
+    if not debug_port or debug_args or debugger_path:


you don't need this method anymore, do you?

sanathkr · 2018-07-20T21:34:54Z

samcli/local/docker/lambda_container.py

+
+        return {
+            debug_options.debugger_path: {
+                "bind": "/tmp/lambci_debug_files",


Can you move this static string to a class member? Also add comments explaining if this is the path in local file system or inside docker container

sanathkr · 2018-07-20T21:39:12Z

tests/unit/commands/local/invoke/test_cli.py

        event_data = "data"
        get_event_mock.return_value = event_data

+        debug_context_data = DebugContext(debug_port=self.debug_port, debug_args=self.debug_args,
+                                          debugger_path=self.debugger_path)
+        get_debug_context_mock.return_value = debug_context_data


This needs to change because get_debug_context method won't exist

will update the test where needed.

sanathkr · 2018-07-20T21:39:32Z

tests/unit/commands/local/lib/test_debug_context.py

+        self.assertEquals(context.debug_args, 'debug_args')
+
+    @parameterized.expand([
+        ('1000', 'debuggerpath', 'debug_args'),


good thought to test with port number as strings :)

sanathkr · 2018-07-20T21:40:26Z

tests/unit/local/docker/test_lambda_container.py


        self.assertIsNotNone(result)
+
+
+class TestLambdaContainer_get_additional_options(TestCase):


jfuss · 2018-07-20T22:51:14Z

samcli/local/docker/lambda_container.py

+            return None
+
+        return {
+            debug_options.debugger_path: {


One other thing I ran into is if the debugger_path is a relative path, this breaks.

Command:
sam local start-api -d 5858 --debugger-path ./sam-app

Failure (start-api works but when invoking I get):
docker.errors.APIError: 400 Client Error: Bad Request ("create ./sam-app: "./sam-app" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path")

The right approach here is to use pathlib.Path(debug_options.debugger_path).resolve() (encapsulate this inside the DebugContext of course). This will resolve the path to the absolute path. My only concern here is windows and docker integrations. We are already running into path issues with Windows and Docker Toolbox and adding this Path resolving will only make that issue more complex. Short term resolving the path is right but we should really bring all these path resolving through some common library to make our lives a little easier long term.

What are your thoughts here @sanathkr?

Using pathlib is fine for now. If you want to be future proof, create a separate helper method to resolve Docker paths in Windows that is a no-op today. We can implement this method in a separate PR

* Removed unneeded methods * Refactored some constants in lambda_container * README updates/corrections

In the Go runtime Container (might be others as well), the container handles the timeout of the function. For debug sessions that last longer than the timeout, the container will halt. Now, we set the timeout to 10 hours when we are in a debug session.

sanathkr

LGTM except for one readme comment.

Also, can you handle relative paths to dlv file in this PR before I approve it?

sanathkr · 2018-07-23T20:26:39Z

README.rst

+
+`GOARCH=amd64 GOOS=linux go build -o <delve folder path>/dlv github.com/derekparker/delve/cmd/dlv`
+
+NOTE: The output path needs to end in `/dlv`. The docker container will expect the dlv binary to be in the <output path>


need to replace here too

missed one :(

jfuss · 2018-07-23T20:28:43Z

Also, can you handle relative paths to dlv file in this PR before I approve it?

Yup, already working on it :)

jfuss · 2018-07-24T14:14:51Z

Adding some additional validation on the debugger_path to make for a better experience and fail fast. Hoping to have a new revision here soon

…ameter

sanathkr · 2018-07-25T19:30:54Z

samcli/commands/local/cli_common/invoke_context.py

+                debugger = Path(debugger_path).resolve(strict=True)
+            except OSError as error:
+                if error.errno == errno.ENOENT:
+                    raise DebugContextException("'debugger_path' could not be found.")


Minor comment: Instead of the string 'debugger_path', can you add the actual value of variable debugger_path to the message? like '/usr/loca/ is not found'

sanathkr

Very minor cchanges. Otherwise looks good

sanathkr · 2018-07-25T19:31:45Z

samcli/commands/local/cli_common/invoke_context.py

+
+            # We turn off pylint here due to https://github.com/PyCQA/pylint/issues/1660
+            if not debugger.is_dir():  # pylint: disable=no-member
+                raise DebugContextException("'debugger_path' should be a directory with the debugger in it.")


have the actual value of debugger path in error message

sanathkr

Awesome! Thanks for all the iterations :)

jfuss · 2018-07-25T19:51:20Z

@austinlparker Did all the heavy lifting :)

austinlparker · 2018-07-25T19:56:17Z

Y’all too kind. 😛

…

Sent from my iPhone

On Jul 25, 2018, at 3:51 PM, Jacob Fuss ***@***.***> wrote: @austinlparker Did all the heavy lifting :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

@mhart

* fix: Functional tests must run on localhost to work in Windows (#552) * fix: spacing typo in Log statement in start-lambda (#559) * docs: Fix syntax highlighting in README.md (#561) * docs: Change jest to mocha in Nodejs init README (#564) * docs: Fix @mhart link in README (#562) * docs(README): removed cloudtrail, added SNS to generate-event (#569) * docs: Update repo name references (#577) * feat(debugging): Fixing issues around debugging Golang functions. (#565) * fix(init): Improve current init samples around docs and fixes (#558) * docs(README): Update launch config to SAM CLI from SAM Local (#587) * docs(README): Update sample code for calling Local Lambda Invoke (#584) * refactor(init): renamed handler for camel case, moved callback call up (#586) * chore: aws-lambda-java-core 1.1.0 -> 1.2.0 for java sam init (#578) * feat(validate): Add profile and region options (#582) Currently, `sam validate` requires AWS Creds (due to the SAM Translator). This commits adds the ability to pass in the credientials through a profile that is configured through `aws configure`. * docs(README): Update README prerequisites to include awscli (#596) * fix(start-lambda): Remove Content-Type Header check (#594) * docs: Disambiguation "Amazon Kinesis" (#599) * docs: Adding instructions for how to add pyenv to your PATH for Windows (#600) * docs: Update README with small grammar fix (#601) * fix: Update link in NodeJS package.json (#603) * docs: Creating instructions for Windows users to install sam (#605) * docs: Adding a note directing Windows users to use pipenv (#606) * fix: Fix stringifying λ environment variables when using Python2 (#579) * feat(generate-event): Added support for 50+ events (#612) * feat(invoke): Add region parameter to all invoke related commands (#608) * docs: Breaking up README into separate files to make it easier to read (#607) * chore: Update JVM size params to match docker-lambda (#615) * feat(invoke): Invoke Function Without Parameters through --no-event (#604) * docs: Update advanced_usage.rst with clarification on --env-vars usage (#610) * docs: Remove an extra word in the sam packaging command (#618) * UX: Improves event names to reflect Lambda Event Sources (#619) * docs: Fix git clone typo in installation docs (#630) * docs(README): Callout go1.x runtime support (#631) * docs(installation): Update sam --version command (#634) * chore(0.6.0): SAM CLI Version bump (#635)

jfuss added 2 commits July 19, 2018 12:04

docs: Update README for Go Debugging

4e54437

* Added the command for building a Go binary for debugging * Added notes being explict about <output path> for dlv and --debugger-path

jfuss requested a review from sanathkr July 20, 2018 20:19

austinlparker reviewed Jul 20, 2018

View reviewed changes

Fix spelling

e18e939

sanathkr suggested changes Jul 20, 2018

View reviewed changes

jfuss commented Jul 20, 2018

View reviewed changes

Addressed PR feedback

e0319fd

* Removed unneeded methods * Refactored some constants in lambda_container * README updates/corrections

burck1 mentioned this pull request Jul 21, 2018

Debugging dotnet core apps with the SAM CLI #568

Closed

sanathkr suggested changes Jul 23, 2018

View reviewed changes

Addressed PR feedback and added early validation on debugger_path par…

c6674cd

…ameter

sanathkr reviewed Jul 25, 2018

View reviewed changes

sanathkr suggested changes Jul 25, 2018

View reviewed changes

Update DebugContextExceptions to include path information

34a267e

sanathkr approved these changes Jul 25, 2018

View reviewed changes

Merge branch 'develop' into golang-debug

41a7b6c

sanathkr merged commit 0e0f815 into aws:develop Jul 25, 2018

jackmcguire1 mentioned this pull request Mar 8, 2019

Debugging go functions works with VS Code but not Goland #886

Closed


		self.assertIsNotNone(result)


		class TestLambdaContainer_get_additional_options(TestCase):


		`GOARCH=amd64 GOOS=linux go build -o <delve folder path>/dlv github.com/derekparker/delve/cmd/dlv`

		NOTE: The output path needs to end in `/dlv`. The docker container will expect the dlv binary to be in the <output path>

feat(debugging): Support debugging Golang functions. #565

feat(debugging): Support debugging Golang functions. #565

Conversation

jfuss commented Jul 19, 2018 • edited

jfuss commented Jul 20, 2018

austinlparker Jul 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinlparker commented Jul 20, 2018

sanathkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss commented Jul 23, 2018

jfuss commented Jul 24, 2018

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment

jfuss commented Jul 25, 2018

austinlparker commented Jul 25, 2018 via email

jfuss commented Jul 19, 2018 •

edited

austinlparker Jul 20, 2018 •

edited