-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(logs): Ensure client & scope can be passed to log functions #16874
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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
size-limit report 📦
|
@cursor
cursor
bot
left a comment
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: Type Guard Fails with Undefined or Array Arguments
The isTemplateArgs type guard, which relies on Array.isArray(args[1]), is flawed. If TypeScript types are bypassed at runtime, this can lead to incorrect argument classification:
- If
messageParams(the second argument for template logging) isundefined, it's incorrectly treated as parameterized arguments, causing argument misalignment where subsequent parameters are misinterpreted. - If
attributes(the second argument for parameterized logging) is an array, it's incorrectly treated as template arguments, leading to runtime errors during destructuring.
packages/node-core/src/logs/capture.ts#L43-L46
sentry-javascript/packages/node-core/src/logs/capture.ts
Lines 43 to 46 in bff9360
Was this report helpful? Give feedback by reacting with 👍 or 👎
Should we make the additional args via an object so we can add args later without users needing to pass client/scope?
Sentry.logger.trace('log message', {}, { client, scope });
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 need to update cloudflare and vercel-edge as well.
It's not ideal that this is duplicated so much, we'll eventually want to move things to @sentry/core, but right now there is an export collision with the sdk debug logger.
Should we make the additional args via an object so we can add args later without users needing to pass client/scope?
Sentry.logger.trace('log message', {}, { client, scope });
No strong feelings, I have also thought about this a bit... WDYT @AbhiPrasad ?
No strong feelings, I have also thought about this a bit... WDYT
I would be open to an object, I think the bundle size hit is minuscule and it does make the API cleaner.
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 thought about this more, and I think we only need to accept a scope (because we can call scope.getClient() to get the client).
Maybe we can get this merged and backported to v9?
This adds support to provide a custom client & scope to log
captureXXmethods:This way, multi-client setups can work as well.
This also adds a size limit entry for browser + logs.