create domain per request, so hub per request is created

[jardakotesovec]

I suspect that creating new Scope() does not do anything.. its just empty object and it will stay empty. You can try - no breadcrumbs get recorded there.

After bit digging, I think only way to isolate requests is to have hub per request, which is saved in domain. This is pretty much what express middleware does.

If you look at the getCurrentHub it creates new hub in domain if there is active domain. Otherwise it uses one global hub.

Using this would also add option to add some context anywhere within request lifecycle with

Sentry.configureScope(scope => {
  scope.setTag('key', 'value');
});

All these global functions like configureScope or withScope they get hub internally via getCurrentHub which gets the current hub from active domain, therefore should be scoped to the request.

I will do some testing sometime soon, but interested in your feedback. This code is mainly just to get point across for now.

Hi @kamilogorek if you would have few minutes just to glance over this PR if it makes sense? It would be super helpful :-) Just trying to make the hapi integration right.

[jardakotesovec]

Ok, rebased. Added some comments and domain clean up. Also removed [WIP] as now I think its legitimate PR :-). Removed test was testing sentryScope which is not used anymore, so removed that.

[jardakotesovec]

In case you decide to go forward with this change, it probably should be major version bump, since that sentryScope is gone, which theoretically someone could use. Also anyone using domains should be aware of that update.. to basically ensure its not interfering with their use case in some bad way. Also unlikely, but possible.

[guischdi]

What about https://github.com/getsentry/sentry-javascript/blob/master/packages/node/src/handlers.ts#L247-L255?
Don't we need something similar here to ensure the domain recognizes errors thrown in a hapi request handler? And another (maybe stupid) question: How is the Hub interconnected to the domain?

[jardakotesovec]

@guischdi Hi, thanks for looking into it.
My understanding why express integration has this approach is that domains are useful also for catching programmer and other errors - simply exceptions that don't end up following callback next(err) and would end up as unhandled exception without any context. We don't care about that in hapi, since version 17 - domains where removed from there because its now relying on catches and promise chains - you can see more details in changelog.

So with this assumption - if you want to catch such unhandled error with domain and still have context which request caused it - thats exactly what they are doing on these lines. With hapi its fine to rely on that request error event and report errors there.

And with regards how the hub is interconnected with domain. Thats not stupid question, it took me sometime to understand that. Best is to look at the getCurrentHub, where you see how its looking for current hub. Point of domain is that it should be around for complete async chain of calls therefore for complete lifecycle of request. Hence for example if console breadcrump integration wants to add breadcrumb - it calls getCurrentHub, which checks if there is currently active domain (each request has its own domain). And either get existing hub from there or creates new one if its not there. So these hubs lives in individual domains.

Hope it helps. And feel free to question it.. I might be also missing something.

[guischdi]

Thanks @jardakotesovec for your extensive explanation. I think I now understand it.

Your approach seems to be working: I only get breadcrumbs of the specific request and global actions like console.logs, even if there are concurrent requests.

The thing about the global actions is, that you sometimes might have other global jobs like health checks running from time to time. This jobs might manipulate the scope, which is then also used to be attached to a sentry event. You can find sample code to reproduce this behavior here:

const hapi = require('hapi');
const plugin = require('./');
const Sentry = require('@sentry/node');

const dsn = '<dsn of a test project>';

const delay = t => new Promise(resolve => setTimeout(resolve, t));

(async () => {
  const server = hapi.server({ port: 8080 });

  server.route({
    method: 'GET',
    path: '/test-sentry',
    async handler() {
      Sentry.configureScope(scope => scope.setTag('foo', 'bar'));
      console.log('Tag set to bar');
      await delay(2000);
      throw new Error('Oh no, bar is now 42!');
    },
  });

  await server.register({
    plugin,
    options: {
      client: { dsn },
    },
  });

  await server.start();
  console.log('Server started');

  server.inject({ method: 'GET', url: '/test-sentry' });

  await delay(1000);
  Sentry.configureScope(scope => scope.setTag('foo', 42));
})();

Sentry gets the tag value 42 which is not set in the request handler but in some globally running job, as can be seen here:

Maybe the deprecation warning (also to be found in the Sentry breadcrumbs of the event which was captured at first request made to the test server) gives you some hints how to fix that issue. Or maybe @kamilogorek has some advice how to limit the access to a Sentry hub (or is it only the scope which is accessible from somewhere else) to a single request.

[jardakotesovec]

Can look at it more in detail on monday. But basically if there is no active domain, which is the case in your ‘global’ example than it talks to global hub and the hubs created in domain (within request) are somewhat inherited from the global hub, so you can see it there. If you want to report something to sentry which is specific for one event you can always use that ‘withScope’ function and scope changes inside that won’t leak to any other scope. Hope it somewhat answers your question.

[jardakotesovec]

And I think any global actions are better of separated in own process run by cron or something. And if you want them run in same process you can just create and enter the domain at the beggining (or run it inside domain.run) to get same separation as requests have.

[guischdi]

I didn't say, I WANT to access a scope used (in a request) from somewhere else. I meant, one COULD do so. So my concern was not about whether changes on a scope in a request handler could leak to some other part of the application. It's about the other direction: No matter if I use withScope or configureScope the global operation (like a database health check or something) could accidentally overwrite some scope properties, which will be passed to sentry by some completely other part of the application (the request handler).

[jardakotesovec]

@guischdi Yes, I understand that. My thoughts on this particular scenario:

• it should not happen with withScope, only with configureScope
• it should not overwrite things in hapi requests, "only" leak/extend. If you are setting same field globally and in request, request always win. Did not test that, but after hub is inherited for hapi request, than you can't touch it from the global hub.
• you can isolate your global health checks with domain, which would prevent that, but you have to be aware how the sentry and domains works
• you could trigger your async tasks with server.inject and do them inside hapi handlers, so you get all the hapi error handling and domains without much effort

But you are right, something from global hub still can get inside requests, which in some cases might be intentional (like some global information about that node instance), but also can be unexpected in other use cases.

On the other hand before this change it was significantly more mixed up and noone really complaint, so its not probably that big deal :-). And I think its also good to consider that domains are already pretty magic (one day will be replaced with async hooks in sentry) - when running things in one thread, its not easy to not mix up things without just passing them everywhere as arguments.

[kamilogorek]

Because of the way you are plugging in into the hapi error handlers, you can do it in 2 ways, which doesn't require domains.

a) Hub based: create it and attach it to the request itself:

server.ext({
    type: 'onRequest',
    method(request, h) {
      const currentHub = Sentry.getCurrentHub()
      const requestHub = new Sentry.Hub(
        currentHub.getClient(),
        Sentry.Scope.clone(currentHub.getScope())
      )
      Sentry.setHubOnCarrier(request, requestHub);
      return h.continue;
    },
});

// and then

server.events.on({ name: 'request', channels: opts.channels }, async (request, event) => {
  const hub = Sentry.getHubFromCarrier(request)

  hub.withScope(scope => {
    scope.addEventProcessor(_sentryEvent => {
      // do whatever
    });

    Hoek.applyToDefaults(scope, request.sentryScope);
    hub.captureException(event.error);
  });
});

b) Client/Scope based: use client directly and give it manually handled scope

server.ext({
    type: 'onRequest',
    method(request, h) {
      // Inherit data from the parent
      request.sentryScope = Sentry.Scope.clone(currentHub.getScope());
      return h.continue;
    },
});

// and then

server.events.on({ name: 'request', channels: opts.channels }, async (request, event) => {
  const client = Sentry.getCurrentHub().getClient()
  const scope = request.sentryScope;

  scope.addEventProcessor(_sentryEvent => {
    // do whatever
  });
  // or set directly
  scope.setExtra("what", "ever")

  Hoek.applyToDefaults(scope, request.sentryScope);
  client.captureException(event.error, {
      // this is EventHint, so pass it whatever you want to expose
      request,
      event
  }, scope);
});

[jardakotesovec]

@kamilogorek Thanks a lot for getting back to us. Attaching hub to req was option I considered, but was inclined to use domains because thats only way to also save breadcrumps and other integrations (which are using getCurrentHub) to request hub.

Only reason why not to use domains I would think are potential performance/stability issues, but since its used a lot inside sentry I would think you don't find domains troublesome?

[kamilogorek]

@jardakotesovec you're right, and it should be totally fine to use them.
However, I'm not 100% sure how Hapi works, so I'd write some manual tests around it as well, see express tests for the reference - https://github.com/getsentry/sentry-javascript/blob/master/packages/node/test/manual/express-scope-separation/start.js

[guischdi]

Thanks @kamilogorek for your responses and having a look into the PR implementation!

@jardakotesovec I understand, why using domains seems the right way of doing it.
But maybe you could do two things for getting this PR to our utmost satisfaction:

1. Write some test analogous to the linked express scope separation test.
2. Adjust the "Scope" section in README.md to the new way of handling scopes from route handlers.

P.S: Sorry for the small delay!

[jardakotesovec]

@guischdi Yes, tests are always nice to have. I am on holidays for rest of the week and next week, hopefully I will be able to find some spare time to make progress on that then.

[mkg20001]

@jardakotesovec You might want to rebase this against latest master after writing tests, so you can check if the new deps break anything

[guischdi]

Sorry for the latency, @jardakotesovec !
I rebased your branch, wrote some tests and re-enabled our scope handling (which should have nothing to do with your introduced changes).

You may have a look at this. If everything is fine with you, we would release this changes with a new semver major (due to dependency updates performed in the meantime). I introduced a new option (useDomainPerRequest) with default value false, because domains imply performance issues.

[jardakotesovec]

@guischdi Thanks! Looks good. Sorry for not responding I was on holidays.