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

feat(logger): introduce loglevel trace #1589 #2902

Merged
merged 14 commits into from
Aug 15, 2024

Conversation

timo92
Copy link
Contributor

@timo92 timo92 commented Aug 8, 2024

Summary

Changes

This PR introduces a new log level TRACE below 'DEBUG' in alignment with the AWS Lambda ALC.

Issue number: closes #1589


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

@timo92 timo92 requested review from a team as code owners August 8, 2024 07:52
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation logger This item relates to the Logger Utility tests PRs that add or change tests labels Aug 8, 2024
@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label Aug 8, 2024
Copy link

boring-cyborg bot commented Aug 8, 2024

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

@timo92
Copy link
Contributor Author

timo92 commented Aug 8, 2024

I performed some refactorings of the exisiting code in the last two commits. Let me know if I should move them into a seperate PR.
So far I was unable to run the tests locally due to Cannot find module '@aws-lambda-powertools/commons'. Will try again in case some of the merge checks fail.

@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Aug 8, 2024
@dreamorosi dreamorosi added the do-not-merge This item should not be merged label Aug 8, 2024
@dreamorosi
Copy link
Contributor

Hi @timo92, thanks for the quick PR.

Looking at the test results it seems that we have two issues:

  • Some of the new tests are failing because the console.trace() method seems to be undefined (see here)
  • Test coverage is below 100% (see here) - I'd suggest we look at this after resolving the previous one.

The first one is quite interesting, and I think it will require a bit more time on this PR.

Before getting into its details, a bit of context is needed. By default, Logger does not use the global console object but instead instantiates its own Console (#748). The only case when we use the global one is when customers set the POWERTOOLS_DEV environment variable to true, which as the name suggests it's a behavior we encourage only during development.

According to the Node.js docs:

The global console is a special Console whose output is sent to process.stdout and process.stderr.

Even though this is not explicitly called out in the docs, the global console has a lot more methods than the parent Console - this includes the trace() method (docs), and it's also is why the unit tests are failing.

My first idea was to suggest we patch the this.console object and add a new trace method that simply logs whatever it's provided. This could work for when the Logger runs on AWS Lambda, and it's roughly equivalent to what the Lambda service actually does in the Node.js managed runtime (see here).

This however requires some extra consideration because in Node.js the console.trace() method is supposed to behave differently than the other one, and instead of just logging its arguments it's supposed to log a formatted message and stack trace to the current position in the code, for example:

console.trace('Show me');
// Prints: (stack trace will vary based on where trace is called)
//  Trace: Show me
//    at repl:2:9
//    at REPLServer.defaultEval (repl.js:248:27)
//    at bound (domain.js:287:14)
//    at REPLServer.runBound [as eval] (domain.js:300:12)
//    at REPLServer.<anonymous> (repl.js:412:12)
//    at emitOne (events.js:82:20)
//    at REPLServer.emit (events.js:169:7)
//    at REPLServer.Interface._onLine (readline.js:210:10)
//    at REPLServer.Interface._line (readline.js:549:8)
//    at REPLServer.Interface._ttyWrite (readline.js:826:14)

This is clearly not what we want, however it means we will need to also modify the logging behavior when POWERTOOLS_DEV is enabled, and document all this so that people are clear on the behavior being aligned with Lambda rather than expecting a stack trace.

--

Side note 1: If you want a faster feedback loop, you can setup the local dev environment, by running npm run setup-local which will install & build the packages. This is not mandatory, and you can continue pushing the changes here and we'll run them in CI.

Side note 2: The scope of this task unexpectedly grew beyond what was going to be a simple change. We are more than happy to work with you and iterate on the PR, however if you feel like it's not what you thought and want to close the PR we completely understand and will put the issue back in the backlog.

@boring-cyborg boring-cyborg bot added the dependencies Changes that touch dependencies, e.g. Dependabot, etc. label Aug 8, 2024
@timo92
Copy link
Contributor Author

timo92 commented Aug 8, 2024

Hey @dreamorosi,

thank you for the infos and links. I have adjusted the PR to patch console with a trace method which simply forwards the arguments to console.log which should exist on any console implementation. I would defer adding the documentation until we have decided that this is an appropriate workaround.

Regarding local development:
I had executed npm run setup-local and npm run build locally but the errors still exist on my windows machine. I changed to github codespaces for the changes I just commited where everything worked as expected.

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.

Hi @timo92, apologies for the delayed review.

I have left a couple of minor comments and a suggestion. After they are addressed I think we can merge this right away.

packages/logger/tests/unit/Logger.test.ts Outdated Show resolved Hide resolved
packages/logger/src/Logger.ts Outdated Show resolved Hide resolved
packages/logger/package.json Outdated Show resolved Hide resolved
@dreamorosi dreamorosi removed the do-not-merge This item should not be merged label Aug 14, 2024
Copy link

@dreamorosi dreamorosi self-requested a review August 15, 2024 14:36
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 working on this and for iterating on the PR with us. Appreciate it!

@dreamorosi dreamorosi merged commit 650252c into aws-powertools:main Aug 15, 2024
9 checks passed
Copy link

boring-cyborg bot commented Aug 15, 2024

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@timo92 timo92 deleted the 1589-feat-add-log-level-trace branch August 15, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Changes that touch dependencies, e.g. Dependabot, etc. documentation Improvements or additions to documentation feature PRs that introduce new features or minor changes logger This item relates to the Logger Utility size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: add additional log level: TRACE & corresponding logger.trace() method
3 participants