Repository navigation
[async_hooks] stable API - tracking issue #124
Description
Activity
/cc @nodejs/async_hooks @nodejs/diagnostics
/cc @jasnell who sometimes asks me about this
@AndreasMadsen how about adding some external criteria, like TC39 uses or N-API (nodejs/node#14532)
- major APM vendor that uses
async_hooks(N/Solid) - npm modules with at least 5000 DL/month based on
async_hooks(https://www.npmjs.com/package/trace & https://www.npmjs.com/package/cls-hooked)
- major APM vendor that uses
@refack added your 1. and 2. to the list. I'm not sure what you mean by TC39 uses. Regarding N-API I don't see why that should be a requirement.
async_hookshas its own native Embedder API outside N-API, we can mark that as stable without N-API. In fact N-API shouldn't be marked stable before theasync_hooksnative Embedder API is marked stable since they depend on that. Butasync_hooksdoesn't depend on N-API.Thanks. I just referenced TC39 and N-API as processes that use exit criteria that are independent of the development process, but take into account ecosystem adoption. So I agree that stability of N-API and async_hooks are independent (except for the embedder API)
FWIW, We plan to start using Async Hooks in Sqreen Agent in the upcomming weeks.
Link by @watson for elastic: elastic/apm-agent-nodejs#77
PR to add async_hooks in Stackdriver Trace: googleapis/cloud-trace-nodejs#538
Updated "Deprecate setTriggerId"
Added:
- Clarify breaking changes necessary for better performance (issue: Performance impact of async_hooks benchmarking#181)
So based on discussion in the benchmarking WG meeting yesterday I did setup test cases to run the Promise heavy Bluebird and Wikipedia benchmarks (used by the V8 project) with and without
async_hooksto answer the first question in this thread, and slow-down is pretty significant even with just an emptyinithook.@mhdawson suggested to kick-off some discussion on the performance via nodejs/benchmarking#188 and bubble up the issue to make sure we consider the performance aspect before
async_hooksgoes out of EXPERIMENTAL.FYI I added an item to list above to identify the perf impact we're willing to tolerate from async-hooks. Current benchmark data cited in nodejs/benchmarking#181 show a ~2x-3x slowdown.
Any thoughts on if this will be coming out of Experimental before v10 launches? Looking at the remaining items I can't quite tell if there are still significant barriers because I'm not familiar enough with the subject 🤔
We haven't run into any blockers in the New Relic agent.
The primary concern we've come up against is an extension of the lifetime of a promise leads to drastically increased memory usage. This only seems to be an issue with immediately resolved promises (e.g.
Promise.resolve()), since those don't emit anafterevent and have to be cleared ondestroy. This situation only causes an issue when there is an existing promise leak and just requires us to be more diligent about finding and eliminating leaks in libraries we interact with. I don't think this issue affects the shape of the API, so it's definitely not a blocker.We currently only instrument core promises using async hooks, though the data exposed through the hooks should be enough to move over the rest of the core instrumentation when the feature is fully released.
19 remaining items
- added a commit that references this issue
on Jan 14, 2020 - added a commit that references this issue
on Jan 16, 2020 - added a commit that references this issue
on Feb 6, 2020 - added a commit that references this issue
on Mar 14, 2020 This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
I figure this shouldn't go stale since it's a long-lived tracking issue?
Reacted by Tierney Cyren and Andreas MadsenHey team, sorry it has taken me so long to follow up. Last time we spoke I believe you asked me to experiment with the
async_hooksAPI more. We now have anasync_hooksimplementation in production in our Node.js integration - although it is not much different (if at all) from Cloud Trace's implementation.I have some thoughts and feedback on the API that it would be great to sync with you all on at some point (if you have time), but it would be good to know if there's anything further you'd like me to do to move this forward?
@xadamy maybe you can plan to come ot the next diagnostics meeting and give the team a runthrough of your thoughts.
I posted requirements list for getting
async_hooksinto stable a while ago (nodejs/node#14717 (comment)). This is a very similar list.Internal Technical
promiseobject fromPromiseWrap(PR: async_hooks: remove promise object from resource node#23443)MicrotaskWrap" instead ofPromiseWrap(issue: Proposal for reworking promise integration into async_hooks #389)async_hooksand see that there is only a small measurable difference given our tolerance.runInAsyncIdScope(issue: Remove async_hooks runInAsyncIdScope API node#14328, PR: async_hooks: deprecate undocumented API node#16972)setTriggerId(issue: Remove async_hooks setTrigger API node#14238, PR: async_hooks: deprecate undocumented API node#16972)async_hooksstate validation in v9.x. (PR: async_hooks: enable runtime checks by default node#16318)MakeCallbackhave anasync_context.async_hooksintoNAN(issue: Integrate C++ AsyncHooks Embedder API with native abstraction node#13254).async_hooksinton-api(issue: Integrate C++ AsyncHooks Embedder API with native abstraction node#13254).Internal Non-Technical
triggerAsyncIdchange will be asemver-major,semver-minororsemver-patch.External Non-Technical
async_hooks(N/Solid, New Relic, Elastic APM)async_hooks(https://www.npmjs.com/package/trace & https://www.npmjs.com/package/cls-hooked)After Stable
MakeCallbackwithoutasync_context(PR: src: deprecate legacy node::MakeCallback node#18632)