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.

Sign up for GitHub

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

Jump to bottom

fix(logger): fix clearstate bug when lambda handler throws #1045

Merged
merged 4 commits into from
Aug 10, 2022
Merged

fix(logger): fix clearstate bug when lambda handler throws #1045

merged 4 commits into from
Aug 10, 2022

Conversation

saragerion
Copy link
Contributor

@saragerion saragerion commented Aug 9, 2022 •
edited by dreamorosi
Loading

Description of your changes

This PR introduces a bug fix for issue #1027.
Previously, when using addPersistentLogAttributes or appendKeys and an error was thrown inside the Lambda handler function, the clear state functionality did not work.
This PR includes the business logic fix and updated unit tests.

How to verify this change

See checks passing, including e2e tests run.

Related issues, RFCs

Issue number: #1027

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

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

@github-actions github-actions bot added the bug Something isn't working label Aug 9, 2022
@saragerion
Copy link
Contributor Author

saragerion commented Aug 9, 2022 •
edited
Loading

E2E tests run:
https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/2828668395

@saragerion saragerion marked this pull request as ready for review August 9, 2022 22:55
@saragerion saragerion self-assigned this Aug 9, 2022
@dreamorosi dreamorosi added the logger This item relates to the Logger Utility label Aug 10, 2022
dreamorosi
dreamorosi previously approved these changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! Left one comment in the unit tests.

packages/logger/tests/unit/Logger.test.ts Outdated Show resolved Hide resolved
dreamorosi
dreamorosi previously approved these changes Aug 10, 2022
View reviewed changes
packages/logger/tests/unit/middleware/middy.test.ts Outdated Show resolved Hide resolved
dreamorosi
dreamorosi previously approved these changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing the comments!

ijemmy
ijemmy previously approved these changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@ijemmy ijemmy left a comment

Choose a reason for hiding this comment

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

LGTM.

I have one suggestion but it's subjective opinion on readability.

packages/logger/src/Logger.ts Outdated Show resolved Hide resolved
packages/logger/src/middleware/middy.ts Outdated Show resolved Hide resolved
@saragerion
test(logger): improve readability of injectLambdaContextBefore & inje…
99c33e4 
…ctLambdaContextAfterOrOnError function behaviour - separation of concerns
@saragerion saragerion dismissed stale reviews from ijemmy and dreamorosi via 99c33e4 August 10, 2022 12:55
@saragerion
Copy link
Contributor Author

E2E tests passing: https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/2832787138

@saragerion saragerion linked an issue Aug 10, 2022 that may be closed by this pull request
Bug: clearState not working when lambda throws error #1027
Closed
@saragerion saragerion merged commit 5ebd1cf into main Aug 10, 2022
@saragerion saragerion deleted the 1027-bug-logger-clearstate branch August 10, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamorosi dreamorosi dreamorosi approved these changes

@ijemmy ijemmy ijemmy approved these changes

@flochaz flochaz Awaiting requested review from flochaz

Assignees

@saragerion saragerion

Labels
bug Something isn't working logger This item relates to the Logger Utility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: clearState not working when lambda throws error
3 participants
@saragerion @ijemmy @dreamorosi