feat: support asyncLocalStorage #1455
Conversation
Codecov ReportBase: 99.60% // Head: 99.61% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1455 +/- ##
=======================================
Coverage 99.60% 99.61%
=======================================
Files 5 5
Lines 508 516 +8
Branches 143 146 +3
=======================================
+ Hits 506 514 +8
Misses 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@fengmk2 if it is optional, why blocking the merge? It won't impact those who do not opt-in. Right? |
|
Performance aside, is it not better to have it as a separate package? IMO this is one of those feature-creep features 😄. |
|
It's non-obvious how to use async local storage in koa without breaking error handling. I'm going to give this a try. If users have to monkeypatch createContext to get this to work then I think this should be in core. |
@fengmk2: The overhead is 8% for Node 14. I haven't tested this particular PR but async local storage is not slow. Waiting for it to change to support it in Koa doesn't seem warranted. |
|
There's a couple of incorrect ways of doing this. I don't think it's a Koa issue though, the Node.js documentation (and my IDE's annotations) had Here's my first attempt that doesn't appear to break error handling: // Provides async local storage context throughout to koa allowing async operations executed
// during a request to later retrieve the koa ctx without requiring a reference to be passed
app.use(async function ContextMiddleware (ctx, next) {
const store = new Map()
store.set('ctx', ctx)
await asyncLocalStorage.run(store, async () => {
try {
await next()
} catch (error) {
ctx.throw(error)
}
})
})Usage with reqCustomProps: (req) => {
const ctx = asyncLocalStorage.getStore()?.get('ctx')
const span = tracer.getCurrentSpan()
return {
app: ctx?.state?.app,
requestId: req.id,
traceId: span?.spanContext?.traceId
}
}If we don't want to merge async-local-storage into core, I think we should at least document some approaches, their pros/cons and/or a preferred method if there is one. The issue I'm seeing is if you want to use open telemetry and structured logging, given the performance overhead, it would be ideal if there were a way to combine those responsibilities into an add-on that only mounts a single middleware. Perhaps if koa were to look into what the open telemetry instrumentation does, provide the requisite hooks to avoid shimming/wrapping. Once you add tracing, the From a performance perspective, if there were a way to accomplish this from a synchronous middleware that'd be ideal but I don't think that's possible. |
|
|
|
I'm sort of curious about the result. It looks like using new PromiseHook API significantly reduces overhead, to the point that this could be finalised and merged IMO. Though others might disagree. Running node 16.2 both master and async-hooks produced: # master branch
$ npm run bench
> koa@2.13.1 bench
> make -C benchmarks
1 middleware
27583.00
5 middleware
27694.75
10 middleware
27476.26
15 middleware
27925.56
20 middleware
27453.76
30 middleware
26357.53
50 middleware
26231.70
100 middleware
25393.45
1 async middleware
28234.63
5 async middleware
27832.86
10 async middleware
27335.61
15 async middleware
27702.56
20 async middleware
27448.03
30 async middleware
26193.47
50 async middleware
25181.45
100 async middleware
22653.18# async-hooks branch
$ npm run bench
> koa@2.11.0 bench
> make -C benchmarks
1 middleware
28156.92
5 middleware
27567.77
10 middleware
26594.23
15 middleware
27381.46
20 middleware
27166.58
30 middleware
26120.80
50 middleware
25935.13
100 middleware
24950.71
1 async middleware
26585.90
5 async middleware
26596.86
10 async middleware
26399.98
15 async middleware
25465.95
20 async middleware
26197.93
30 async middleware
25552.58
50 async middleware
23979.93
100 async middleware
21888.75This feels like I'm missing something though. @dead-horse can you confirm on your end? |
and I've tested in both node@16.1 and node@16.2, 16.2 performance is much better, I think we can consider supporting this feature in koa soon. |
Yep, I misspelled it :) I think naming could get a bit confusing. To follow Koa convention of delegating - instead of |
|
Any update on this? |
|
I'm inclined to merge this but I want an additional approve because this feels like kind of a feature creep (depending on perspective). I wonder if there's a different implementation approach to consider? Also, I wonder about the name Also, I think travis changes belongs in a different PR and we need conflicts resolved (and I would like to see a History addition here as well). |
|
update test with node 18.12.1 |
|
I will rebase and add node 16, 18 test and drop node < 14 support before merge and release major version. |
|
@jonathanong It's time to release koa v3. |
fengmk2
changed the title
| return this.handleRequest(ctx, fn) | ||
| } | ||
|
|
||
| return handleRequest | ||
| } | ||
|
|
||
| /** | ||
| * return currnect contenxt from async local storage |
|
Great work everyone getting this merged. Many thanks! 🥳 |
|
I don't think this PR is a good feature. This PR finally will be refactored to use a middleware. See #1718. But using a built-in middleware seems not good. It breaks the principle that koa core does NOT bundle any middleware. |
|
Is there an official documentation for this feature? |
|
@zanminkian fix on #1757 |
closes #1453
options.asyncLocalStorageto enableapp.ctxStorageto get the asyncLocalStorage instanceapp.currenctContextto get currenct contextUnfortunately, there is a large performance loss after opening. The deeper the async call stack, the greater the performance loss.