-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Move default option handling from init to NodeClient
#16353
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
size-limit report 📦
|
❌ 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.
c9cdd5b to
0fa085a
Compare
0fa085a to
8e0b4d5
Compare
8e0b4d5 to
6fc5ba3
Compare
packages/node/src/sdk/index.ts
Outdated
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.
This function isn't exported, do we need to mention it shouldn't be used?
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.
ah yeah, I moved stuff around a bit here, you are right :)
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.
@timfish does this impact electron?
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 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 <168741329+andreiborza@users.noreply.github.com>
dea8972 to
b7abc65
Compare
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!
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 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)
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...
...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.
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
initfunction itself, instead of handling stuff in theNodeClientitself. 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:
init(), as there is a lot going on.initAndBindwhich we have in other places.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.Extracted this out of #15307