Express middleware state leaking across requests

[tpbowden]

Package + Version

• @sentry/browser
• @sentry/node
• raven-js
• raven-node (raven for node)
• other:

Version:

4.4.0

Description

When calling Sentry.captureException from an express middleware, the url field reported to Sentry is incorrect. It is almost always being reported as /health or /metrics (these are hit very frequently), even though the error happened on a totally different URL. I have Sentry.Handlers.requestHandler() as the first middleware and Sentry.Handlers.errorHandler() as the error middleware

Is there a way to isolate the Sentry client for each request? Please let me know if you need more information

[kamilogorek]

Node domains which we use should do that on their own. Is there a chance you could provide a repro-case and node version details I could use to debug this?

[CharlieEarnup]

  Sentry.init({
    dsn: 'https://[email protected]/1353261',
    defaultIntegrations: false,
  });

  app.get('/testme', (req, res, next) => {
    try {
      setTimeout(() => {
        Sentry.configureScope((scope) => {
          scope.setUser({"email": "[email protected]"});
        });
      }, 2000);

      const ham = {};
      ham.bone.t = 'bacon';
    } catch (err) {
      next(err);
    }
  });

  app.get('/testme2', (req, res, next) => {
    try {
      setTimeout(() => {
        Sentry.configureScope((scope) => {
          scope.setUser({"email": "[email protected]"});
        });
      }, 2000);

      const ham = {};
      ham.bone.home = 'bacon';
    } catch (err) {
      next(err);
    }
  });

  app.use(Sentry.Handlers.errorHandler());

I was able to reproduce this error using this code. I may be missing something here, but I'm not sure how initializing a single global sentry hub (like the documentation does) would keep information separate when requests are being handled concurrently. 🤔

Is intended that the user initializes a hub on a per request basis?

[asafigan]

This is a huge problem with breadcrumbs. If you capture an event it will have all of the breadcrumbs from all of the requests that the machine has received.

[adrienboulle]

@asafigan @kamilogorek not only for breadcrumbs.
As @CharlieEarnup maybe I missed something but as I understood the source code, the node integration of sentry is just not useable at all with the current implementation, there is only one hub created, shared via the global variable.
Scope seems to be shared everywhere, its written is if we are in the browser with only one context or as if javascript request where synchronous

plus, captureMessage and captureException signatures shows that it assumes that there is one context, this is a major issue, and Node domains is not the answer, we should be able to use Sentry sdk without domains, in my app i have a custom errorHandler cause I want to be able to keep trace of what happens (error or not) from req start, during, and sometimes after end of request, all of this being obviously asynchronous, and many requests are handled simultaneously, so i just want to send whatever i want to sentry, whenever i want.

only solution I can think of is using captureEvent by itself

[asafigan]

That is what I feared. I think it's a problem because it looks like all the sentry libraries have the same interface but JavaScript is not like most languages. I don't think a scope system can really works in a concurrent environment like JavaScript.

[adrienboulle]

yes exactly, and when you read the doc it become even clearer, its write as if javascript was a thread by request language :
https://docs.sentry.io/enriching-error-data/scopes

[kamilogorek]

@CharlieEarnup you have to add app.use(Sentry.Handlers.requestHandler()); as the first handler of your Express app. We mention that in our docs: https://docs.sentry.io/platforms/node/express/

Node domains is not the answer

There is no way to separate context between requests without the domains.

we should be able to use Sentry sdk without domains

You can, you just won't have a separation of context data.

so i just want to send whatever i want to sentry, whenever i want.

You can do that. Track your own scopes and pass them to captureException handler through the client directly.

https://docs.sentry.io/platforms/javascript/advance-settings/

import { BrowserClient } from "@sentry/browser";
const client = new BrowserClient({ dsn });
client.captureException(exception, {}, yourOwnScope);

I don't think a scope system can really works in a concurrent environment like JavaScript.

It can with some restrictions and pre-requisites.

[CharlieEarnup]

@kamilogorek It is the first error middleware in my example code. It is the only error middleware in my example code.

[kamilogorek]

@CharlieEarnup it's definitely not here – #1773 (comment)

[CharlieEarnup]

@kamilogorek there must be some miscommunication here, do you mean it needs to go above the routes as well?

[adrienboulle]

There is no way to separate context between requests without the domains.

juste declare a scope variable in the request object

You can, you just won't have a separation of context data.

idem

so i just want to send whatever i want to sentry, whenever i want.

we have different views of what an advanced usage is but ok :)

thx for the hints, I'll use BaseClient and not BrowserClient I guess

Still, I think that the usage of domains is way overkill

[CharlieEarnup]

@kamilogorek Oh I see the requestHandler middleware is required in order to create a domain. Well it's not very nodey, but I guess it works.

[CharlieEarnup]

@adrienboulle you don't know the half of it that Api is getting deprecated for a reason.

https://nodejs.org/en/docs/guides/domain-postmortem/

[kamilogorek]

do you mean it needs to go above the routes as well?

@CharlieEarnup it states very clearly in the docs. requestHandler => your handlers => errorHandler => your error handlers

// The request handler must be the first middleware on the app
app.use(Sentry.Handlers.requestHandler());

...

// The error handler must be before any other error middleware
app.use(Sentry.Handlers.errorHandler());

@adrienboulle you don't know the half of it that Api is getting deprecated for a reason.

It's soft deprecated for 5 years now with no real alternative (other than async hooks which are not there yet).

juste declare a scope variable in the request object

It won't catch async errors without domains. Thats the reason why we use implicit binding and call everything in run context (see domain docs linked above).

[CharlieEarnup]

@kamilogorek it's actually very unclear because nowhere does it communicate that you need requestHandler to support errorHandler. I personally know that I saw domains in the requestHandler code and omitted it on purpose.

it won't catch async errors without domains.

It won't catch unhandled async errors without domains. Typically speaking though most people tend to implement their controllers with a wrapper that handles their errors. If you absolutely cannot take any chances with uncaught errors, sure domains work, but for most they are way overkill.

[adrienboulle]

@CharlieEarnup exactly, and Sentry is supposed to be a tool, not a framework, i handle my code errors by myself I wont rely on a third party for that

[kamilogorek]

@CharlieEarnup exactly, and Sentry is supposed to be a tool, not a framework, i handle my code errors by myself I wont rely on a third party for that

I don't get what's the issue here tbh. If you don't want to use requestHandler, then you can skip it and use the client directly, without any context, scope etc., just Sentry.captureException itself.

@CharlieEarnup

it's actually very unclear because nowhere does it communicate that you need requestHandler to support errorHandler.

https://docs.sentry.io/platforms/node/express/

I'm pretty sure "must" means "you need to use" :)

If you absolutely cannot take any chances with uncaught errors, sure domains work, but for most they are way overkill.

In our experience, most people want a "plug and play" solution. Using requestHandler + errorHandler gives you async error handling, scope separation and additional request context for every error. We provide a manual Sentry.NodeClient instance for those that want to use our SDK just as a simple error reporter.

[adrienboulle]

I get that most people want a plug and play solution, the "issue" is that this client is black or white, either plug and play or git clone the project and try to find how to do

[HazAT]

@adrienboulle So your point basically is we are lacking documentation on how to use the internals of the SDK if you want to manage everything yourself?

[rhyek]

I have a situation where I need to capture an exception with sentry that I do not need express to handle after, so I am using captureException explicitly. However, I want all my request context (url, payload, etc) to be included in the report just like when I let sentry's Sentry.Handlers.errorHandler() do its work.

An example of what I need to do:

app.use(Sentry.Handlers.requestHandler());
app.use('/some-work', async (req, res, next) => {
  try {
    await someAsyncWork.catch(error => {
      Sentry.captureException(error); // capture, but ignore error
    });
    res.send('All good.');
  } catch (error) {
    next(error);
  }
});
app.use(Sentry.Handlers.errorHandler());

Is this intended to work as I except it to?

Edit:
Perhaps this necessary?

  await someAsyncWork.catch(error => {
    Sentry.withScope(scope => {
      scope.addEventProcessor(async event => Sentry.Handlers.parseRequest(event, req));
      Sentry.captureException(error);
    });
  });

[adrienboulle]

@HazAT now that things has been explained and that I have explored the code by myself yes basically, even so I think the use of domain is not a very good idea but for my use case I do not really care. I think it would be very very helpful to explain in the doc how to use the low level API (how to send sentry events with request, user, exception/message and tags

[Grmiade]

Is this issue still relevant?
Because I use the Sentry.Handlers.requestHandler() as the first handler of my Express app. But I have a lot of mismatch between the tags/headers of my requests in Sentry (url, browser, ...).

[adrienboulle]

@Grmiade i'd recommend to use Sentry.captureException and not the requestHandler

[kamilogorek]

Closing due to the issue getting out-dated. Feel free to ping me to reopen it if it's still relevant and/or create a more concrete ticket for a specific issue.

[rhyek]

@kamilogorek no one ever answered my question.

[kamilogorek]

@rhyek sorry I missed that. You're totally correct. Your second example is 100% accurate.