Configurable logging #3730
Configurable logging #3730
Conversation
Changed format of console logging. Added ability to turn off logging. Added option to turn off Sentry logging.
Ryszard-Trojnacki
requested review from
antsgar and
SayakaOno
and removed request for
a team
packages/api/src/common/logger.js
Outdated
| * @param value log level string | ||
| * @param def default log level | ||
| */ | ||
| function getLogLevel(value, def) { |
There was a problem hiding this comment.
Could we change def to default? I prefer not to shorten variable names because it can be confusing
There was a problem hiding this comment.
default is a JavaScript reserved keyword used in switch. It can't be used as a variable name.
There was a problem hiding this comment.
OK, I have changed this to defaultValue. I hope this is OK now.
There was a problem hiding this comment.
Ah, you're right! Ironically I think I found def confusing from how it's used in Python. Thanks for updating it!
packages/api/src/common/logger.js
Outdated
| if (typeof value !== 'string') return def; | ||
| value = value.toLowerCase().trim(); |
There was a problem hiding this comment.
NIT but we usually prefer to avoid inline conditionals
There was a problem hiding this comment.
Done and corrected in other places.
packages/api/src/common/logger.js
Outdated
| @@ -61,7 +101,7 @@ class SentryTransport extends Transport { | |||
| } | |||
|
|
|||
| // Report Errors to Sentry | |||
| if (process.env.NODE_ENV !== 'development') { | |||
| if (process.env.NODE_ENV !== 'development' && !isTrue(process.env.LOG_DISABLE_SENTRY)) { | |||
There was a problem hiding this comment.
The negation of isTrue is a bit hard to read IMO. Could we change the env var to LOG_ENABLE_SENTRY instead?
|
@Ryszard-Trojnacki left a few minor comments, thank you for working on this! |
Minor code readability fixes.
|
@antsgar I have included all your comments. |
Co-authored-by: Antonella Sgarlatta <[email protected]>
packages/api/src/common/logger.js
Outdated
| if (process.env.NODE_ENV !== 'production') { | ||
| const consoleLogLevel = getLogLevel( | ||
| process.env.LOG_CONSOLE_LEVEL, | ||
| process.env.NODE_ENV !== 'production' ? 'error' : 'off', |
There was a problem hiding this comment.
I'm so sorry for the late review Ryszard.
@antsgar
As we discussed in this Slack thread, we want to enable error logs in production. Could you confirm that:
process.env.NODE_ENV !== 'production' ? 'error' : 'off'
should instead just be 'error'?
Just checking before requesting any changes, especially since this PR has been open for a while.
Also, if we go ahead with this change, we would need to replace console.log used for error logging with console.error.
There was a problem hiding this comment.
I have changed the first part.
Also, if we go ahead with this change, we would need to replace console.log used for error logging with console.error.
As I understand you want me to change almost all calls to console.log to console.error?
There is also other option. I can change:
console.log = (...args) => logger.info.call(logger, ...args);to:
console.log = (...args) => logger.error.call(logger, ...args);and console.log would be by default logger.error.
There was a problem hiding this comment.
@Ryszard-Trojnacki Thank you so much for making the change and for your comment!
We actually discussed it this morning and are now thinking about setting LOG_CONSOLE_LEVEL to info, since we’ll want to support both info and log.
I also noticed that objects are being logged as undefined with the changes. I was thinking we might want to consider formatting the output using winston.format.printf (https://github.com/winstonjs/winston?tab=readme-ov-file#formats).
Do you happen to have any suggestions or alternatives that might work better?
Just for reference, here's the current log:
And here's how it looks with the changes:
Added possibility to configure LiteFarm logging.
Till now all logging configuration was hardcoded and only variable was
NODE_ENV. I have introduced new variables for logging:LOG_LEVEL- allows to setup main/file logging,LOG_CONSOLE_LEVEL- allows to setup console login,LOG_DISABLE_SENTRY- allows to disable Sentry logging.Also I have changed console logging format from JSON to cli, because it was totally unreadable.
Beside changing the level of logging, there is a possibility to turn off logging by setting
offvalue to parameter.LOG_CONSOLE_LEVEL=offallows to turn off console logging.New variables does not change the actual behavior.
Only change that is done by this patch is change of default format of console logging.
Type of change
I have updated
.env.defaultfile with variables.How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Tested all configuration options manually.
Checklist: