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.

[nwalters512]

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

Thanks, this reference was really helpful! I was able to trace through and prove to myself that headers, etc. will still work as intended even without Express instrumentation. I do indeed see that parameterized transaction names are missing without the Express instrumentation, as you pointed out. It would be very nice if that could work the same as before out of the box (though it's nice that I can just enable the Express instrumentation on its own - thanks for pointing that out).

[MattIPv4]

When we encounter an error in our stack, we manually call Sentry.Handlers.errorHandler()(err, req, res, () => {}); to track the error in Sentry with all the request metadata associated with it. To migrate to v8, I replaced this with a regular Sentry.captureException(err); call.

We also call server.use(Sentry.Handlers.requestHandler({ user: [ 'id', 'permissions' ] })); when creating the server. I removed this, and ensured we passed integrations: [ Sentry.requestDataIntegration({ include: { user: { id: true, permissions: true } } }) ] in our Sentry.init call.

With both of these changes in place, the two immediate differences I noticed in behaviour are:

• res.sentry is no longer populated with the tracked error Id. This can be fixed easily enough by doing res.sentry = Sentry.captureException(err); at the point we're capturing the error.

• The error culprit is now the method signature, not the request route. There is also no longer a transaction value set with the request route.

---

I appreciate you aren't going to add these back, and I agree that the http monkey-patch seems to have most of the functionality, but it would be nice to have 1:1 behaviour with what happened before.

It'd also be nice to see these be documented as being removed, with recommendations for what to do instead, rather than just silently removed and leaving users to figure it out, getting angry at Sentry in the process.

[nwalters512]

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.

This is actually not quite correct; with tracesSampleRate: undefined in my Sentry.init options, the instrumentation will be enabled. In v7, the same config option did not result in any additional instrumentation. I opened an issue here: #12034

[mydea]

We also call server.use(Sentry.Handlers.requestHandler({ user: [ 'id', 'permissions' ] })); when creating the server. I removed this, and ensured we passed integrations: [ Sentry.requestDataIntegration({ include: { user: { id: true, permissions: true } } }) ] in our Sentry.init call.

This seems correct to me 👍

The error culprit is now the method signature, not the request route.

With culprit, you mean the top of the stack frame? I guess this makes sense - is it, from your POV, not correct the way it is shown?

There is also no longer a transaction value set with the request route.

This is true - it is a limitation of not having the expressIntegration(). If you want proper transaction names on errors, you'll need this! We may look into adding this by default even if performance is disabled, it's a trade off between how much stuff we want to enable by default vs. what you get out of the box 🤔

It'd also be nice to see these be documented as being removed, with recommendations for what to do instead, rather than just silently removed and leaving users to figure it out, getting angry at Sentry in the process.

I totally get the feeling! FWIW, it is documented here: https://docs.sentry.io/platforms/javascript/guides/express/migration/v7-to-v8/#updated-sdk-initialization but I understand that not everybody will see this properly.

To give some background, the reason why we didn't deprecate this in v7, is that there simply was no direct replacement - you could not get rid of this before updating to v8, so we couldn't show a deprecation warning (because they should always be actionable). I understand that this could lead to annoyance after updating, though.

We are currently working on updating our codemod migr8 to also update these for you, but this is still WIP - but very soon, it should update the handlers for you too!

This is actually not quite correct; with tracesSampleRate: undefined in my Sentry.init options, the instrumentation will be enabled. In v7, the same config option did not result in any additional instrumentation. I opened an issue here: #12034

Thanks for raising this, this is not ideal and we'll adjust this, should not be this way!

[Michsior14]

I don't want to hijack the conversation, but is there any solution for tracing with node service bundled using webpack and without any external dependencies setup (so virtually express is part of the bundle)? This previously worked fine with Handlers.tracingHandler(), but now I can't think of any solution for such setup.

[MattIPv4]

The error culprit is now the method signature, not the request route.

With culprit, you mean the top of the stack frame? I guess this makes sense - is it, from your POV, not correct the way it is shown?

I mean the hint that shows up after the error name in Sentry:

and what gets sent in Slack too:

Compared to before where it showed the route:

and in Slack:

There is also no longer a transaction value set with the request route.

This is true - it is a limitation of not having the expressIntegration(). If you want proper transaction names on errors, you'll need this! We may look into adding this by default even if performance is disabled, it's a trade off between how much stuff we want to enable by default vs. what you get out of the box 🤔

We're not using Express, so I'm not sure we want to use the Express integration (we also didn't use the full integration before -- specifically we used the request handler, but not the error handler)?

[Lms24]

@Michsior14 I believe your concern is tracked in #10940 - feel free to add your thoughts there.

Generally, you should get basic http instrumentation, meaning a transaction, as well as tracing. Express route handler and middleware instrumentation won't work in this case (yet) but you should get something

[Lms24]

UPDATE: There was a bug in the http instrumentation that caused the transactionName update not to be applied correctly. See @mydea's response below (rest of my response still stays valid)

@MattIPv4 the event.transaction name or culprit assignment has received some changes in v8 (tracked in #10846) If our http instrumentation is initialized correctly, you should get a culprit transactionName in the form of <Method> <Route> (see code here).

The change here primarily is that you need the http or a framework (e.g. express) instrumentation to be used; what we had before didn't require this strictly but the names could still be off significantly depending on the framework. Sadly, this is one of the consequences of moving to OpenTelemetry.

[mydea]

Actually, we just figured out that we are not setting the transaction name correctly in http-only mode without performance instrumentation. See #12071, this should be fixed in the next version!

[mydea]

Hey folks,

We have since shipped some updates to this:

1. Transaction name (culprit) should be set correctly with just http integration now.
2. Performance integrations are correctly skipped if tracesSampleRate: undefined is set.

You should now be able to have a good experience with just the http integration, when only using Sentry for errors!

[MattIPv4]

Transaction name seems to be set correctly now!

However, it looks like requestDataIntegration won't let me track custom properties on the user, like I could do with requestHandler? I have { include: { user: { id: true, permissions: true } } } set as I mentioned before, but it seems to only be tracking the id?

Looking at the typings, it seems only a predefined set of properties on a user can be selected, not anything custom like before:

sentry-javascript/packages/core/src/integrations/requestdata.ts

Lines 10 to 24 in f3c8a86

	include?: {
	cookies?: boolean;
	data?: boolean;
	headers?: boolean;
	ip?: boolean;
	query_string?: boolean;
	url?: boolean;
	user?:
	| boolean
	| {
	id?: boolean;
	username?: boolean;
	email?: boolean;
	};
	};

That being said, looking at the actual code, it seems like it should just accept any arbitrary keys, so I'm very confused as to why its not making it through:

sentry-javascript/packages/core/src/integrations/requestdata.ts

Lines 118 to 124 in f3c8a86

	const userIncludeKeys: string[] = [];
	for (const [key, value] of Object.entries(user)) {
	if (value) {
	userIncludeKeys.push(key);
	}
	}
	addReqDataUserOpt = userIncludeKeys;

sentry-javascript/packages/utils/src/requestdata.ts

Lines 277 to 286 in f3c8a86

	if (include.user) {
	const extractedUser = req.user && isPlainObject(req.user) ? extractUserData(req.user, include.user) : {};
	if (Object.keys(extractedUser).length) {
	event.user = {
	...event.user,
	...extractedUser,
	};
	}
	}

sentry-javascript/packages/utils/src/requestdata.ts

Lines 120 to 136 in f3c8a86

	function extractUserData(
	user: {
	[key: string]: unknown;
	},
	keys: boolean | string[],
	): { [key: string]: unknown } {
	const extractedUser: { [key: string]: unknown } = {};
	const attributes = Array.isArray(keys) ? keys : DEFAULT_USER_INCLUDES;
	attributes.forEach(key => {
	if (user && key in user) {
	extractedUser[key] = user[key];
	}
	});
	return extractedUser;
	}

What is the replacement for the ability to track custom user properties (in this case an object) via requestHandler?

Scratch that, looks like this is no longer showing up in production either, filed a general issue for the Sentry UI: getsentry/sentry#71305

[getsantry]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀