sentry-javascript
sentry-javascript copied to clipboard
feat(node): Move default option handling from `init` to `NodeClient`
While working on https://github.com/getsentry/sentry-javascript/pull/15307, I noticed that for the Node SDK init, in contrast to most other init functions, we have a lot of logic in the init function itself, instead of handling stuff in the NodeClient itself. This is likely still that way since we moved the NodeClient stuff over to OTEL, where a bunch of this was initially re-implemented.
Now, this has a few problems:
- It makes it rather hard to follow what happens in
init(), as there is a lot going on. - There is some unnecessary re-implementation, e.g. we do not use
initAndBindwhich we have in other places. - If you use the NodeClient manually, you'll actually be missing a bunch of things that it could do.
Possibly relevant changes:
client.startClientReportTracking()is now called in the NodeClient constructor - so unlesssendClientReports: falseis configured, this will always be setup now, even for manual clients.Failed to register ESM hookwarning is now aconsole.warn, not using the logger - this means we do not need to manually enable the logger before we callinitAndBindanymore.- spotlight is now properly handled like a default integration
Extracted this out of https://github.com/getsentry/sentry-javascript/pull/15307
size-limit report 📦
| Path | Size | % Change | Change |
|---|---|---|---|
| @sentry/browser | 23.95 kB | -0.01% | -2 B 🔽 |
| @sentry/browser - with treeshaking flags | 23.72 kB | -0.01% | -2 B 🔽 |
| @sentry/browser (incl. Tracing) | 38.27 kB | -0.01% | -2 B 🔽 |
| @sentry/browser (incl. Tracing, Replay) | 76.39 kB | -0.01% | -3 B 🔽 |
| @sentry/browser (incl. Tracing, Replay) - with treeshaking flags | 69.5 kB | -0.01% | -3 B 🔽 |
| @sentry/browser (incl. Tracing, Replay with Canvas) | 81.17 kB | -0.01% | -2 B 🔽 |
| @sentry/browser (incl. Tracing, Replay, Feedback) | 93.26 kB | -0.01% | -3 B 🔽 |
| @sentry/browser (incl. Feedback) | 40.72 kB | -0.01% | -3 B 🔽 |
| @sentry/browser (incl. sendFeedback) | 28.68 kB | -0.02% | -4 B 🔽 |
| @sentry/browser (incl. FeedbackAsync) | 33.56 kB | -0.02% | -4 B 🔽 |
| @sentry/react | 25.73 kB | - | - |
| @sentry/react (incl. Tracing) | 40.25 kB | -0.01% | -1 B 🔽 |
| @sentry/vue | 28.35 kB | -0.01% | -1 B 🔽 |
| @sentry/vue (incl. Tracing) | 40.1 kB | - | - |
| @sentry/svelte | 23.98 kB | -0.02% | -3 B 🔽 |
| CDN Bundle | 25.24 kB | -0.01% | -2 B 🔽 |
| CDN Bundle (incl. Tracing) | 38.38 kB | -0.02% | -4 B 🔽 |
| CDN Bundle (incl. Tracing, Replay) | 74.25 kB | -0.01% | -3 B 🔽 |
| CDN Bundle (incl. Tracing, Replay, Feedback) | 79.67 kB | -0.01% | -3 B 🔽 |
| CDN Bundle - uncompressed | 73.66 kB | -0.01% | -5 B 🔽 |
| CDN Bundle (incl. Tracing) - uncompressed | 113.69 kB | -0.01% | -5 B 🔽 |
| CDN Bundle (incl. Tracing, Replay) - uncompressed | 227.66 kB | -0.01% | -5 B 🔽 |
| CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed | 240.48 kB | -0.01% | -5 B 🔽 |
| @sentry/nextjs (client) | 41.91 kB | -0.01% | -1 B 🔽 |
| @sentry/sveltekit (client) | 38.75 kB | -0.01% | -2 B 🔽 |
| @sentry/node | 149.4 kB | -0.08% | -108 B 🔽 |
| @sentry/node - without tracing | 97.58 kB | -0.55% | -534 B 🔽 |
| @sentry/aws-serverless | 122.93 kB | -0.43% | -522 B 🔽 |
:x: Unsupported file format
Upload processing failed due to unsupported file format. Please review the parser error message:
Error parsing JUnit XML in /home/runner/work/sentry-javascript/sentry-javascript/packages/solidstart/vitest.junit.xml at 95:20 Caused by: RuntimeError: Error parsing XML Caused by: 0: ill-formed document: expected `</testsuites>`, but `</testcase>` was found 1: expected `</testsuites>`, but `</testcase>` was foundFor more help, visit our troubleshooting guide.
After these changes, spotlight will always be included in production bundles and release/environment/etc will automatically be pulled from environment variables at runtime which we don't want.
Ah, valid points!
- About spotlight, this could be alleviated by building custom
getDefaultIntegrationsinstead of using the ones from node - by combininggetDefaultIntegrationsWithoutPerformance()and possiblygetAutoPerformanceIntegrations()you should be able to tree-shake spotlight. We also do this e.g. for aws-serverless! - About the env. vars, is this problematic to be pulled from there, or simply "useless" because it will never be there? 🤔 Also generally, if we already set env/release/etc before that in the electron SDK it should not matter because anything passed in always takes precedence?
Anyways will move this to draft until we verified it will not interfere with Electron negatively!
RE Spotlight: Is having it in the server-side bundle a concern? It shouldn't do anything unless it's enabled by users, so it would primarily be "just" bloat
Correct and it doesn't look like it's a huge amount of code either.
It looks like spotlight can be enabled simply by setting the SENTRY_SPOTLIGHT environment variable? And if this variable is set, the integration will automatically post all Sentry envelopes to localhost? There are likely classes of app where this would be considered a problem.
this could be alleviated by building custom
getDefaultIntegrationsinstead of using the ones from node
As long as it can be excluded by default, I have no concerns.
if we already set env/release/etc before that in the electron SDK it should not matter because anything passed in always takes precedence?
We do set a default app-name@version release for Electron so this should take precedence. We would need to add tests to confirm there's no way to inadvertently bypass this though because leaking environment variables from machines would be bad. For example currently, because we merge user options with the defaults using object spread, if users set release as something falsey, this would result in the environment variables being used!
My preference would always be to not have code included when it's not used, but I can see why this needed changing. The options and config split across init and the client has always felt strange!
It will almost certainly make sense for Electron to have it's own client so we can exclude this unused code. Would this be a breaking change? We do currently export the NodeClient...