-
Notifications
You must be signed in to change notification settings - Fork 36
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
fix(serverless): resolved printing debug logs by default #1317
Conversation
087d666
to
1431660
Compare
packages/core/src/logger.js
Outdated
parentLogger = consoleLogger; | ||
// No custom logger has been provided via config, we create a new minimal logger with pino. | ||
// @ts-ignore | ||
parentLogger = pino({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is indeed a problem with falling back to a "minimal" logger.
A soon as you require the core module, all required files call getLogger
on the top of the file. Thats why we have added this fallback. This needs to be refactored away, because it does not respect any log level. I'd suggest to raise an issue for that. But IMO this is not the cause of the problem.
IMO we are seeing debug logs by default from any serverless package because we do not forward their logger!
We define which logger we want to use for the pkg (logger from serverless pkg).
We normalize the config (which I also do think is very wrong!, but lets ignore that. Please add a comment.) and then we do not forward the logger in the config:
https://github.com/instana/nodejs/blob/v3.17.1/packages/core/src/index.js#L69
https://github.com/instana/nodejs/blob/v3.17.1/packages/core/src/logger.js#L43
The shortest quick fix (without having too much to refactor) is IMO for every serverless pkg:
const config = normalizeConfig({});
config.logger = logger;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code. I will create a card for refactor the logging.
1431660
to
5d40dda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/aws-fargate/src/activate.js
Outdated
|
||
function init() { | ||
if (process.env.INSTANA_DEBUG || process.env.INSTANA_LOG_LEVEL) { | ||
logger.setLevel(process.env.INSTANA_DEBUG ? 'debug' : process.env.INSTANA_LOG_LEVEL); | ||
const logLevel = process.env.INSTANA_DEBUG === 'true' ? 'debug' : process.env.INSTANA_LOG_LEVEL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding, we don't have to set the INSTANA_DEBUG to 'true' or anything. We can just set the value as say test
|| 1
or any "TRUTHY" values for the debugger to be enabled.
Find the public doc here
You can also configure the log level by setting the environment variable INSTANA_LOG_LEVEL to either debug, info, warn or error. Finally, setting INSTANA_DEBUG to any non-empty string will set the log level to debug.
This applies to all the similar changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find!
We had trouble on Aryas laptop because she could not turn off debug mode.
Let me revert that for now and add a comment. Not sure we need to change something or not.
refs #1316