-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[async-hooks] request for feedback #129
Comments
Or non-native modules. But where they have a queue. |
I have an issue here detailing my attempt to move from CLS-based-on-domains to CLS-based-on-async-hooks. The connection pooling problem prevents me from using async hooks at present. |
Yeah, userland queuing is a major problem. The majority of userland queuing happens via the generic-pool, so most APM vendors just patch that, but there are definite exceptions. |
Our usage of async-hooks is limited to instrumenting native promises when used with async/await. We had a few customers complain of a memory leak when using our async-hooks instrumentation. Upon investigation this turned out to actually be a promise leaked through a closure in pg-pool. Async-hooks simply exacerbated what would otherwise have been a small leak since the We have had no other reports of issues regarding async-hooks, and Node 8 (where async-hooks is on by default) makes up more than half of our usage. On the performance side, things are a little different. In our contrived benchmark of resolving a 300-promise chain simply enabling async-hooks causes the test to perform at ~15% of the base case. no agent, no hooks x 15,710 ops/sec ±1.17% (81 runs sampled)
no agent, noop async hooks x 2,386 ops/sec ±1.33% (78 runs sampled)
agent async hooks x 1,769 ops/sec ±0.91% (77 runs sampled)
instrumentation x 2,553 ops/sec ±0.99% (79 runs sampled)
Real-world applications do not see this extreme of overhead, but it is still noticeable. Unfortunately I don't have numbers for that at the moment. :/ Regarding user-land queueing, we've found that the majority of that is actually in the plethora of promise libraries out there. Every one of them manages its own micro task queue which must be independently instrumented. |
I have not started to implement Async Hooks in Sqreen agent yet. My plan was to evaluate overhead in a few open source apps before continuing but end of year being extremely busy, it is still in the backlog. If I have time to do this, I'll share my results and tests. |
We are using async-hooks in Node 8 for logging capabilities. Uniq transaction ids are passed through promise chains. This gives us detailed, correlated logging output. Not more that 10 lines of code for setup, but of great use. |
petkaantonov/bluebird#1472 (comment) shows a slow-down in bluebird when they use the |
Thanks everyone for your input here. I'm going to close this. I've added an item to issue #124 to figure out what "acceptable performance" impact is. In addition to performance, there's also a question around whether the API is "the right API". i.e., does the API solve the intended problems. If anyone has feedback on the utility of the API, please feel free to re-activate this, or open another issue. |
In the 2017-11-29 Diagnostics WG meeting, one of the issues that came up was a lack of insight into how async-hooks was preceived by it's early adopters, in particular early adopters who were using async-hooks at scale.
After discussion we decided to open an issue here to solicit feedback from ppl. In particular, can you chime in here with comments, including any details on the following:
Any cases where async-hooks is losing context or has incorrect context? If so, what are you doing about it? How did you detect the "lost context"?
Perf impact of async-hooks?
Any modules (native or non-native) that you're seeing that need updated to correctly propogate context?
/cc @watson (Elastic), @jkrems (groupon), @vdeturckheim (sqreen), @NatalieWolfe (new relic), @kjin (GCP/stack driver), @ofrobots (GCP/stack driver)
The text was updated successfully, but these errors were encountered: