Open DerekSeverson opened 7 years ago
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.
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:
context
, wrap
, get/set/mergeContext
methodsI'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:
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.
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.
Node v8 has been released and async_hooks
is finally out. Are there plans for implementing a new alternative to domains?
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.
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?
Would the new Node change work immediatel
We haven't tested yet – I don't honestly know.
Should we use https://github.com/btford/zone.js/ as an alternative?
-1 from me on Zone.js. I moved to Sentry for Node from Opbeat because Zone screws up Bluebird promises completely.
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.
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.
@jdrydn we are working on making this happen soon, in the next major release. Thanks for the extensive write-up!
@kamilogorek Great to hear! What's the timetable for the next major release? Also happy to lend a hand ✍️
@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! :)
From my understanding the node.js core domain module has been deprecated since v4.
Would you mind fielding the following questions...
Apologies if this is not the right place to ask about this topic or if its been covered somewhere else.
Thank You!