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): add silent log level to suppress the emission of all logs #1347

Merged
merged 5 commits into from
Mar 3, 2023

Conversation

shdq
Copy link
Contributor

@shdq shdq commented Feb 28, 2023

Description of your changes

This PR introduces a new logLevel value SILENT. When this level is set, either via constructor or via environment variable, the utility does not emit any log regardless of where it's running.

Constructor example:

const logger = new Logger({ logLevel: 'SILENT' });

Environment variable example:

process.env.LOG_LEVEL = 'SILENT';
const logger = new Logger();

This behavior is desirable for those customers who would like to have the ability to instrument their code with logs, but optionally have the chance to suppress them based on their requirements (see #1198 (comment) for example).

Resolves #1198.

How to verify this change

N/A

Related issues, RFCs

Issue number: #1198

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
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

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

@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label Feb 28, 2023
@shdq
Copy link
Contributor Author

shdq commented Feb 28, 2023

It's also worth mentioning in a tip box near the logLevel in docs or in testing your code section with a description of particular use case.

@dreamorosi
Copy link
Contributor

It's also worth mentioning in a tip box near the logLevel in docs or in testing your code section with a description of particular use case.

I agree that we should update the docs but I'm still unsure about where it should go.

At the moment I'm leaning towards this not being a test-only use case, especially if we keep in mind the use case that influenced the feature being green lit.

If that's okay for you, let's start with the implementation and unit tests, and maybe also adding the new value to this table. This will give me some more time to think about how to frame this new feature.

@shdq shdq marked this pull request as ready for review March 1, 2023 05:53
@@ -169,7 +203,7 @@ describe('Class: Logger', () => {

// Prepare
const logger: Logger = createLogger({
logLevel: 'ERROR',
logLevel: 'SILENT',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed to the highest log level when it doesn't print to stdout from any method and still prints when the sample rate is 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate on this one? Not sure I'm following

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case consists of two tests: testing with and w/o sample rate.

For the first one, it is needed to show that no logs emit with the higher log level. The error log level was used to show that it doesn't print to stdout from all methods and is ignored on the error method:

expect(consoleSpy).toBeCalledTimes(method === 'error' ? 1 : 0)

Now we have silence as the highest level, so I changed to it.

The second one test that all logs emit with sample rate for every method even with the highest log level.

I pointed out this, because this is a dynamic set of tests, and it wasn't obvious to me what is "higher" level means in the description, and why it uses a particular log level. So I thought it worth mentioning in the review.

@shdq
Copy link
Contributor Author

shdq commented Mar 1, 2023

This will give me some more time to think about how to frame this new feature.

Take your time and then let me know!

@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Mar 1, 2023
docs/core/logger.md Outdated Show resolved Hide resolved
@dreamorosi
Copy link
Contributor

Regarding the docs, I think we should add a new section under "Advanced", and right after "Sampling logs", titled "Silencing logs":

The SILENT log level is a quick and effective way to suppress all log messages without having to modify your code. When this log level is set, all logs, regardless of severity, are suppressed. This is useful in those cases when you want to have your code instrumented to produce logs, but due to some requirement or business decision, you prefer to not emit them. By setting the log level to SILENT, either via the logLevel constructor option, or via the LOG_LEVEL environment variable, you can easily suppress all logs on demand.

Then also add a "Note" box/callout that says:

Note that using the SILENT log level can make it harder to debug and monitor your application, so we recommend to use this log level judiciously.

(Housekeeping) Also, since you're editing that file already, could you please fix these two:

  • the typo at this line - it should be function's invocation instead of function"s invocation (single quote).
  • the line highlights of this code snippet

@shdq
Copy link
Contributor Author

shdq commented Mar 3, 2023

You found a good place for this feature in the docs. Good job, Andrea! I paraphrased the text a bit. I hope it's suitable and you like it.

@dreamorosi dreamorosi self-requested a review March 3, 2023 14:44
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.

Looking great, thanks for the help on this one.

Let's 🚢 it!

@dreamorosi dreamorosi merged commit c82939e into aws-powertools:main Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature PRs that introduce new features or minor changes size/L PRs between 100-499 LOC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Silent mode for all testing environments
2 participants