APMs in Deno -or- Async hooks and isolate->SetPromiseHook?

[benjamingr]

Hey,

One of the features Node uses from V8 is promise hooks and in particular Node's async_hooks lets users keep context enabling a lot of use cases (like APMs).

I saw the last issue but it doesn't look like there was a lot of discussion there. As far as I'm aware there is no analogy in the DOM/browser world for this. When I talked to some tooling people in browser vendors they said they were not aware of a direct analogy.

I would recommend (barring actual discussions with APM vendors or telling users to use a user promise library and instrumenting its scheduler):

• Exposing SetPromiseHook pretty directly and letting users and vendors deal with it.
• Exposing an async_hooks like API - possibly based on Zones (or hopefully another web proposal).
• Exposing only the node async hooks API under std/node.
• Wait for vendors and complain and not solve it until APM vendors or users complain (then use their feedback to pick an API).
• Something else entirely.

It's true async_hooks are experimental and domains are deprecated but without neither - it is pretty impossible to build an APM for Node. You have to instrument methods to know "where you came from" in an async context and APMs are pretty valuable tools.

[kitsonk]

Hmmm... fine grained understanding of async processes seems like something that would be desirable. The analogue in the browser is what is available in the dev tools, it is seldom exposed in user land (the exception would be the Performance APIs).

We have Deno.metrics which is sort of in this space, but what is talked about where is a broader API.

[PatrickJS]

Please consider Zone.js API for Deno

https://gist.github.com/mhevery/63fdcdf7c65886051d55

https://github.com/domenic/zones/tree/eb65c6d43b452a877c24561cd64c6901e790ecf0

[benjamingr]

Hey @ry @kitsonk - I was speaking to a large'ish company today and this issue was the show-stopper for them in their case.

I'm happy to help and work on this. If your preference is that the next step is that I make a PR - I am happy to do that.

Is there a preference for how this should look like? I think the clearest API would be:

• Exposing SetPromiseHook pretty directly and letting users and APM vendors deal with it.

Deno.metrics.setPromiseHook

That exposes

Isolate::SetPromiseHook

Also see the API document in case you are not too familiar with it.

I am also happy to ask some Node promise hooks and diagnostics people for guidance regarding what API would be optimal here.

[benjamingr]

@PatrickJS note that the use case you are describing (tracking state across context) is solving a bigger and different issue than what APMs need/want.

[kitsonk]

cc/ @bartlomieju @bnoordhuis @piscisaureus I know there is a lot of chat at the moment about the resource table and how we are going to be handling promises... feels like it maybe worth considering the APM needs as well?

[bnoordhuis]

v8::Isolate::set_promise_hook() is available (denoland/rusty_v8#496 - in deno now) so I suppose that's a first step?

Having been both an APM vendor and a maintainer of Node.js, it's been my experience that low-level hooks are enough.

[benjamingr]

Oh great (also hi!), I missed that.

So the only bit remaining is exposing this to the typescript bit?

Having been both an APM vendor and a maintainer of Node.js, it's been my experience that low-level hooks are enough.

Cursory emails to APM vendors seemed to indicate a strong preference to low level hooks as well.

[bartlomieju]

Exposing SetPromiseHook pretty directly and letting users and APM vendors deal with it.
Deno.metrics.setPromiseHook

@benjamingr this API needs direct binding to V8 API so to keep consistent with other such APIs (Deno.core.getProxyDetails, Deno.core.getPromiseDetails) it should live in Deno.core namespace.

[benjamingr]

Would it help if I tried working on this? If Ben already has something in mind - it'll probably be easier for him to do it.

If it would help I'm happy to try adding a Deno.core.setPromiseHooks

[bartlomieju]

Would it help if I tried working on this? If Ben already has something in mind - it'll probably be easier for him to do it.

If it would help I'm happy to try adding a Deno.core.setPromiseHooks

@benjamingr sure! Look in core/bindings.rs for the methods listed above

[benjamingr]

Hmm, question - in rust closures and functions are different - in Ben's PR he uses a global name (hook) to store the hook in JS land and then "get" it in TS land.

I found very little cases of .call - stuff like queueMicrotask already takes v8::Function and doesn't have this problem.

Is there a standard way to do this in Deno? (Store callbacks to call later?) I can store Deno.core._hookCallback (that's the approach I initially took) but I'm wondering if there is something better?

[bartlomieju]

@benjamingr take a look at JsRuntime::js_recv_cb, it is set using Deno.core.recv. I think you can take similar approach

[benjamingr]

Thanks, I'll do that, I have something ready with the global approach, I will refactor it and try to use the same approach as js_recv_cb and open a PR (not sure it'll be today, but probably tomorrow).

Note my rust is pretty terrible 😅 so any sort of feedback is good feedback so I can improve it :]

I'll open a draft PR with a task list for myself.

Edit: draft at #8209

[benjamingr]

API question: would should be the behaviour when someone calls setPromiseHook twice?

We can either:

• Replace the old hook (might be surprising behaviour)
• Throw an error (might be an issue if multiple consumers try setting up hooks)
• Keep a list of callbacks and call all of them?

This is more of a "for the future" concern I guess.

[Flarna]

I would vote to add a thin layer to allow multiple users.

There are several usecase for context tracking therefore this functionality should not be consumed by a single user. In special APMs tend to act as hidden as possible therefore it's a no go to take a global resource.

[benjamingr]

Ok, I am convinced @Flarna - it makes sense people would want something like cls-hooked and an APM at the same time that sounds reasonable. I'll refactor it to use a Vec<v8::Global<v8::Function> and not a Option<v8::Global<v8::Function>> and make the code work with multiple callbacks.

Is there a compelling use case for removing hooks? That is also not currently supported in the API.

I thought we can use an EventTarget for this but that sounds very magic since listening to an event would need to set up the promise hook (to not pay the penalty without it).

Checking with the V8 team - it doesn't look like these events are exposed in browsers and there is no web standard to follow.

[benjamingr]

Ok, I think the PR is about ready #8209

I am not sure about how tests should work for testing a "directly exposed" V8 API (more tests? fewer tests?) I personally think that an integration test of hooks on a "real world" app would be very useful in identifying APMs breaking on V8 updates.

Another open issue is docs. I left a comment on the PR.

[Flarna]

@benjamingr I don't think removing is really needed but a nice to have. Usually APM tools or something like cls-hooked are not removed from a running process. If they result in a problem you usually have to restart the process anyway.

But it's at least helpful for testing as it avoids that your test changes a global state.

[benjamingr]

Ok, I don't mind adding removing to another PR - right now what I need is more eyes on the PR - I think it's ready for some reviews and hopefully to land.

I think the part missing from Deno after it lands is the "non v8 bits" for context.

In Node, this is typically done by wrapping objects (like overriding Deno.readFile) but in Deno that is impossible to do (it's a read only property).

Might be obvious to you - but this means that if tools need to track context not just across async functions and promise chains - but also across sockets and file operations - deno needs an API for that. That is, when you do a jsonOpAsync you need it to know "something" before/after the call.

[austinhallock]

What are the next steps for this now that denoland/rusty_v8#938 is merged in?

Sounds like it involves reworking #8209 to use the new SetPromiseHooks(init, before, after, resolve) API.

So end result should look something like this using context.setPromiseHooks (from rusty_v8 PR)?

type PromiseHook = (promise: Promise<unknown>, parent?: Promise<unknown>) => void
Deno.core.setPromiseHooks(
  init: PromiseHook,
  before: PromiseHook,
  after: PromiseHook,
  resolve: PromiseHook
);

Opposed to the previous PR's

Deno.core.setPromiseHook((type: number) => ... )

Is that correct?

[bartlomieju]

@austinhallock that sounds about right