-
Notifications
You must be signed in to change notification settings - Fork 2
fix: incorrect print level for the DefaultLogger #134
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
base: master
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR fixes critical logging issues in the DefaultLogger class, correcting incorrect console method usage and log level handling that was causing all messages to appear as errors with misleading prefixes.
- Replaces incorrect static prefix assignment with dynamic level-based prefixes
- Fixes console method selection to use appropriate methods (error, warn, info, debug) instead of always using console.error
- Removes unnecessary single-argument suppression and simplifies the logging logic
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Introduces a logLevelIndices map for more efficient log level lookups and simplifies the DefaultLogger implementation. This change improves performance and code readability by avoiding repeated array searches.
please help me to take a look @cre8ivejp |
it will look like `destroy finished Timeout`
this.featureFlagProcessor?.stop(); | ||
this.segementUsersCacheProcessor?.stop(); | ||
this.config.logger?.info('destroy finished', this.registerEventsScheduleID); | ||
this.config.logger?.info('destroy finished'); |
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.
it was like
destroy finished Timeout {
_idleTimeout: -1,
_idlePrev: null,
_idleNext: null,
_idleStart: 5616,
_onTimeout: null,
_timerArgs: undefined,
_repeat: 30000,
_destroyed: true,
[Symbol(refed)]: true,
[Symbol(kHasPrimitive)]: false,
[Symbol(asyncId)]: 644,
[Symbol(triggerId)]: 0
}
It made me feel like something bad had happened, but it hadn’t.
I found this issue while investigating an error in OpenFeature Node.js e2e
I saw a log with the prefix “error,” but it was not actually an error—it was a misleading message caused by incorrect logic in the DefaultLogger..
Issues with the Original
DefaultLogger
ImplementationIncorrect Console Method Usage
All log messages, regardless of level (
debug
,info
,warn
,error
), were output usingconsole.error
. This caused all logs to appear as errors, even when they were informational or debug messages.Log Message Suppression
The logger did not output messages if only a single argument was provided (e.g.,
logger.error('something')
), due to an unnecessaryargs.length === 1
check.Static Prefix Assignment
The log message prefix was set based on the logger’s configured level, not the actual level of the message being logged. This could result in misleading prefixes.
Unnecessary Complexity
The use of a separate
write()
method added complexity and made the code harder to maintain, especially since the logic could be handled more simply and clearly.