Domain Module Deprecated

[DerekSeverson]

From my understanding the node.js core domain module has been deprecated since v4.

Would you mind fielding the following questions...

1. Is there a plan/roadmap to move raven-node away from using the "domain" module?
2. If a developer wants to stay clear of domains when using raven-node, is it a safe bet that he/she just has to avoid the "context" (context, wrap, setContext, mergeContext, getContext, captureMessage) methods in raven-node?

Apologies if this is not the right place to ask about this topic or if its been covered somewhere else.

Thank You!

[LewisJEllis]

Hey @DerekSeverson - thanks for bringing this up. A couple others have asked similar questions in non-public settings, but I haven't written down our complete take on it somewhere easily accessible.

[LewisJEllis]

There's kind of a lot to it, and I've done a ton of reading on the matter, but I'll try to summarize all the important/relevant parts:

TLDR:

• Yea, we know, they're "deprecated", but for a nuanced usage of the term "deprecated"
  • Node Core team thinks they should be avoided but are still sort-of-okay to use when absolutely necessary, with awareness that they will eventually be replaced
  • Node Core team has clearly stated that domains absolutely will not be removed from Node until a viable replacement for them exists
  • They continue to be used by a number of libraries in these necessary cases
  • They are relatively respected by a number of major ecosystem libraries (mainly DB drivers) which need to go out of their way to avoid poor interactions with domain propagation
• We've been using them for a good while; they're the only/most viable way to do some things we need to be doing
• We're paying close attention to upcoming replacements based on async_hooks and look forward to migrating away from domains when viable

The long version

On domains being documented as "deprecated"

• when that deprecation happened, the Node core team's opinion was basically:
  • we don't like domains, they have issues and caveats and gotchas
    • their implementation is hairy and we don't want to try to fix problems with them
  • we plan to eventually replace them entirely, and they shouldn't be used unless they're absolutely necessary
    • so the average end-user of Node probably shouldn't be messing with them
      • so we'll mark them as deprecated in the docs to deter people and make sure people know they're gonna get replaced...eventually
    • BUT there are certainly use cases where they are absolutely necessary and valid to use (for now, until a replacement)
      • though people working in those use cases need to be aware of their caveats, potential complications, etc
  • a domains postmortem was written on problems, trickinesses, etc
• note this is my interpretation and summary from memory of having read multiple long discussion threads
  • so there may be important points I haven't represented here; I welcome corrections/clarifications
  • but I think this is an accurate enough summary that continues to hold as the stance on domains today
• bottom line: the Node Core team has clearly stated that domains absolutely will not be removed from Node until a viable replacement for them exists
  • so we're ultimately (cautiously) okay with relying on them until that replacement exists

On our current usage of domains

• We've been using them for nearly two years in our Express middleware, since long before breadcrumbs
  • to capture+properly associate asynchronous errors with the request they came from
• Recent context functionality has expanded our usage of domains to enable breadcrumbs
  • but largely just generalization of our previous express middleware use pattern
• Context functionality doesn't currently work with promises
  • see Breadcrumbs & context wrapping broken with native promises #265, see also promises prevent domain propagation issue on node archive
  • currently investigating viability of a monkeypatch workaround to satisfy common cases

On potential replacements

• continuation-local-storage also provides asynchronous context propagation capabilities
  • built on top of experimental async-listener API from node 0.11, the precursor to async_hooks
  • A good async-listener shim exists, so we could use it, but...
• One of our primary concerns on any alternative backing to our context functionality is avoiding instances of the userland queueing problem (ULQP) as much as possible
  • See this document for a primer on the userland queuing problem and various async context propagation techniques
  • See my examples of the ULQP occurring in Node and a comparison between domains and CLS in those instances
  • CLS unfortunately isn't widely recognized in the ecosystem
    • many popular DB drivers and other libraries cause ULQP with CLS
      • PRs to many such libraries to propagate CLS context have been rejected for various reasons
      • compatibility patch modules exist for various libraries, but aren't a viable solution for our needs
  • Domains OTOH are relatively well-recognized/respected in the ecosystem
    • most popular DB drivers and other libraries that might cause ULQP make sure to bind the active domain to avoid it
    • this is what I mean when I say "relatively respected by a number of major ecosystem libraries"
• async_hooks is an upcoming Node core feature that will be the foundation on top of which a proper domain replacement can be built
  • core PR, EPS proposal
  • will hopefully land soon, and then a CLS-like functionality can be built on top of it
• we are 100% looking to migrate to an async_hooks-based CLS-like functionality ASAP
  • recent "What will Domain be replaced with?" issue
  • first we need replacement to be recognized and respected by ecosystem (mainly DB drivers etc) to minimize ULQP

On staying clear of domains with raven-node:

• This would require avoiding both the express middleware and the context, wrap, get/set/mergeContext methods
• This means breadcrumbs, user context, etc would be limited to global scope only
  • Probably acceptable in one-off scripts or lambda scripts
  • Probably not acceptable in express/server/long-running use cases

Summary

I'm happy to go into more in-depth discussion on any of these bullets or on any of the concerns/decision points that went into the stance we have. Ultimately, we'd rather not be using domains, but to provide the user experience we're aiming for:

• simple installation/setup
• avoid asking users to make heavy modifications to their code
• interact well with most ecosystem libraries out of the box so users don't run into all sorts of random incompatibilities with their dependencies
  • with the notable exception of Promises; we work with Bluebird but not with native promises and we knew this but failed to document it (see Breadcrumbs & context wrapping broken with native promises #265)

we believe domains are currently the most viable solution. We very much look forward to migrating to something based on async_hooks once that option is similarly viable and can provide the same user experience.

/cc @DerekSeverson @benvinegar

todo before closing this issue: put some sort of condensed take on this + link to this issue in our docs in some relevant spot.

[DerekSeverson]

Thank you so much for the detailed response! It was exactly what I was looking to find out. It will be interesting to see if the node.js team can come up with a viable alternative. Thank you for your work on this library/integration with Sentry - it's a great solution our team is using on all our node projects.

[aemino]

Node v8 has been released and async_hooks is finally out. Are there plans for implementing a new alternative to domains?

[benvinegar]

AFAIK, Node 8 also added new functionality that continues to support domains. Despite their deprecated status, it doesn't seem like domains are going away anytime soon. From the changelog:

Native Promise instances are now Domain aware [84dabe8373] #12489.

Re: plans for an alternative – it's on our radar, and might take the form of a Node 8 (and up) specific version of raven-node. But it's not an immediate priority, yet.

[aemino]

I was just wondering if there were plans to implement an alternative. I'm glad that native promises are now domain aware; one of the issues I've been having is that async functions would not be able to set their context properly. I assume this is an issue with the domain module and promises not working well together... I'll have to test how well it works with Node 8 though. Would the new Node change work immediately, or is there something in node-sentry that needs to be updated first?

[benvinegar]

Would the new Node change work immediatel

We haven't tested yet – I don't honestly know.

[niftylettuce]

Should we use https://github.com/btford/zone.js/ as an alternative?

[jordaaash]

-1 from me on Zone.js. I moved to Sentry for Node from Opbeat because Zone screws up Bluebird promises completely.

[kamilogorek]

I'm not willing to use Zone.js anytime soon as well. We'll start to think about something when Node will officially announce that they will remove domains.

[jdrydn]

What if the context was portable? IMO I would much prefer to attach a (clean) Sentry context to my request, and in my own error handler pass that context to the captureException method in order to avoid domains.

For example, with Koa:

app.use(async errorHandler(ctx, next) => {
  ctx.raven = Raven.createContext();

  try {
    await next();
  } catch (err) {
    Raven.captureException(err, {
      context: ctx.raven,
    });

    ctx.status = err.status || 500;
    ctx.body = err.message || `${err}`;
  }
});

app.use(async ctx => {
  const { id, username, email } = await authenticate(ctx.get('authorization'));
  ctx.raven.setUser({ id, username, email });
  ctx.raven.captureBreadcrumb({ message: `Authenticated as user #${id}` });

  ctx.throw(403, 'These are not the droids you are looking for, move along');
});

Or with Express:

app.use((req, res, next) => {
  req.raven = Raven.createContext();
  next();
});

app.use((req, res, next) => {
  authenticate(req.get('authorization'), (err, user) {
    if (err) return next(err);

    const { id, username, email } = user;
    req.raven.setUser({ id, username, email });
    req.raven.captureBreadcrumb({ message: `Authenticated as user #${id}` });

    const err = new Error('These are not the droids you are looking for, move along');
    err.status = 403;
    next(err);
  });
});

app.use((err, req, res, next) => {
  Raven.captureException(err, {
    context: req.raven,
  });

  res.status(err.status || 500).json(err.message || `${err}`);
});

(I understand that this sort of solution requires a little more code for new users, but personally I would prefer that over a "magic" .install() function that hooks into all sorts)

In my mind after reading the clientdev documentation I assumed this library would be able to get/set the context in a similar manner to above, however I can't see any documentation supporting a context outside of Raven.context(...), which I'm trying to avoid as it uses domains.

[kamilogorek]

@jdrydn we are working on making this happen soon, in the next major release. Thanks for the extensive write-up!

[jdrydn]

@kamilogorek Great to hear! What's the timetable for the next major release? Also happy to lend a hand ✍️

[kamilogorek]

@jdrydn hard to tell. We're aiming at the end of Q2 for the 1.0 version of "next" version.
You can follow along with the progress here - https://github.com/getsentry/raven-js/tree/next/packages
Thanks! :)