feat: Autoload Database Integrations in Node environment

[kamilogorek]

This feature will auto-load DB tracing integrations whenever a related package is available in the environment.

Postgres testing snippets: #3064
Mongo testing snippets: #3072
Mysql testing snippets: #3088

Docs: getsentry/sentry-docs#3563

[github-actions]

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	20.65 KB (+0.09% 🔺)
@sentry/browser - Webpack	21.52 KB (+0.1% 🔺)
@sentry/react - Webpack	21.55 KB (+0.09% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	28.14 KB (+0.8% 🔺)

[HazAT]

I am going to pick up the PR and carry it to the 🏁

[HazAT]

👍

[rhcarvalho]

Is this going to work for all clients (the default one from init and custom clients)?
Can users disable those DB integrations the same way they can disable other integrations?

[rhcarvalho]

Why does this try to assign mod twice? If the first dynamicRequire threw no exception, why don't we return mod there?

[kamilogorek]

Because T | undefined return type requires us to return on every branch anyway, so the best we could do is

  try {
    mod = dynamicRequire(module, moduleName);
  } catch (e) {
    // no-empty
  }

  try {
    const { cwd } = dynamicRequire(module, 'process');
    mod = dynamicRequire(module, `${cwd()}/node_modules/${moduleName}`) as T;
  } catch (e) {
    // no-empty
  }

  return undefined;

In which case the current solution is imo more readable.

[rhcarvalho]

Maybe I'm reading it wrong, but if there are no exceptions, this will execute linearly like this:

mod = dynamicRequire(module, moduleName);
const { cwd } = dynamicRequire(module, 'process');
mod = dynamicRequire(module, `${cwd()}/node_modules/${moduleName}`) as T;

In which case mod = dynamicRequire(module, moduleName); is overwritten by mod = dynamicRequire(module, ${cwd()}/node_modules/${moduleName}) as T;.

I'd understand it better if it was

  try {
    mod = dynamicRequire(module, moduleName);
    return mod; // NEW CODE
  } catch (e) {
    // no-empty
  }

  try {
    const { cwd } = dynamicRequire(module, 'process');
    mod = dynamicRequire(module, `${cwd()}/node_modules/${moduleName}`) as T;
    return mod; // NEW CODE
  } catch (e) {
    // no-empty
  }

  return undefined;

[kamilogorek]

We do want to override it, as if possible, we want the second value. This is to mimick 1:1 require.resolve behavior.

[rhcarvalho]

Ah, I see. So why don't we start with what we want first, and fallback later?

The code as is is non-obvious.

Suggested change

	try {
	mod = dynamicRequire(module, moduleName);
	} catch (e) {
	// no-empty
	}
	try {
	const { cwd } = dynamicRequire(module, 'process');
	mod = dynamicRequire(module, `${cwd()}/node_modules/${moduleName}`) as T;
	} catch (e) {
	// no-empty
	}
	try {
	const { cwd } = dynamicRequire(module, 'process');
	mod = dynamicRequire(module, `${cwd()}/node_modules/${moduleName}`) as T;
	return mod
	} catch (e) {
	// ignore
	}
	try {
	mod = dynamicRequire(module, moduleName);
	return mod
	} catch (e) {
	// ignore
	}

[rhcarvalho]

This is to mimick 1:1 require.resolve behavior.

Just saw this in the JSdoc; if we are trying to mimic also the side-effects of importing, then ignore my suggestion.

[kamilogorek]

It's stylistic thing already, and I don't want to run CI over and over again :P

[rhcarvalho]

We’re changing in this same PR some unrelated code to return early, and here choose to do an import potentially twice :-)

The import is not only a style preference, it also contains potential side-effects. We are not profiling the code nor explicitly trying to make SDK startup as fast as possible, but we can also say that the double import also makes it overall slower.

No need to block on this, we've got 2 review approvals mine included!

[kamilogorek]

Is this going to work for all clients (the default one from init and custom clients)?

No, it won't work for custom clients, the same way as default integrations doesn't work for them.

Can users disable those DB integrations the same way they can disable other integrations?

Yes, with the same defaultIntegrations: false setting. Or filter them out as with regular integrations.

[x5a]

we're still investigating, but this change unexpectedly broke our apps in a very insidious and subtle way that caused a production outage - I'd encourage you to release this sort of monkey-patching-database-connectors change in a major version rather than a minor version.

[HazAT]

@x5a Can you provide a bit more info; How does your stack look like / how can we repro this?

[x5a]

@HazAT we're using postgres ([email protected]) specifically in concert with [email protected]. Because you monkeypatch Client.prototype.query and assume that the return value is thenable (it's not when working with https://node-postgres.com/api/cursor), we started getting an exception:

TypeError: orig.call(...).then is not a function\n' + '    at Client.query (/opt/app/node_modules/@sentry/tracing/dist/integrations/postgres.js:49:56)

which maps to: https://github.com/getsentry/sentry-javascript/blob/master/packages/tracing/src/integrations/postgres.ts#L63

[Kikobeats]

I opened a new issue to warn users the latest version is broken:

#3566

[elliotttf]

I can also confirm that this change has broken my application's postgres integration. In my case, the failure occurs when using a QueryStream via knex and pg-query-stream. The failure is due to the exact same reason that @x5a has pointed out. For example:

const stream = knex('posts').select('*').stream();
stream.pipe(someWritableStream);

The resulting query object is a QueryStream and just like the cursor is not thenable. The assumption in the sentry code about the original query method being thenable fails but the error is swallowed in a not-obvious way.