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

Bug: Log Level SILENT must be set before instantiation #1503

Closed
zirkelc opened this issue Jun 15, 2023 · 6 comments
Closed

Bug: Log Level SILENT must be set before instantiation #1503

zirkelc opened this issue Jun 15, 2023 · 6 comments
Assignees
Labels
discussing The issue needs to be discussed, elaborated, or refined logger This item relates to the Logger Utility not-a-bug New and existing bug reports incorrectly submitted as bug

Comments

@zirkelc
Copy link
Contributor

zirkelc commented Jun 15, 2023

Expected Behaviour

I expected the log level to be a toggle that is being evaluated at the time a message is being printed. That means if I set process.env.LOG_LEVEL="SILENT", the message should not be printed and if I reset the log level the message should be printed.

const logger = new Logger({ serviceName: "serverlessAirline" });
logger.info("INFO"); // should log 

process.env.LOG_LEVEL = "SILENT";
logger.info("SILENT"); // should not log

Current Behaviour

The log level is evaluated at the time of instantiation. That means changing the log level after the logger was created, has no effect and will still print messages.

This prints:

const logger = new Logger({ serviceName: "serverlessAirline" });
logger.info("INFO"); // prints

process.env.LOG_LEVEL = "SILENT";
logger.info("SILENT"); //  prints

And this doesn't print:

process.env.LOG_LEVEL = "SILENT";
const logger = new Logger({ serviceName: "serverlessAirline" });
logger.info("INFO"); // doesn't print
logger.info("SILENT"); // doesn't print

Code snippet

See sandbox: https://codesandbox.io/s/muddy-bush-j54nwp?file=/src/index.ts

Steps to Reproduce

See sandbox: https://codesandbox.io/s/muddy-bush-j54nwp?file=/src/index.ts

Possible Solution

The process.env.LOG_LEVEL variable should always be checked when a message is being printed. If the process.env.LOG_LEVEL differs from the constructor logLevel, the environment variable should have priority.

Alternatively, the if the constructor was was called without logLevel, then the log level from process.env.LOG_LEVEL, should always have priority.

Powertools for AWS Lambda (TypeScript) version

1.7

AWS Lambda function runtime

18.x

Packaging format used

npm

Execution logs

No response

@zirkelc zirkelc added triage This item has not been triaged by a maintainer, please wait bug Something isn't working labels Jun 15, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jun 15, 2023

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

@dreamorosi
Copy link
Contributor

dreamorosi commented Jun 15, 2023

Hi @zirkelc thank you for taking the time to open the issue.

The behavior you describe is intended.

You have multiple ways of setting the e log level, both when instantiating the Logger and at runtime.

One of them is via environment variables, which together with the logLevel parameter passed to the constructor, is always evaluated upon instance creation and stored in the class.

If you want to change the log level after that, aka at runtime, then you can do so using the setLogLevel() method. When you change the log level using the method, all logs emitted after that should comply with that level. The feature was released/added in the last release (1.9.0)

@dreamorosi dreamorosi added logger This item relates to the Logger Utility discussing The issue needs to be discussed, elaborated, or refined not-a-bug New and existing bug reports incorrectly submitted as bug and removed bug Something isn't working triage This item has not been triaged by a maintainer, please wait labels Jun 15, 2023
@dreamorosi dreamorosi self-assigned this Jun 15, 2023
@zirkelc
Copy link
Contributor Author

zirkelc commented Jun 15, 2023

Hi @dreamorosi thanks for the quick response!

I understand, I thought changing the environment variable LOG_LEVEL should have the same effect as changing the log level via the class method setLogLevel().

In some case I don't have access to the logger instance, because the logger is nested in other functions which I don't have access to. In these cases it would be good to have a flag to temporarily switch the log level.

A similar discussion happened a few months ago: #1198
I think evaluating the LOG_LEVEL variable at log-time would allow to turn off the logs during tests without having the need to silence the test framework completely, like it is currently needed for Jest with --silent.

@dreamorosi
Copy link
Contributor

I'm not sure I'm following, if you don't have access to the logger then how are you logging?

Regarding to that discussion, we decided to support local development via another environment variable called POWERTOOLS_DEV.

Overall I don't think evaluating the log level every time we need to print a log is a good idea for performance, and generally speaking I don't consider environment variables something that should change at runtime in most cases.

I think it'd help if you could share a few more details on the use case and what you're trying to do, maybe I can help find an alternative.

@zirkelc
Copy link
Contributor Author

zirkelc commented Jun 15, 2023

I meant that the logger instance is created inside functions that are called, but those functions do not export the logger instance itself.

Nonetheless, I thought it might be a bug that changing LOG_LEVEL has no effect, but if it is intended that way, it is fine. Thanks for the clarification!

@zirkelc zirkelc closed this as completed Jun 15, 2023
@github-project-automation github-project-automation bot moved this from Working on it to Coming soon in AWS Lambda Powertools for TypeScript Jun 15, 2023
@github-actions
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussing The issue needs to be discussed, elaborated, or refined logger This item relates to the Logger Utility not-a-bug New and existing bug reports incorrectly submitted as bug
Projects
None yet
Development

No branches or pull requests

2 participants