Support/make use of Async Hooks

[AdriVanHoudt]

• Review the documentation: https://docs.sentry.io/
• Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
• 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
• @sentry/node
• raven-js
• raven-node (raven for node)
• other:

Version:

6.5.1

Description

I've talked about this extensivly in #2172.
But since that issue seems to have stalled and others are also seeming to request this (see #2817 (comment)) 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 All global breadcrumbs appear in all captured exceptions. #3120 and [Call for Feedback] Roadmap to Sentry JavaScript SDK v7 #2817 (comment)
• Should fix issues of child spans not being correctly attached to the right transaction.
  • See Feature request: proper full hapi support #2172 (comment)
• Domains are deprecated and have performance and error handling issues.
  • See Feature request: proper full hapi support #2172 (comment)
• Domains don't seem to actually properly work.
  • See Feature request: proper full hapi support #2172 (comment)

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 #2172 (comment)? 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 #2172 (comment) + 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!