Sentry.setUser concurrency issues #4372
Comments
|
We recommend isolating async resources using domains or async hooks. As an example, the Sentry express integration uses domains under the hood to accomplish this. If you don't want to use domains or async hooks, we recommend you manually pass around an explicit scope object so you control exactly what is being set and sent. If you can provide a small demo or reproduction of what you're trying to accomplish we can help debug further. |
|
At the moment my code is something like this: Hook1 fastify.addHook("onRequest", (request: FastifyRequest, _reply: FastifyReply, done) => {
const currentHub = Sentry.getCurrentHub();
currentHub.configureScope((scope) => {
scope.addEventProcessor((event) => {
event.request = {
method: request.method,
...event.request,
};
return event;
});
});
const tx = Sentry.startTransaction({
name: `${request.method} ${request.url}`,
op: "http.server",
description: "HTTP request",
});
currentHub.configureScope((scope) => {
scope.setSpan(tx);
});
request.sentryTx = tx;
done();
});Hook2 fastify.addHook("onRequest", async (request: FastifyRequest, reply: FastifyReply) => {
Sentry.getCurrentHub().configureScope((scope) => {
// eslint-disable-next-line @typescript-eslint/naming-convention
scope.setUser({ id: oid, username: upn, email: upn, ip_address: ipaddr });
});
});Handler fastify.get("/", async (_request: FastifyRequest, _reply: FastifyReply) => {
const sentryTxSpan = request.sentryTx.startChild({
op: "db.do_stuff",
description: "Do stuff",
});
// stuff
sentryTxSpan.finish();
return "Test";
});Hook3 fastify.addHook("onResponse", (request: FastifyRequest, reply: FastifyReply, done) => {
request.sentryTx.setHttpStatus(reply.statusCode);
request.sentryTx.finish();
done();
});The way each request is executed is: Hook1 -> Hook2 -> Handler -> Hook3 Is this the right way to track/associate the current user with its own request? |
|
I see the express.js handler is using domains: https://github.com/getsentry/sentry-javascript/blob/master/packages/node/src/handlers.ts#L420-L421 But they are deprecated: https://nodejs.org/dist/latest-v16.x/docs/api/domain.html#domain Not sure why sentry went with this architecture of making everything global, data should've been instance scoped, not global. Or at last allow to set some data on the instance level as well. For browser it made sense maybe to have everything global, but for server side, for nodejs everything should be request scoped as each request handles data related to different user. We need to be able to associate a transaction with a specific user. |
|
Regarding "passing around an explicit scope object" I would do that as well if it would be possible. But it seems you can only set an explicit scope to Sentry.captureException/captureMessage/etc. You can't set an scope object to a transaction. At least I was not able to find out how this can be done. |
|
I'm having the same issue when trying to capture transactions during server-side rendering of a Next.js app. Not being able to configure the scope per-transaction is… unfortunate. In Node.js there are many situations where multiple transactions are running concurrently. |
|
Your problem isn't so much passing around an explicit scope; it's handling things such that Sentry.getCurrentHub() returns the right hub for the current request. The reason this is needed is because you're trying to capture errors from all over the app, and you simply can't easily pass a local around on the stack for this without potentially altering every function, which is painful. Sentry's own integration for Express uses a Node.js Domain - these are indeed deprecated, but they're in limbo as far as I can tell, awaiting a replacement architecture for the same thing. So I reckon use them for now. Domain usage is encapsulated almost completely within Sentry.getCurrentHub(), so you needn't actually do much more than create and enter them. In principle, you could hook into the async scheduler, which is what Domain does under the hood, and I believe the lower-level replacement APIs are around now, but I think you'd then need to handle Sentry.getCurrentHub() in a different way which sounds like a lot of hard work to me. I'm not all that familiar with Fastify, but my gut feeling is that you could hook into the request early, and create a domain there, and enter it immediately: fastify.addHook("onRequest", (request: FastifyRequest, _reply: FastifyReply, done) => {
request.__sentry_domain = domain.create()
request.__sentry_domain.enter()
Sentry.getCurrentHub() // Actually creates the current hub on demand, nice to have to debug
}
fastify.addHook("onResponseOrSomethingRightAtTheEnd", ... => {
// Do normal Sentry cleanup work like ending transaction.
if (request.__sentry_domain) {
request.__sentry_domain.exit()
request.__sentry_domain = null
}
}Then everything else in the API should mostly work. You'll need to capture exceptions at sensible places too (and finish transactions, and so on), but it looks like you have that covered. |
|
Hey @danechitoaie, it seems like @dwd provided a good answer here (Thanks @dwd !). Did you give it a try? Is this resolving the error for you? |
|
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 label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
I'm having the same issue today with my Apollo graphql server. Why can't we set a user directly on a transaction ? Wouldn't that solve the problem ? |
Package + Version
@sentry/browser@sentry/nodeDescription
I'm trying to integrate Sentry with Node.js serverside application and the problem is that
Sentry.setUser();is global. And this causes concurrency issues. The Node.js application handles multiple requests, each associated with different user.Is there a way to set the user per request, scoped to current transaction and not globally, so that we don't have concurrency issues?
The text was updated successfully, but these errors were encountered: