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

mydea · 2025-05-21T12:14:55Z

While working on #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 initAndBind which 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 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 #15307

github-actions · 2025-05-21T12:21:11Z

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

codecov · 2025-05-21T12:28:19Z

❌ 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.

packages/core/src/utils-hoist/logger.ts

packages/node/src/sdk/client.ts

andreiborza · 2025-05-21T14:51:00Z

packages/node/src/sdk/index.ts

+ * This is an internal method the SDK uses under the hood to set up things - you should not use this as a user!
+ * Instead, use `init()` to initialize the SDK.
+ *
+ * @hidden
+ * @internal


This function isn't exported, do we need to mention it shouldn't be used?

ah yeah, I moved stuff around a bit here, you are right :)

AbhiPrasad

@timfish does this impact electron?

timfish

I need to think about how this will impact the Electron SDK. Currently we can skip over everything in init by simply using the Node client.

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.

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. * Env. vars will automatically be picked up and put on the current scope in the NodeClient constructor * `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

Co-authored-by: Andrei <[email protected]>

mydea · 2025-05-23T07:27:08Z

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 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!
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!

Lms24

I think this is generally fine but agree with waiting on checking for electron.

@timfish 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 (which I agree with is far from optimal but we lost that battle a long time ago for the Node SDK)

timfish · 2025-05-23T11:12:13Z

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

…init()` (#16354) Similar to #16353, this changes how default options for BrowserClient are handled. Instead of building this in `init`, we now do (some) of it in BrowserClient. Additionally, this also adjusts what we do if we detect a browser extension: Instead of skipping the setup, we now just disable the client. This streamlines this a bit and also ensures that we actually always return a `Client` from init. It also fixes the type for `BrowserOption` to actually allow to configure `cdnBaseUrl`, which was actually only set for the ClientOptions, oops. The reason for this is to streamline the `init` code, making it easier to extend/adjust it. Right now, there is a lot going on in different places there. By moving as much as we can (and makes sense) to `BrowserClient` this becomes a bit easier. While playing with different ways to handle the browser extension stuff, I ended up landing on just disabling the SDK in this scenario.

mydea self-assigned this May 21, 2025

mydea force-pushed the fn/better-node-client-init branch from c9cdd5b to 0fa085a Compare May 21, 2025 12:30

mydea mentioned this pull request May 21, 2025

feat(browser): Disable client when browser extension is detected in init() #16354

Merged

mydea force-pushed the fn/better-node-client-init branch from 0fa085a to 8e0b4d5 Compare May 21, 2025 13:46

mydea marked this pull request as ready for review May 21, 2025 13:59

mydea requested review from AbhiPrasad, andreiborza and Lms24 May 21, 2025 13:59

mydea force-pushed the fn/better-node-client-init branch from 8e0b4d5 to 6fc5ba3 Compare May 21, 2025 14:36

andreiborza reviewed May 21, 2025

View reviewed changes

AbhiPrasad reviewed May 21, 2025

View reviewed changes

timfish reviewed May 21, 2025

View reviewed changes

mydea and others added 3 commits May 22, 2025 12:07

Apply suggestions from code review

4dab0aa

Co-authored-by: Andrei <[email protected]>

PR feedback

b7abc65

mydea force-pushed the fn/better-node-client-init branch from dea8972 to b7abc65 Compare May 22, 2025 12:58

move a bit more to client

1779f66

mydea marked this pull request as draft May 23, 2025 07:27

Lms24 reviewed May 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

mydea commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025 •

edited

Loading

Uh oh!

codecov bot commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreiborza May 21, 2025

Uh oh!

mydea May 22, 2025

Uh oh!

AbhiPrasad left a comment

Uh oh!

timfish left a comment

Uh oh!

mydea commented May 23, 2025

Uh oh!

Lms24 left a comment •

edited

Loading

Uh oh!

timfish commented May 23, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

mydea commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

codecov bot commented May 21, 2025

❌ Unsupported file format

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreiborza May 21, 2025

Choose a reason for hiding this comment

Uh oh!

mydea May 22, 2025

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad left a comment

Choose a reason for hiding this comment

Uh oh!

timfish left a comment

Choose a reason for hiding this comment

Uh oh!

mydea commented May 23, 2025

Uh oh!

Lms24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timfish commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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

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

github-actions bot commented May 21, 2025 •

edited

Loading

Lms24 left a comment •

edited

Loading

timfish commented May 23, 2025 •

edited

Loading