Skip to content
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

fix: Invoke stderr stream missing stack trace #5972

Merged
merged 18 commits into from
Sep 26, 2023

Conversation

mildaniel
Copy link
Contributor

@mildaniel mildaniel commented Sep 21, 2023

Which issue(s) does this change fix?

N/A

Why is this change necessary?

After updating sam local commands to use the RIE in version 1.13.1, when a stack trace is streamed from the emulation container it contains carriage return characters that then don't get displayed by the stream writer.

How does it address the issue?

Replaces the carriage return characters with the system's linesep character.

What side effects does this change have?

Will display addition logs when using local invoke commands that contain a stack trace.

Before:

Mounting C:\Users\Administrator\ws\sam-app\.aws-sam\build\HelloWorldFunction as /var/task:ro,delegated, inside runtime container
START RequestId: 06dd2e5e-af4c-47b0-b893-a9979544e244 Version: $LATEST
    "body": son.dumps({y", line 38, in lambda_handler
END RequestId: 06dd2e5e-af4c-47b0-b893-a9979544e244
REPORT RequestId: 06dd2e5e-af4c-47b0-b893-a9979544e244  Init Duration: 0.15 ms  Duration: 155.80 ms     Billed Duration: 156 ms Memory Size: 128 MB     Max Memory Used: 128 MB

After:

Mounting C:\Users\Administrator\ws\sam-app\.aws-sam\build\HelloWorldFunction as /var/task:ro,delegated, inside runtime container
START RequestId: 5ff38d1a-03ed-426a-8a4e-aef0b37bf0de Version: $LATEST
[ERROR] NameError: name 'son' is not defined
Traceback (most recent call last):
  File "/var/task/app.py", line 38, in lambda_handler
    "body": son.dumps({
END RequestId: 5ff38d1a-03ed-426a-8a4e-aef0b37bf0de
REPORT RequestId: 5ff38d1a-03ed-426a-8a4e-aef0b37bf0de  Init Duration: 0.32 ms  Duration: 110.39 ms     Billed Duration: 111 ms Memory Size: 128 MB     Max Memory Used: 128 MB

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

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

@mildaniel mildaniel requested a review from a team as a code owner September 21, 2023 18:49
@mildaniel mildaniel changed the title fix: Invoke stack trace loggs missing fix: Invoke stderr stream missing stack trace Sep 22, 2023
Copy link
Contributor

@mndeveci mndeveci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this issue! LGTM in general, left a small comment

samcli/local/docker/container.py Show resolved Hide resolved
Copy link
Contributor

@sriram-mv sriram-mv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me.

Please add a comment in the code which draws attention to this change with something like "Important! DO NOT CHANGE", and an explanation of why. Because the decode, encode can seem like weird behavior.

@mildaniel mildaniel added this pull request to the merge queue Sep 26, 2023
Merged via the queue into aws:develop with commit cc23e4c Sep 26, 2023
76 checks passed
@mildaniel mildaniel deleted the fix-invoke-stack-trace-logging branch October 3, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants