-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Add logger to core and allow scope to be passed log methods #17698
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
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
f909fb6
to
5e90e01
Compare
5e90e01
to
e8359c4
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.
LGTM, just a general question regarding the scope
parameter: AFAICT, we only use it to get the scope's client. No complaints with that but I'm wondering if you chose to go with scope to potentially extract other scope data as well in the future? If not, wouldn't it be easier to let users pass in the client directly?
Just a thought, as I said, the scope approach is fine.
@@ -1,11 +1,8 @@ | |||
import { feedbackAsyncIntegration } from './feedbackAsync'; | |||
import { feedbackSyncIntegration } from './feedbackSync'; | |||
import * as logger from './log'; |
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.
does this mean we can remove log.ts
from browser?
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.
We use the scope to grab tracing info + user attributes: sentry-javascript/packages/core/src/logs/internal.ts Lines 134 to 142 in e8359c4
The variable is just called |
Supercedes #16874
This PR makes two changes
logger
as an export to@sentry/core
, and then refactors thebrowser
,cloudflare
andvercel-edge
packages to just re-export@sentry/core
's logger.This change makes it easy to use logging in an isomorphic way, and reduces duplication between our various packages. We couldn't change the export from
node-core
because it has a different type signature than the standard logger.