sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

feat(node): Move default option handling from `init` to `NodeClient`

Open mydea opened this issue 6 months ago • 4 comments

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:

  1. It makes it rather hard to follow what happens in init(), as there is a lot going on.
  2. There is some unnecessary re-implementation, e.g. we do not use initAndBind which we have in other places.
  3. 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 unless sendClientReports: false is configured, this will always be setup now, even for manual clients.
  • Failed to register ESM hook warning is now a console.warn, not using the logger - this means we do not need to manually enable the logger before we call initAndBind anymore.
  • spotlight is now properly handled like a default integration

Extracted this out of https://github.com/getsentry/sentry-javascript/pull/15307

mydea avatar May 21 '25 12:05 mydea

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 🔽

View base workflow run

github-actions[bot] avatar May 21 '25 12:05 github-actions[bot]

: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 found

For more help, visit our troubleshooting guide.

codecov[bot] avatar May 21 '25 12:05 codecov[bot]

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!

  1. About spotlight, this could be alleviated by building custom getDefaultIntegrations instead of using the ones from node - by combining getDefaultIntegrationsWithoutPerformance() and possibly getAutoPerformanceIntegrations() you should be able to tree-shake spotlight. We also do this e.g. for aws-serverless!
  2. 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!

mydea avatar May 23 '25 07:05 mydea

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 getDefaultIntegrations instead 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...

timfish avatar May 23 '25 11:05 timfish