Replacement for Handlers.requestHandler, Handlers.errorHandler, etc.

[nwalters512]

Problem Statement

I'm trying to upgrade to v8 of @sentry/node. By and large, the migration instructions at https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.mdand https://docs.sentry.io/platforms/javascript/migration/v7-to-v8 have done a good job explaining how to upgrade. However, there's no mention of the fact that the Handlers export was removed, or what APIs have replaced Handlers.requestHandler(), Handlers.tracingHandler(), or Handlers.errorHandler(). The only relevant mention of handlers in either doc is under "Removal of Sentry.Handlers.trpcMiddleware() in favor of Sentry.trpcMiddleware()".

Solution Brainstorm

I'd expect the v8 migration instructions to contain documentation on replacement APIs for Handlers.requestHandler(), Handlers.tracingHandler(), and Handlers.errorHandler().

[HazAT]

Hey, we are going to highlight this - but there is no need for these handlers anymore.
If you import Sentry first thing of your application in Node, we patch everything automatically with OTEL under the hood.

So it should work, if it doesn't it's a bug.

[HazAT]

For Expess for example: https://docs.sentry.io/platforms/javascript/guides/express/#use

[nwalters512]

I ran headfirst into another issue before I could test if auto-instrumentation works: #12011

It seems like v8 of the SDK is really oriented around tracing. For instance, from looking at the implementation, it seems like Express is only instrumented if we enable tracing:

sentry-javascript/packages/node/src/sdk/init.ts

Line 81 in 1390402

...(hasTracingEnabled(options) ? getAutoPerformanceIntegrations() : []),

If I'm interpreting that correctly, I frankly think this is a step back. We don't use Sentry for tracing (we use Honeycomb), and we don't plan on starting. We use Sentry only for error reporting, and it's really really good at that! The old Handlers.requestHandler() was beautifully simple and worked without y'all moneky-patching third-party modules.

I'll withhold judgement until #12011 is fixed and I can actually try the new SDK. However, on an initial look, it doesn't seem like the Express auto-instrumentation will grab headers, cookies, the body, etc. and attach them to error events, as I don't see a call to extractRequestData anywhere in the Express instrumentation. Is this in fact the case? If so, that'll block us from moving to v8.

Aside, I see that the Remix support actually still operates how the Express integration used to:

sentry-javascript/packages/remix/src/utils/serverAdapters/express.ts

Lines 21 to 37 in 1390402

	function wrapExpressRequestHandler(
	origRequestHandler: ExpressRequestHandler,
	build: ServerBuild | (() => Promise<ServerBuild> | ServerBuild),
	): ExpressRequestHandler {
	let routes: ServerRoute[];
	return async function (
	this: unknown,
	req: ExpressRequest,
	res: ExpressResponse,
	next: ExpressNextFunction,
	): Promise<void> {
	await withIsolationScope(async isolationScope => {
	// eslint-disable-next-line @typescript-eslint/unbound-method
	res.end = wrapEndMethod(res.end);
	const request = extractRequestData(req);

[MattIPv4]

👋 I am also very concerned by this and it will likely block us moving to v8. We cannot initialize Sentry before importing other stuff due to the nature of how our application passes around the Sentry DSN etc. We're also using a custom-wrapped version of Restify, so I am very doubtful that Sentry is going to auto-inject itself as middleware correctly. I feel this is quite the regression in the usability of Sentry for folks that aren't following a standard path... and I would ask that these handlers are readded, or at least have documented self-serve replacements, not just documented as being removed with the recommendation being to rely on import magic.

[mydea]

Thank you for your feedback!

I'll try to address the points:

1. The express instrumentation is not strictly needed for errors-only mode, request isolation should also work without it (this is handled in the base httpIntegration). That being said, there is still value in having the express integration, to get parametrized route names etc, so we'll look into adding this even if tracing is disabled. Note that even today you are free to manually add expressIntegration() to your integrations in your Sentry.init() to have the same outcome. See: test(node): Add test for express without tracing #12014
2. With the current state of things, if you have tracing disabled, only http will be patched - and we did patch this before too! So out of the box, without performance, there should not be any more monkey patching than before. We simply now leverage the monkey-patched http module more than before, and thus can get rid of the need to add a requestHandler. For reference, this is where we now store the request so we can add headers etc. to the events: https://github.com/getsentry/sentry-javascript/blob/develop/packages/node/src/integrations/http.ts#L112
3. We are aware that it can be inconvenient to ensure Sentry is executed first, in some code stacks. This is, sadly, a restriction that OpenTelemetry (which is used under the hood now) brings with it. If execution order is hard to reason about in your application, you can also use --require when you run your node app to ensure Sentry is called first:

a. Create a file instrument.js where you put your Sentry code & initialize it.
b. When you run your node app, run it as node --require ./instrument.js my-app.js - this will execute instrument.js before your app, ensuring wrapping everything etc. works as expected.

If it is really impossible for you to have a DSN to be set early in your application, I recommend to do something like this:

// instrument.js - Call this right at the top of your application!
import * as Sentry from '@sentry/node';

// No idea what the DSN is yet
// We still call Sentry.init(), which will setup a client but not do anything with it because we have no DSN
Sentry.init({
  dsn: undefined
}); 

const client = Sentry.getClient();
// Now we add the integrations we def. need
// If we do not care about performance, http is enough
// This will setup handlers but do nothing until later when we're ready
client.addIntegration(Sentry.httpIntegration());

// sentry-is-loaded.js - called when we know the DSN and want to finish the setup
import * as Sentry from '@sentry/node';

// Calling init again will setup a new client that is now enabled
Sentry.init({
  dsn: 'MY_DSN'
});

You can look at this PR: #12016 to see a test that implements something like this.

We will look into ways to possibly make this easier!

4. In regards to Restify, I don't know this in detail, but really any framework that uses http under the hood - which, to my knowledege, is really any framework - should have working request isolation out of the box.