Skip to content

Move Node specific code to @sentry/node #7346

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

Closed
AbhiPrasad opened this issue Mar 6, 2023 · 4 comments
Closed

Move Node specific code to @sentry/node #7346

AbhiPrasad opened this issue Mar 6, 2023 · 4 comments
Assignees
Labels
Package: node Issues related to the Sentry Node SDK
Milestone

Comments

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Mar 6, 2023

### Node
- [ ] https://github.com/getsentry/sentry-javascript/pull/7503
- [ ] https://github.com/getsentry/sentry-javascript/pull/7503

For Node we probably just want to update the docs to match OpenTelemetry:

const Sentry = require("@sentry/node");

Sentry.init({
  integrations: [...Sentry.autoDiscoverNodePerformanceMonitoringIntegrations()]
});
@timfish
Copy link
Collaborator

timfish commented Mar 16, 2023

@AbhiPrasad how were you thinking autoDiscoverNodePerformanceMonitoringIntegrations would work?

autoDiscoverNodePerformanceMonitoringIntegrations could simply return an array of integrations as they are but many of them will log warnings when loadModule fails in setupOnce. This is also in no way "auto discovery"

I can see that some integrations won't work without constructor options so they should not be considered.

It looks like the Apollo and GraphQL integrations are not currently loaded automatically via _autoloadDatabaseIntegrations but it looks like they can support auto initialisation.

What should be prioritised in the design? Do we need to keep the usage of dynamicRequire so that the integrations are never picked up by a bundler? if autoDiscoverNodePerformanceMonitoringIntegrations doesn't find any integrations we can assume a bundler is in use and warn that the integrations will need selecting manually.

Current plan is:

  • Attempt to load the integrations like in _autoloadDatabaseIntegrations to keep them out of bundles
  • If none are found, log a warning about how autoDiscoverNodePerformanceMonitoringIntegrations doesn't work with bundlers and integrations will need to be imported individually
  • Modify all integrations so loadModule code is in another method we can call separately (like loadDependencies())
  • Cycle through integrations and only return those where loadDependencies returns true

@AbhiPrasad
Copy link
Member Author

how were you thinking autoDiscoverNodePerformanceMonitoringIntegrations would work

Yeah this is tricky. I think your plan should suffice for now, but let's see if that changes.

It looks like the Apollo and GraphQL integrations are not currently loaded automatically via _autoloadDatabaseIntegrations but it looks like they can support auto initialization.

We did this intentionally because we had plans for v8 to remove the auto initialization all together, but maybe this changes.

@timfish
Copy link
Collaborator

timfish commented Mar 17, 2023

  • Modify all integrations so loadModule code is in another method we can call separately (like loadDependencies())
  • Cycle through integrations and only return those where loadDependencies returns true

I'm going to leave these out of the initial PR to keep it smaller and this adds additional behaviour over the existing _autoloadDatabaseIntegrations

@AbhiPrasad
Copy link
Member Author

Closing, let's add the docs task and any other clean up to #5815

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK
Projects
None yet
Development

No branches or pull requests

2 participants