feat: expose setPromiseHook support #8209

benjamingr · 2020-10-31T15:15:38Z

This is a draft PR to expose promise hooks enabling APMs.

My rust is bad, I haven't used it in ±2 years, any criticism and feedback about the code below is very appreciated.

Still missing and need to do:

Document Deno.core.setPromiseHook
Refactor to use js_recv_obj to avoid the global (currently exposed).
Expose the constants used by the promise hooks (INIT, RESOLVE, BEFORE, AFTER)
Add tests (copy the Node or V8 promise tests probably?):
- Test thenables
- Test an async function
- Test a "raw" promise
- Test a promise subclass
Get APM vendor feedback in order to ensure this API "looks good".
Run formatter and linter

Nice to have in the future but out of scope:

Shim async_hooks using this in deno_std
Guide on how async hooks work

benjamingr · 2020-10-31T15:41:00Z

@bartlomieju I've changed the code to use the same machinery as js_recv_cb (I hope) and it works. Would you mind taking a look and making sure I understood it correctly? (to be clear: none of this is urgent)

benjamingr · 2020-10-31T15:45:05Z

Question: I noticed I can't find references to other Deno.core stuff in the docs - where (at all?) should this be documented?

bartlomieju · 2020-10-31T17:01:51Z

@bartlomieju I've changed the code to use the same machinery as js_recv_cb (I hope) and it works. Would you mind taking a look and making sure I understood it correctly? (to be clear: none of this is urgent)

@benjamingr yeah, looks great!

bartlomieju · 2020-10-31T17:02:52Z

Question: I noticed I can't find references to other Deno.core stuff in the docs - where (at all?) should this be documented?

Deno.core does not have type declarations at the moment - this is on purpose as we consider Deno.core unstable and that it can change at any moment. Let's ponder at this a bit more and decide what to do next when this PR is ready to land.

benjamingr · 2020-11-01T12:38:00Z

After a comment by @Flarna I am convinced the correct behaviour here is to allow multiple consumers - so I will replace the Option to a Vec and add a test for multiple consumers.

benjamingr · 2020-11-01T12:40:12Z

Also, on second thought I don't think it makes sense to unit test the behaviour in various cases I mentioned - but rather just our API surface (that is, test multiple consumers but not the exact promise-hook behaviour of promise subclasses for example) since it's tested on the V8 side.

I would like to (eventually) see an integration test (with a complex promise context tracking use case) added but I think the tests I've added probably cover the basic cases.

benjamingr · 2020-11-01T12:50:41Z

Ok, this is about ready for review now @bartlomieju thanks for the help!

I feel strongly that this should be documented somewhere though, even if it just points to the V8 docs. I will happily write docs for it if you tell me where to put them :]

kitsonk · 2020-11-02T23:53:49Z

@benjamingr @bartlomieju my opinion on the documentation is that we should move embedding_deno to a new Advanced section in https://github.com/denoland/deno/tree/master/docs and add another document there about "Performance Monitoring" with this being the initial content.

bnoordhuis

LGTM insofar I'm able to judge deno_core changes.

There's no mechanism to remove a promise hook again. Personally, I'm fine with leaving that out for now but I thought I'd mention it anyway.

bnoordhuis · 2020-12-04T09:00:27Z

core/bindings.rs

+  for (i, hook_name) in
+    ["INIT", "RESOLVE", "BEFORE", "AFTER"].iter().enumerate()


Suggested change

for (i, hook_name) in

["INIT", "RESOLVE", "BEFORE", "AFTER"].iter().enumerate()

use v8::PromiseHookType as Ty;

for (hook_name, value) in

[("INIT", Ty::Init), ("RESOLVE", Ty::Resolve), ("BEFORE", Ty::Before), ("AFTER", Ty::After)].iter()

(and maybe sort them)

If the original name is good enough you can also do:

for v in [Ty::After, Ty::Before, Ty::Init, Ty::Resolve].iter() { let name = format!("{:?}", v); // ... }

bnoordhuis · 2020-12-04T09:04:56Z

core/bindings.rs

+      let msg = v8::String::new(scope, "Invalid argument").unwrap();
+      let exception = v8::Exception::type_error(scope, msg);
+      scope.throw_exception(exception);
+      return;


For a future PR: consider moving this into a helper function. I count 5 or 6 other instances of the exact same pattern.

bnoordhuis · 2020-12-04T09:10:29Z

core/bindings.rs

+  for js_promise_hook_cb_handle in state_rc.borrow().js_promise_hook_cb.iter() {
+    js_promise_hook_cb_handle
+      .get(scope)
+      .call(scope, global.into(), &[hook_type, promise.into(), parent])


Maybe use v8::undefined().into() instead of global.into()? Passing global is not wrong and it seems to be the predominant pattern in deno_core but it's marginally less efficient.

bnoordhuis · 2020-12-04T09:11:08Z

core/bindings.rs

+    js_promise_hook_cb_handle
+      .get(scope)
+      .call(scope, global.into(), &[hook_type, promise.into(), parent])
+      .unwrap();


You should probably not unwrap here unless you want exceptions from the promise hook to terminate the process (with a fairly opaque error message.)

stale · 2021-02-02T09:30:51Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

brillout · 2021-02-02T09:37:11Z

Any news on this? (I'm interested in porting https://github.com/reframejs/wildcard-api to Deno but for that I need async hooks.)

benjamingr · 2021-02-02T09:49:33Z

I am unfortunately in the process of switching jobs and moving houses and working on kids so I won't be able to work on this soon - other people are welcome to pick this up in the meantime.

brillout · 2021-02-02T09:52:17Z

Ok I see, thanks for the update.

bartlomieju · 2021-07-06T13:05:14Z

@bnoordhuis @benjamingr maybe we should wait for the SetPromiseHooks(init, before, after, resolve) API? (referenced in https://docs.google.com/document/d/1vtgoT4_kjgOr-Bl605HR2T6_SC-C8uWzYaOPDK5pmRo and nodejs/node#36394)

benjamingr · 2021-07-20T09:42:49Z

It most likely should

bartlomieju · 2021-07-23T12:22:14Z

It most likely should

Great, I'll close this PR for now, as it became stale. Once SetPromiseHooks() lands in V8 we can use this PR as a reference point for the implementation. Thank you @benjamingr!

benjamingr · 2021-07-24T11:57:42Z

That sounds correct.

jayphelps · 2021-09-15T18:08:07Z

@bartlomieju Is this the hook? https://source.chromium.org/chromium/_/chromium/v8/v8.git/+/c0fceaa0669b39136c9e780f278e2596d71b4e8a

bartlomieju · 2021-09-15T18:11:04Z

Yes, I believe it is

bartlomieju · 2021-10-01T09:34:43Z

@benjamingr are you still interested in this functionality? I think we could try to tackle this once more with the API linked above.

benjamingr · 2021-10-01T09:53:22Z

@bartlomieju I think Deno isn't actually production-ready until this API exists because it means it is impossible to write performance monitoring and analysis tools - it means users don't actually know where their production apps are spending time reliably which is a must for big real-world apps in my experience.

So yes - since I would like to see Deno used for production apps I am interested in this :) I've had Deno apps running "in production" since 2018 but I haven't been able to use it for anything substantial because of the lack of monitoring capabilities.

That said, I don't have time to work on this and I think your best approach to get this ready quickly would be to talk to APM vendors like New Relic or Elasticsearch and ask them to implement this. It's probably a good idea to have a discussion with APM vendors since they're the ones likely to eventually use this API the most.

Alternatively Deno can choose not to expose this functionality to end-users directly and provide its own APM offering which is also fine probably.

benjamingr mentioned this pull request Oct 31, 2020

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

Closed

benjamingr force-pushed the promise-hooks-expose branch from d79f6dc to a6c056b Compare October 31, 2020 16:51

benjamingr marked this pull request as ready for review November 1, 2020 12:49

benjamingr mentioned this pull request Nov 10, 2020

fix(web/worker) define event handlers in DOM order #8334

Merged

add promise hook, expose

0e0977a

benjamingr force-pushed the promise-hooks-expose branch from b22121c to 0e0977a Compare November 14, 2020 12:39

bnoordhuis reviewed Dec 4, 2020

View reviewed changes

stale bot added the stale label Feb 2, 2021

stale bot removed the stale label Feb 2, 2021

kitsonk added the feat new feature (which has been agreed to/accepted) label Feb 2, 2021

Base automatically changed from master to main February 19, 2021 14:58

xialvjun mentioned this pull request May 13, 2021

Support continuation/async local storage #7010

Closed

bartlomieju self-assigned this Jul 6, 2021

bartlomieju closed this Jul 23, 2021

vmarchaud mentioned this pull request Feb 6, 2022

Deno Support open-telemetry/opentelemetry-js#2293

Open

legendecas mentioned this pull request Apr 18, 2022

feat: add binding for Context::SetPromiseHooks denoland/rusty_v8#938

Merged

lbguilherme mentioned this pull request Aug 15, 2022

feat(core): add Deno.core.setPromiseHooks #15475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose setPromiseHook support #8209

feat: expose setPromiseHook support #8209

benjamingr commented Oct 31, 2020 •

edited

Loading

benjamingr commented Oct 31, 2020

benjamingr commented Oct 31, 2020

bartlomieju commented Oct 31, 2020

bartlomieju commented Oct 31, 2020

benjamingr commented Nov 1, 2020

benjamingr commented Nov 1, 2020

benjamingr commented Nov 1, 2020

kitsonk commented Nov 2, 2020

bnoordhuis left a comment

bnoordhuis Dec 4, 2020

bnoordhuis Dec 4, 2020

bnoordhuis Dec 4, 2020

bnoordhuis Dec 4, 2020

stale bot commented Feb 2, 2021

brillout commented Feb 2, 2021

benjamingr commented Feb 2, 2021

brillout commented Feb 2, 2021

bartlomieju commented Jul 6, 2021

benjamingr commented Jul 20, 2021

bartlomieju commented Jul 23, 2021

benjamingr commented Jul 24, 2021

jayphelps commented Sep 15, 2021

bartlomieju commented Sep 15, 2021

bartlomieju commented Oct 1, 2021

benjamingr commented Oct 1, 2021

		for (i, hook_name) in
		["INIT", "RESOLVE", "BEFORE", "AFTER"].iter().enumerate()

-  for (i, hook_name) in
-    ["INIT", "RESOLVE", "BEFORE", "AFTER"].iter().enumerate()
+  use v8::PromiseHookType as Ty;
+  for (hook_name, value) in
+    [("INIT", Ty::Init), ("RESOLVE", Ty::Resolve), ("BEFORE", Ty::Before), ("AFTER", Ty::After)].iter()

feat: expose setPromiseHook support #8209

feat: expose setPromiseHook support #8209

Conversation

benjamingr commented Oct 31, 2020 • edited Loading

benjamingr commented Oct 31, 2020

benjamingr commented Oct 31, 2020

bartlomieju commented Oct 31, 2020

bartlomieju commented Oct 31, 2020

benjamingr commented Nov 1, 2020

benjamingr commented Nov 1, 2020

benjamingr commented Nov 1, 2020

kitsonk commented Nov 2, 2020

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Dec 4, 2020

Choose a reason for hiding this comment

bnoordhuis Dec 4, 2020

Choose a reason for hiding this comment

bnoordhuis Dec 4, 2020

Choose a reason for hiding this comment

bnoordhuis Dec 4, 2020

Choose a reason for hiding this comment

stale bot commented Feb 2, 2021

brillout commented Feb 2, 2021

benjamingr commented Feb 2, 2021

brillout commented Feb 2, 2021

bartlomieju commented Jul 6, 2021

benjamingr commented Jul 20, 2021

bartlomieju commented Jul 23, 2021

benjamingr commented Jul 24, 2021

jayphelps commented Sep 15, 2021

bartlomieju commented Sep 15, 2021

bartlomieju commented Oct 1, 2021

benjamingr commented Oct 1, 2021

benjamingr commented Oct 31, 2020 •

edited

Loading