-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(core): Rewrite logger
to avoid side effects
#16897
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
Conversation
size-limit report 📦
|
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.
I wonder if we should rename logger
to sentryLogger
as well.
This would allow us to export the sentry logging logger
from @sentry/core
as well.
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.
Bug: Logger Singleton Issue in Non-DEBUG Builds
In non-DEBUG builds, _getLoggerSettings()
incorrectly returns a new { enabled: false }
object on each call instead of using the global singleton. This breaks the logger's state management: enable()
and disable()
become no-ops as they modify a discarded temporary object, and isEnabled()
always returns false
. This behavior differs from the previous implementation, which correctly maintained the logger's enabled
state even when logging was disabled.
packages/core/src/utils/logger.ts#L128-L135
sentry-javascript/packages/core/src/utils/logger.ts
Lines 128 to 135 in c82b4e8
function _getLoggerSettings(): { enabled: boolean } { | |
if (!DEBUG_BUILD) { | |
return { enabled: false }; | |
} | |
return getGlobalSingleton('loggerSettings', () => ({ enabled: false })); | |
} |
packages/core/src/utils/logger.ts#L76-L87
sentry-javascript/packages/core/src/utils/logger.ts
Lines 76 to 87 in c82b4e8
function enable(): void { | |
_getLoggerSettings().enabled = true; | |
} | |
function disable(): void { | |
_getLoggerSettings().enabled = false; | |
} | |
function isEnabled(): boolean { | |
return _getLoggerSettings().enabled; | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Going to merge this in so I can work on #16901 |
This has bugged me often already, and I looked into it again nudged by #16846. Today the logger export from core has a side effect and reads from the global. This is a bit weird...
This PR rewrites this to instead only keep the enabled flag on the carrier. The logger itself is a "normal" method that just looks at the carrier to see if logging is enabled or not.