feat(logs): Ensure client & scope can be passed to log functions #16874
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 📦
|
There was a problem hiding this comment.
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 }); |
No strong feelings, I have also thought about this a bit... WDYT @AbhiPrasad ? |
I would be open to an object, I think the bundle size hit is minuscule and it does make the API cleaner. |
| attributes?: Log['attributes'], | ||
| severityNumber?: Log['severityNumber'], | ||
| client?: Client, | ||
| scope?: Scope, |
There was a problem hiding this comment.
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?
|
any updates on this PR? we really want to use logging in our Chrome Extension and we're stuck because this PR hasn't been merged. Anything we can do to help expedite merging this PR? @mydea @timfish @AbhiPrasad |
|
This is a bit stale, opened #17698 to tackle this on a new branch. |
…ds (#17698) Supercedes #16874 This PR makes two changes 1. It adds `logger` as an export to `@sentry/core`, and then refactors the `browser`, `cloudflare` and `vercel-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. 2. It expands the logger exports to accept an optional scope argument. This allows for users to provide their own custom clients to the methods, which helps with standalone client cases. ```js import * as Sentry from "@sentry/browser"; const client = createMySentryClient(); const scope = new Sentry.Scope(); scope.setClient(client); Sentry.logger.info("Hello World!", {}, { scope }); ```
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.