Support/make use of Async Hooks

[AdriVanHoudt]

• [x] Review the documentation: https://docs.sentry.io/
• [x] Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
• [x] Use the latest release: https://github.com/getsentry/sentry-javascript/releases
• [ ] Provide a link to the affected event from your Sentry account

Package + Version

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

Version:

6.5.1

Description

I've talked about this extensivly in https://github.com/getsentry/sentry-javascript/issues/2172. But since that issue seems to have stalled and others are also seeming to request this (see https://github.com/getsentry/sentry-javascript/issues/2817#issuecomment-756864140) I thought it would be nicer to open an official dedicated issue.

Why:

• Better and out of the box propper transactions and context handling.
• Should fix issues like breadcrumbs not being properly attached to the right error/transaction.
  • See https://github.com/getsentry/sentry-javascript/issues/3120 and https://github.com/getsentry/sentry-javascript/issues/2817#issuecomment-855163926
• Should fix issues of child spans not being correctly attached to the right transaction.
  • See https://github.com/getsentry/sentry-javascript/issues/2172#issuecomment-850447362
• Domains are deprecated and have performance and error handling issues.
  • See https://github.com/getsentry/sentry-javascript/issues/2172#issuecomment-845804452
• Domains don't seem to actually properly work.
  • See https://github.com/getsentry/sentry-javascript/issues/2172#issuecomment-850447362

Some examples of agents who do use Async Hooks:

• googleapis/cloud-trace-nodejs
  • I can personally confirm this one works out of the box and does tracing correctly
• honeycombio/beeline-nodejs
• elastic/apm-agent-nodejs
• https://github.com/DataDog/dd-trace-js/tree/master/packages/dd-trace

Atm I don't see how one can use Sentry's performance offering using Node without this being fixed (unless you only serve 1 request at the same time, which isn't realistic). I hope I'm missing something since we do want to use your offering but don't see a way of doing that at this time.

[mitsuhiko]

I will reply in detail later bit we are already using domains to solve this. Express sets up a domain per request and for as long as you use domains correctly we should be using the tight isolation.

We are going to also support async hooks in the future but fundamentally they were solving the same issue.

[AdriVanHoudt]

Could you have a look at https://github.com/getsentry/sentry-javascript/issues/2172#issuecomment-850447362? I tried using domains but couldn't get it to work. Maybe I'm doing it wrong?

[jpike88]

I can confirm elastic/apm-agent-nodejs (which we've been using for some time before considering sentry's offering) works like a charm.

Is this connected to the strange bit of code that's appears to be needed to make a transaction async aware? I've used a few APM libraries now and this is the first time I had to include this just to have what I consider to be default/basic behaviour to kick in:

				Sentry.configureScope((scope) => {
					scope.setSpan(transaction);
				});

[mitsuhiko]

You need to wrap your code in domain.run(…) for the domain to be active.

[AdriVanHoudt]

There is no function to wrap in hapi's case, as far as I understood domain.enter would do the same, is that not the case? https://nodejs.org/docs/latest-v14.x/api/domain.html#domain_domain_enter (it seems like breadcrumbs do work better with the domain code but not tested in depth)

[AdriVanHoudt]

Another thing I found that might be interesting for this is https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-context-async-hooks

[kamilogorek]

I've never built anything with Hapi before, so I don't know it's architecture, but moving away from domains in favor of async_hooks is definitely on our roadmap. It won't be a quick think though, as domains are in the very core of the SDK, and a lot of moving pieces rely on them.

From what I see https://github.com/hydra-newmedia/hapi-sentry are also using domains in their plugin, but not sure how accurate it is. If there's something specific that's currently not working, I'm more than happy to look into finding a workaround if a small repro case is provided.

[AdriVanHoudt]

I understand it won't be quick but I don't see the current implementation working.

I also based my implementation on https://github.com/hydra-newmedia/hapi-sentry. I didn't test this with a minimal version but using latest hapi + the code from https://github.com/getsentry/sentry-javascript/issues/2172#issuecomment-850447362 + https://github.com/Salesflare/hapi-plugin-mysql is probably it. What I observed is that, when using multiple connections it stopped properly assigning the db spans to the right transaction.

[kamilogorek]

Thanks, I totally get your use case explanation, but because I've never used Hapi before, could you please gather those pieces you described above together and put into a single gist file that I could run locally?

[AdriVanHoudt]

@kamilogorek I unfortunately don't have the time atm to provide you with a full example. We decided to not put our efforts into this and instead use a solution that just works. We'd be happy to revisit Sentry for apm once this gets resolved though.

[CesarLanderos]

Hi! is this been worked on? Domain is deprecated by node and we would like to not depend on it on our project.

thanks in advance!

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[AdriVanHoudt]

Please don't close this issue, it is still relevant

[jasonk]

The big problem that async_hooks would allow solving is that domains are a much larger scope. You can use a domain to say "If anything that happens in this code throws an error then report it with this context information", but everything that runs within that domain is all considered to be part of the same transaction, so if you have async methods all their breadcrumbs and spans and context are going to get mixed together.

The other thing about this is that people who care about this problem are very likely already using async_hooks. In my case my application already has a context manager where I can do things like ctx.run( { transaction_id }, () => { /*...*/ } ) and the context information passed in the first argument will be available to all the async activity that happens as a result of that. In order to get Sentry to work the way I want it to, I don't even need it to migrate to async_hooks, I just need it to provide a way to let me use async_hooks.

For my use case it would be enough if Sentry provided a way to register a function to be called when it needs to get the current scope, then I could just wrap that up with my own context provider and everything would work. Right now what happens (mostly) is that it calls hub = getCurrentHub() to get the hub associated with the current domain, and then calls hub.getStackTop() to get the scope at the top of it'1 stack. However, this assumes that all of the code that is doing that is synchronous. If there were a way to make it ask me what scope to use instead then it would all just work.

You can almost get it to work by replacing the getCurrentHub() method exported from @sentry/hub with one that returns a new Hub with just the appropriate scope.

[jasonk]

For anyone else who ends up here hoping to find a way to make Sentry work with async_hooks, I posted a Gist showing how I made this work by using AsyncLocalStorage to create what are effectively thread-local hubs that can make this work better... https://gist.github.com/jasonk/a06153476ae7fad41c527e321e318088

[frimuchkov]

Any updates? There are 2022, Node.js 18 and no async_hooks in sentry :(

[AbhiPrasad]

Hey - we're finally working on this! Take a look at https://github.com/getsentry/sentry-javascript/issues/7691 to see our migration path.

This should be fully backwards compat, and require no changes to existing code.

Node 12 and below will still continue using domains, Node 14 and above will get AsyncLocalStorage!

Would also like to remind everyone though that domains actually use async_hooks under the hood.

[AbhiPrasad]

With the 7.48.0 release of the JS SDK we use AsyncLocalStorage instead of domains under the hood.

If you want to manually wrap methods in an async context to isolate breadcrumbs/scope/spans, you can use Sentry.runWithAsyncContext, which is documented here: https://docs.sentry.io/platforms/node/configuration/async-context/