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

feat(logger): adopted Utility class & updated unit tests #550

Merged
merged 15 commits into from
Feb 28, 2022
Merged

feat(logger): adopted Utility class & updated unit tests #550

merged 15 commits into from
Feb 28, 2022

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Feb 10, 2022 •
edited
Loading

Description of your changes

This PR introduces changes to the Logger module to adopt the forthcoming Utility class that will be released in the @aws-lambda-powertools/commons package.

  • Install released @aws-lambda-powertools/commons package
  • Make changes to the code to use the Utility class
  • Run unit & e2e tests
  • Add note in the docs about initialising utility outside of function handler (ref)

How to verify this change

See results of PR checks, also see the screenshot below that shows a successful e2e test run (on local):
WIP

Optionally, re-run the e2e tests on GitHub Actions

Related issues, RFCs

#484
#547

PR status

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

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • 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

Breaking change checklist

N/A

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

@dreamorosi dreamorosi added enhancement logger This item relates to the Logger Utility on-hold This item is on-hold and will be revisited in the future labels Feb 10, 2022
@dreamorosi dreamorosi added this to the production-ready-release milestone Feb 10, 2022
@dreamorosi dreamorosi self-assigned this Feb 10, 2022
@dreamorosi dreamorosi linked an issue Feb 10, 2022 that may be closed by this pull request
Bug: cold start flag is set to true in subsequent invocations in Logger #518
Closed
@dreamorosi dreamorosi removed the on-hold This item is on-hold and will be revisited in the future label Feb 17, 2022
@dreamorosi dreamorosi mentioned this pull request Feb 24, 2022
Closed
@dreamorosi dreamorosi force-pushed the feat/logger/adopt_utility_common_cold_start branch from d4b93be to aaecc99 Compare February 24, 2022 15:12
@dreamorosi dreamorosi marked this pull request as ready for review February 28, 2022 10:54
@dreamorosi
Copy link
Contributor Author

This PR also includes changes to package.json file and some of the GitHub Actions workflows according to what described in this issue npm/cli#4475

saragerion
saragerion reviewed Feb 28, 2022
View reviewed changes
docs/core/logger.md
@@ -44,6 +44,8 @@ For a **complete list** of supported environment variables, refer to [this secti

#### Example using AWS Serverless Application Model (SAM)

The `Logger` utility is instantiated outside of the Lambda handler. In doing this, the same instance can be used across multiple invocations inside the same execution environment. This allows `Metrics` to be aware of things like whether or not a given invocation had a cold start or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this paragraph!

Suggestion #1 (minor): I understand what we are trying to achieve with this paragraph, but I don't know if it's entirely clear what is the advantage of reusing the instance on multiple invocation here. The Metrics reference can also be confusing since the Logger has the cold start logic too.

Suggestion #2 (minor): I'd move this text outside of the SAM section as this does not relate to SAM specifically. Maybe it can be formatted as inline block to make it more visible?
https://squidfunk.github.io/mkdocs-material/reference/admonitions/?h=warning#inline-blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes total sense, I'll open a separate PR after this one is merged as the same "notice" appears also in Tracer & Metrics which have already been merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#604

saragerion
saragerion approved these changes Feb 28, 2022
View reviewed changes
Copy link
Contributor

@saragerion saragerion left a comment

Choose a reason for hiding this comment

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

LGTM, added 2 minor comments

@dreamorosi dreamorosi merged commit 48f3487 into main Feb 28, 2022
@dreamorosi dreamorosi deleted the feat/logger/adopt_utility_common_cold_start branch February 28, 2022 13:35
dreamorosi added a commit that referenced this pull request Feb 28, 2022
@dreamorosi
build(deps-dev): bump esbuild from 0.14.x to 0.14.23 (#589)
6a9ebac 
* build(deps-dev): bump esbuild from 0.14.x to 0.14.23

* feat(logger): adopted Utility class & updated unit tests (#550)

* feat: adopted Utility class & updated unit tests

* docs: added notice in docs

* WIP

* build: added commons dependency

* deps: fixed lock

* rebuilt lock after rebase

* chore: update commented

* set explicit packages order

* amend lock

* run with older

* removed commons from workspace

* build: added foreground-scripts flag to CI

* fix: issue with workflows

* fix: lock file

* build(deps-dev): bump esbuild from 0.14.x to 0.14.23
@dreamorosi dreamorosi mentioned this pull request Feb 28, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flochaz flochaz flochaz approved these changes

@saragerion saragerion saragerion approved these changes

@ijemmy ijemmy Awaiting requested review from ijemmy

Assignees

@dreamorosi dreamorosi

Labels
logger This item relates to the Logger Utility
Projects
None yet
Milestone
production-ready-release
Development

Successfully merging this pull request may close these issues.

Bug: cold start flag is set to true in subsequent invocations in Logger
3 participants
@dreamorosi @flochaz @saragerion