sentry-javascript: Express middleware state leaking across requests

@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

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);
    });
  });

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.

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

@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/

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).

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 it’s definitely not here – https://github.com/getsentry/sentry-javascript/issues/1773#issuecomment-449094541

  Sentry.init({
    dsn: 'https://753235da75a243988b77c7e850280d1d@sentry.io/1353261',
    defaultIntegrations: false,
  });

  app.get('/testme', (req, res, next) => {
    try {
      setTimeout(() => {
        Sentry.configureScope((scope) => {
          scope.setUser({"email": "big.frank@example.com"});
        });
      }, 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": "angst.joe@example.com"});
        });
      }, 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?