koa
koa copied to clipboard
koajs
[WIP] feat: support asyncLocalStorage
closes #1453
-
options.asyncLocalStorageto enable -
app.ctxStorageto get the asyncLocalStorage instance -
app.currenctContextto get currenct context - node 13+ required
Unfortunately, there is a large performance loss after opening. The deeper the async call stack, the greater the performance loss.
- without asyncLocalStorage
1 middleware
21312.11
5 middleware
20286.01
10 middleware
20702.86
15 middleware
21740.76
20 middleware
21164.56
30 middleware
21155.04
50 middleware
20994.66
100 middleware
19356.09
1 async middleware
21410.63
5 async middleware
21537.31
10 async middleware
19848.90
15 async middleware
19031.05
20 async middleware
18554.31
30 async middleware
19903.52
50 async middleware
18689.10
100 async middleware
17426.28
- with asyncLocalStorage
1 middleware
17065.02
5 middleware
16251.25
10 middleware
15339.71
15 middleware
16713.63
20 middleware
14572.50
30 middleware
13395.76
50 middleware
14125.53
100 middleware
13956.02
1 async middleware
14470.50
5 async middleware
11909.50
10 async middleware
10390.65
15 async middleware
8125.27
20 async middleware
7202.19
30 async middleware
5950.12
50 async middleware
4170.79
100 async middleware
2383.32
Codecov Report
Base: 99.60% // Head: 99.61% // Increases project coverage by +0.00% :tada:
Coverage data is based on head (
5b8ac93) compared to base (702eb13). Patch coverage: 100.00% of modified lines in pull request are covered.
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
| Impacted Files | Coverage Δ | |
|---|---|---|
| lib/application.js | 98.37% <100.00%> (+0.11%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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.
Need wait for asyncLocalStorage improve it's performance.
@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 run as a void. It apparently returns a promise and can/should be awaited. The documentation says that it returns the value of run.... I might need to read the Node.js source to verify the behavior.
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 koa-pino-logger to retrieve ctx in the logging context:
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 koa-instrumentation defaults to creating a span per middleware so the incentive to reduce the number of anonymous middleware becomes apparent.
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. @fengmk2: Any thoughts?
~~@dead-horse mind rerunning this PR bench against node 16.2 release that has updated async hooks that uses new v8::Context PromiseHook API?~~ Actually I got to it before you.
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
> [email protected] 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
> [email protected] 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.75
This feels like I'm missing something though. @dead-horse can you confirm on your end?
This feels like I'm missing something though. @dead-horse can you confirm on your end? Yep, you should change the benchmark test file to
new Koa({ asyncLocalStorage: true })to enable this feature.
and I've tested in both [email protected] and [email protected], 16.2 performance is much better, I think we can consider supporting this feature in koa soon.
#[email protected] without asyncLocalStorage
1 async middleware
21026.42
5 async middleware
20347.36
10 async middleware
20525.89
15 async middleware
20328.99
20 async middleware
20341.83
30 async middleware
19403.87
50 async middleware
18543.07
100 async middleware
16682.64
# [email protected] with asyncLocalStorage
1 async middleware
13851.01
5 async middleware
13827.81
10 async middleware
11712.27
15 async middleware
10615.83
20 async middleware
9528.57
30 async middleware
7698.06
50 async middleware
5840.24
100 async middleware
3443.92
# [email protected] with asyncLocalStorage
1 async middleware
18193.72
5 async middleware
17330.99
10 async middleware
16814.28
15 async middleware
16032.19
20 async middleware
15429.25
30 async middleware
13937.79
50 async middleware
12049.11
100 async middleware
9028.56
Yep, you should change the benchmark test file to new Koa({ asyncLocalStorage: true }) to enable this feature.
Yep, I misspelled it :)
I think naming could get a bit confusing. To follow Koa convention of delegating - instead of currentContext how about getStore?
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 currentContext vs getContext as previously mentioned.
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
# node 18.12.1 without asyncLocalStorage
1 middleware
24572.61
5 middleware
24792.98
10 middleware
24653.46
15 middleware
24135.85
20 middleware
24327.15
30 middleware
24284.75
50 middleware
23088.51
100 middleware
22117.29
1 async middleware
24379.84
5 async middleware
24484.20
10 async middleware
24110.51
15 async middleware
23677.08
20 async middleware
23476.87
30 async middleware
22917.99
50 async middleware
22108.90
100 async middleware
20184.91
# node 18.12.1 with asyncLocalStorage
1 middleware
24133.40
5 middleware
23972.34
10 middleware
22588.11
15 middleware
23257.67
20 middleware
22003.86
30 middleware
22063.52
50 middleware
22648.46
100 middleware
20524.94
1 async middleware
23208.18
5 async middleware
22686.83
10 async middleware
21315.18
15 async middleware
20162.39
20 async middleware
19588.33
30 async middleware
17118.10
50 async middleware
15384.44
100 async middleware
11298.86
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. Should we only support node >= 14?
I don't think this PR is a good feature. This PR finally will be refactored to use a middleware. See https://github.com/koajs/koa/pull/1718. But using a built-in middleware seems not good. It breaks the principle that koa core does NOT bundle any middleware.
Actually, we can create a @koa/async-local-storage middleware to achieve our goal. There is no enough reason to bundle a built-in middleware into koa core.
